From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: sysctl_source(): move copyout(9) out of netlock
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Thu, 23 Jan 2025 22:30:26 +0100

On Thu, Jan 23, 2025 at 11:10:37PM +0300, Vitaliy Makkoveev wrote:
> On Thu, Jan 23, 2025 at 07:55:05PM +0100, Alexander Bluhm wrote:
> > On Thu, Jan 23, 2025 at 08:15:25PM +0300, Vitaliy Makkoveev wrote:
> > > Netlock required only to store data to local variable, the rest could be
> > > done lockless. sysctl_source() is local to net/rtsock.c.
> > 
> > struct sockaddr sa is not enough to store all adreses families
> 
> Sorry, every time forget about it. This diff uses union of sockaddr_in
> and sockaddr_in6 as temporary buffer. 

OK bluhm@

> Index: sys/net/rtsock.c
> ===================================================================
> RCS file: /cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.377
> diff -u -p -r1.377 rtsock.c
> --- sys/net/rtsock.c	9 Jan 2025 18:20:29 -0000	1.377
> +++ sys/net/rtsock.c	23 Jan 2025 20:05:14 -0000
> @@ -2136,11 +2136,17 @@ sysctl_ifnames(struct walkarg *w)
>  int
>  sysctl_source(int af, u_int tableid, struct walkarg *w)
>  {
> +	union {
> +		struct sockaddr_in in;
> +#ifdef INET6
> +		struct sockaddr_in6 in6;
> +#endif
> +	}		 buf;
>  	struct sockaddr	*sa;
>  	int		 size, error = 0;
>  
> -	sa = rtable_getsource(tableid, af);
> -	if (sa) {
> +	NET_LOCK_SHARED();
> +	if ((sa = rtable_getsource(tableid, af)) != NULL) {
>  		switch (sa->sa_family) {
>  		case AF_INET:
>  			size = sizeof(struct sockaddr_in);
> @@ -2151,11 +2157,19 @@ sysctl_source(int af, u_int tableid, str
>  			break;
>  #endif
>  		default:
> -			return (0);
> +			sa = NULL;
> +			break;
>  		}
> +
> +	}
> +	if (sa != NULL)
> +		memcpy(&buf, sa, size);
> +	NET_UNLOCK_SHARED();
> +
> +	if (sa != NULL) {
>  		w->w_needed += size;
>  		if (w->w_where && w->w_needed <= w->w_given) {
> -			if ((error = copyout(sa, w->w_where, size)))
> +			if ((error = copyout(&buf, w->w_where, size)))
>  				return (error);
>  			w->w_where += size;
>  		}
> @@ -2236,7 +2250,6 @@ sysctl_rtable(int *name, u_int namelen, 
>  		tableid = w.w_arg;
>  		if (!rtable_exists(tableid))
>  			return (ENOENT);
> -		NET_LOCK_SHARED();
>  		for (i = 1; i <= AF_MAX; i++) {
>  			if (af != 0 && af != i)
>  				continue;
> @@ -2247,7 +2260,6 @@ sysctl_rtable(int *name, u_int namelen, 
>  			if (error)
>  				break;
>  		}
> -		NET_UNLOCK_SHARED();
>  		break;
>  	}
>  	free(w.w_tmem, M_RTABLE, w.w_tmemsize);