From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: sysctl: push netlock down to mrt{,6}_sysctl_mfc()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 24 Jun 2025 22:55:56 +0200

On Tue, Jun 24, 2025 at 11:22:56PM +0300, Vitaliy Makkoveev wrote:
> Move copyout() and sleeping M_WAITOK malloc(9) out of netlock. I like to
> keep exclusive netlock instead of shared because the walker callback
> does `rt_gateway' dereference. May be I'm too wary, but the locking
> relax could be done with separate diff.

OK bluhm@

> Also, the netlock could be pushed deep down to the `rtableid' loop, like
> below, to allow stack to run between iterations.
> 
>         for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
>                 NET_LOCK();
>                 rtable_walk(rtableid, AF_INET6, NULL, mrt6_rtwalk_mf6csysctl,
>                     &msa);
>                 NET_UNLOCK();
>         }

I am not convinced that this is a good idea.  Regrabbing the lock
often is expensive.  Better make mrt6_rtwalk_mf6csysctl MP safe.

> Index: sys/netinet/ip_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.414
> diff -u -p -r1.414 ip_input.c
> --- sys/netinet/ip_input.c	24 Jun 2025 18:05:51 -0000	1.414
> +++ sys/netinet/ip_input.c	24 Jun 2025 20:07:35 -0000
> @@ -1804,10 +1804,7 @@ ip_sysctl(int *name, u_int namelen, void
>  	case IPCTL_MRTMFC:
>  		if (newp)
>  			return (EPERM);
> -		NET_LOCK();
> -		error = mrt_sysctl_mfc(oldp, oldlenp);
> -		NET_UNLOCK();
> -		return (error);
> +		return (mrt_sysctl_mfc(oldp, oldlenp));
>  	case IPCTL_MRTVIF:
>  		if (newp)
>  			return (EPERM);
> Index: sys/netinet/ip_mroute.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> retrieving revision 1.147
> diff -u -p -r1.147 ip_mroute.c
> --- sys/netinet/ip_mroute.c	23 Jun 2025 20:56:38 -0000	1.147
> +++ sys/netinet/ip_mroute.c	24 Jun 2025 20:07:59 -0000
> @@ -524,10 +524,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
>  		msa.msa_len = *oldlenp;
>  	}
>  
> +	NET_LOCK();
>  	for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
>  		rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
>  		    &msa);
>  	}
> +	NET_UNLOCK();
>  
>  	if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
>  	    (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
> Index: sys/netinet6/ip6_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.277
> diff -u -p -r1.277 ip6_input.c
> --- sys/netinet6/ip6_input.c	24 Jun 2025 18:03:47 -0000	1.277
> +++ sys/netinet6/ip6_input.c	24 Jun 2025 20:08:19 -0000
> @@ -1531,10 +1531,7 @@ ip6_sysctl(int *name, u_int namelen, voi
>  	case IPV6CTL_MRTMFC:
>  		if (newp)
>  			return (EPERM);
> -		NET_LOCK();
> -		error = mrt6_sysctl_mfc(oldp, oldlenp);
> -		NET_UNLOCK();
> -		return (error);
> +		return (mrt6_sysctl_mfc(oldp, oldlenp));
>  #else
>  	case IPV6CTL_MRTSTATS:
>  	case IPV6CTL_MRTPROTO:
> Index: sys/netinet6/ip6_mroute.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_mroute.c,v
> retrieving revision 1.148
> diff -u -p -r1.148 ip6_mroute.c
> --- sys/netinet6/ip6_mroute.c	24 Jun 2025 18:03:47 -0000	1.148
> +++ sys/netinet6/ip6_mroute.c	24 Jun 2025 20:08:19 -0000
> @@ -472,10 +472,12 @@ mrt6_sysctl_mfc(void *oldp, size_t *oldl
>  		msa.ms6a_len = *oldlenp;
>  	}
>  
> +	NET_LOCK();
>  	for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
>  		rtable_walk(rtableid, AF_INET6, NULL, mrt6_rtwalk_mf6csysctl,
>  		    &msa);
>  	}
> +	NET_UNLOCK();
>  
>  	if (msa.ms6a_minfos != NULL && msa.ms6a_needed > 0 &&
>  	    (error = copyout(msa.ms6a_minfos, oldp, msa.ms6a_needed)) != 0) {