From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: Push netlock down to ifconf() and move copyout() out of netlock
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Fri, 14 Nov 2025 20:26:11 +0300

On Fri, Nov 14, 2025 at 02:20:33PM +0100, Alexander Bluhm wrote:
> On Thu, Nov 13, 2025 at 09:44:46PM +0300, Vitaliy Makkoveev wrote:
> > Link interface and address descriptors to the `if_tmplist_lock'
> > protected temporary lists while holding netlock and then release it. Do
> > copyout() while following these lists and holding only
> > `if_tmplist_lock'.
> > 
> > The pagefault handler releases exclusive netlock while copyout(9) needs
> > to load pages from swap. So in the cases like ifconf() netlock protected
> > lists could became inconsistent. Since we use shared netlock, pagefault
> > handler leaves it held, but this breaks swap on NFS. I doubt someone
> > uses swap on NFS, but we hold netlock for unpredictable time.
> > 
> > ok?
> 
> I would prefer to have the cleanup cases in one block.  Instead of
> 
> 		...
> > +	}
> > +
> > +	while ((ifp = TAILQ_FIRST(&if_tmplist))) {
> > +		TAILQ_REMOVE(&if_tmplist, ifp, if_tmplist);
> > +		if_put(ifp);
> >  	}
> > -out:
> > +	rw_exit_write(&if_tmplist_lock);
> > +
> >  	ifc->ifc_len -= space;
> >  	return (0);
> > +
> > +free_addrlist:
> > +	while((ifa = TAILQ_FIRST(&addr_tmplist))) {
> > +		TAILQ_REMOVE(&addr_tmplist, ifa, ifa_tmplist);
> > +		ifafree(ifa);
> > +	}
> > +free_iflist:
> > +	while ((ifp = TAILQ_FIRST(&if_tmplist))) {
> > +		TAILQ_REMOVE(&if_tmplist, ifp, if_tmplist);
> > +		if_put(ifp);
> > +	}
> > +	rw_exit_write(&if_tmplist_lock);
> > +
> > +	if (error == 0)
> > +		ifc->ifc_len -= space;
> > +	return (error);
> >  }
> 
> 
> I would have written
> 
> 		...
> 	}
>  free:
> 	while((ifa = TAILQ_FIRST(&addr_tmplist))) {
> 		TAILQ_REMOVE(&addr_tmplist, ifa, ifa_tmplist);
> 		ifafree(ifa);
> 	}
> 	while ((ifp = TAILQ_FIRST(&if_tmplist))) {
> 		TAILQ_REMOVE(&if_tmplist, ifp, if_tmplist);
> 		if_put(ifp);
> 	}
> 	rw_exit_write(&if_tmplist_lock);
> 
> 	if (error == 0)
> 		ifc->ifc_len -= space;
> 	return (error);
> }
> 
> In the commomn case we do useless free and error checks.  But it
> is only 3 comaprisons.  And we do not have two exit paths.  Everything
> is always double checked and freed.  There is only one kind of goto
> free and you don't have to care to get the right one.
> 
> anyway OK bluhm@
> 

I will commit this version. This is not the hot path, so couple of
useless comparisons are not significant.