From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Move copyout() out of netlock within sysctl_ifnames()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 4 Nov 2025 20:07:49 +0100

On Tue, Nov 04, 2025 at 12:16:26PM +0300, Vitaliy Makkoveev wrote:
> Bump reference counter and link desired interface descriptors into
> temporary `if_tmplist' while holding shared netlock. This temporary
> list is protected by `if_tmplist_lock' rwlock so release of the
> netlock is fine. Do copyout() while holding `if_tmplist_lock' and
> then tear down temporary list.
> 
> We follow this way in if_getgroupmembers() and related paths.

Two nitpicks:

You could write
	struct ifnet_head if_tmplist = TAILQ_HEAD_INITIALIZER(if_tmplist);
then it fits on a line.

We don't put space between TAILQ_FOREACH and (

anyway OK bluhm@

> Index: sys/net/rtsock.c
> ===================================================================
> RCS file: /cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.388
> diff -u -p -r1.388 rtsock.c
> --- sys/net/rtsock.c	21 Aug 2025 08:49:21 -0000	1.388
> +++ sys/net/rtsock.c	4 Nov 2025 12:26:25 -0000
> @@ -2110,14 +2110,24 @@ sysctl_iflist(int af, struct walkarg *w)
>  int
>  sysctl_ifnames(struct walkarg *w)
>  {
> +	TAILQ_HEAD(, ifnet)     if_tmplist =
> +	    TAILQ_HEAD_INITIALIZER(if_tmplist);
>  	struct if_nameindex_msg ifn;
>  	struct ifnet *ifp;
>  	int error = 0;
>  
> +	rw_enter_write(&if_tmplist_lock);
> +	NET_LOCK_SHARED();
>  	/* XXX ignore tableid for now */
>  	TAILQ_FOREACH(ifp, &ifnetlist, if_list) {
>  		if (w->w_arg && w->w_arg != ifp->if_index)
>  			continue;
> +		if_ref(ifp);
> +		TAILQ_INSERT_TAIL(&if_tmplist, ifp, if_tmplist);
> +	}
> +	NET_UNLOCK_SHARED();
> +
> +	TAILQ_FOREACH (ifp, &if_tmplist, if_tmplist) {
>  		w->w_needed += sizeof(ifn);
>  		if (w->w_where && w->w_needed <= w->w_given) {
>  
> @@ -2127,12 +2137,18 @@ sysctl_ifnames(struct walkarg *w)
>  			    sizeof(ifn.if_name));
>  			error = copyout(&ifn, w->w_where, sizeof(ifn));
>  			if (error)
> -				return (error);
> +				break;
>  			w->w_where += sizeof(ifn);
>  		}
>  	}
>  
> -	return (0);
> +	while ((ifp = TAILQ_FIRST(&if_tmplist))) {
> +		TAILQ_REMOVE(&if_tmplist, ifp, if_tmplist);
> +		if_put(ifp);
> +	}
> +	rw_exit_write(&if_tmplist_lock);
> +
> +	return (error);
>  }
>  
>  int
> @@ -2261,10 +2277,7 @@ sysctl_rtable(int *name, u_int namelen, 
>  		break;
>  
>  	case NET_RT_IFNAMES:
> -		NET_LOCK_SHARED();
> -		error = sysctl_ifnames(&w);
> -		NET_UNLOCK_SHARED();
> -		break;
> +		return (sysctl_ifnames(&w));
>  	}
>  	free(w.w_tmem, M_RTABLE, w.w_tmemsize);
>  	if (where) {