From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Unlock IPCTL_MFORWARDING case of ip_sysctl()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 15 Jul 2025 15:32:31 +0200

On Tue, Jul 15, 2025 at 12:27:15PM +0300, Vitaliy Makkoveev wrote:
> Atomically accessed boolean. We don't need to use cached value in
> ip_output(). While following through "reroute" label the `ifp' pointer
> is NULL, so we don't reach the "if (atomic_load_int(&ipmforwarding) &&"
> check.

The call path ip_input_if() -> ip_mforward() -> ip_mdq() -> ip_output()
-> ip_mforward() is also safe.  Check (flags & IP_FORWARDING) == 0)
prevents that variable ipmforwarding is evaluated again.

OK bluhm@

> Index: sys/netinet/ip_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> diff -u -p -r1.418 ip_input.c
> --- sys/netinet/ip_input.c	15 Jul 2025 08:27:47 -0000	1.418
> +++ sys/netinet/ip_input.c	15 Jul 2025 09:21:42 -0000
> @@ -92,7 +92,7 @@
>  
>  /* values controllable via sysctl */
>  int	ip_forwarding = 0;			/* [a] */
> -int	ipmforwarding = 0;
> +int	ipmforwarding = 0;			/* [a] */
>  int	ipmultipath = 0;			/* [a] */
>  int	ip_sendredirects = 1;			/* [a] */
>  int	ip_dosourceroute = 0;			/* [a] */
> @@ -124,10 +124,10 @@ const struct sysctl_bounded_args ipctl_v
>  	{ IPCTL_IPPORT_HIFIRSTAUTO, &ipport_hifirstauto, 0, 65535 },
>  	{ IPCTL_IPPORT_HILASTAUTO, &ipport_hilastauto, 0, 65535 },
>  	{ IPCTL_IPPORT_MAXQUEUE, &ip_maxqueue, 0, 10000 },
> +	{ IPCTL_MFORWARDING, &ipmforwarding, 0, 1 },
>  };
>  
>  const struct sysctl_bounded_args ipctl_vars[] = {
> -	{ IPCTL_MFORWARDING, &ipmforwarding, 0, 1 },
>  	{ IPCTL_ARPTIMEOUT, &arpt_keep, 0, INT_MAX },
>  	{ IPCTL_ARPDOWN, &arpt_down, 0, INT_MAX },
>  };
> @@ -529,7 +529,8 @@ ip_input_if(struct mbuf **mp, int *offp,
>  		m->m_flags |= M_MCAST;
>  
>  #ifdef MROUTING
> -		if (ipmforwarding && ip_mrouter[ifp->if_rdomain]) {
> +		if (atomic_load_int(&ipmforwarding) &&
> +		    ip_mrouter[ifp->if_rdomain]) {
>  			int error;
>  
>  			if (m->m_flags & M_EXT) {
> @@ -1843,6 +1844,7 @@ ip_sysctl(int *name, u_int namelen, void
>  	case IPCTL_IPPORT_HIFIRSTAUTO:
>  	case IPCTL_IPPORT_HILASTAUTO:
>  	case IPCTL_IPPORT_MAXQUEUE:
> +	case IPCTL_MFORWARDING:
>  		return (sysctl_bounded_arr(
>  		    ipctl_vars_unlocked, nitems(ipctl_vars_unlocked),
>  		    name, namelen, oldp, oldlenp, newp, newlen));
> Index: sys/netinet/ip_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> diff -u -p -r1.412 ip_output.c
> --- sys/netinet/ip_output.c	8 Jul 2025 00:47:41 -0000	1.412
> +++ sys/netinet/ip_output.c	15 Jul 2025 09:21:42 -0000
> @@ -324,7 +324,8 @@ reroute:
>  			 * above, will be forwarded by the ip_input() routine,
>  			 * if necessary.
>  			 */
> -			if (ipmforwarding && ip_mrouter[ifp->if_rdomain] &&
> +			if (atomic_load_int(&ipmforwarding) &&
> +			    ip_mrouter[ifp->if_rdomain] &&
>  			    (flags & IP_FORWARDING) == 0) {
>  				int rv;
>