From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Unlock ICMPV6CTL_ERRPPSLIMIT case of icmp6_sysctl()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Fri, 24 Oct 2025 01:07:13 +0200

On Thu, Oct 23, 2025 at 11:56:36PM +0300, Vitaliy Makkoveev wrote:
> The last one of locked `icmpv6ctl_vars'.
> 
> `icmp6errppslim' is the integer loaded once within icmp6_ratelimit()
> which is accessed from two paths. The first one, icmp6_do_error() is
> always the end of the error path. The usage of the second one, the
> icmp6_redirect_output() is similar to already unlocked `ip6_forwarding',
> except that icmp6_ratelimit() called only once. So we don't need to
> cache `icmp6errppslim' anywhere.
> 
> Since icmp6_sysctl() became mp-safe, unlock it.
> 
> ok?

OK bluhm@

> Index: sys/netinet6/icmp6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> retrieving revision 1.279
> diff -u -p -r1.279 icmp6.c
> --- sys/netinet6/icmp6.c	16 Sep 2025 09:19:43 -0000	1.279
> +++ sys/netinet6/icmp6.c	23 Oct 2025 20:35:03 -0000
> @@ -1693,7 +1693,7 @@ icmp6_ratelimit(const struct in6_addr *d
>  {
>  	/* PPS limit */
>  	if (!ppsratecheck(&icmp6errppslim_last, &icmp6errpps_count,
> -	    icmp6errppslim))
> +	    atomic_load_int(&icmp6errppslim)))
>  		return 1;	/* The packet is subject to rate limit */
>  	return 0;		/* okay to send */
>  }
> @@ -1776,15 +1776,12 @@ icmp6_mtudisc_timeout(struct rtentry *rt
>  }
>  
>  #ifndef SMALL_KERNEL
> -const struct sysctl_bounded_args icmpv6ctl_vars_unlocked[] = {
> +const struct sysctl_bounded_args icmpv6ctl_vars[] = {
>  	{ ICMPV6CTL_ND6_DELAY, &nd6_delay, 0, INT_MAX },
>  	{ ICMPV6CTL_ND6_UMAXTRIES, &nd6_umaxtries, 0, INT_MAX },
>  	{ ICMPV6CTL_ND6_MMAXTRIES, &nd6_mmaxtries, 0, INT_MAX },
>  	{ ICMPV6CTL_MTUDISC_HIWAT, &icmp6_mtudisc_hiwat, 0, INT_MAX },
>  	{ ICMPV6CTL_MTUDISC_LOWAT, &icmp6_mtudisc_lowat, 0, INT_MAX },
> -};
> -
> -const struct sysctl_bounded_args icmpv6ctl_vars[] = {
>  	{ ICMPV6CTL_ERRPPSLIMIT, &icmp6errppslim, -1, 1000 },
>  };
>  
> @@ -1805,12 +1802,6 @@ icmp6_sysctl_icmp6stat(void *oldp, size_
>  	return (ret);
>  }
>  
> -/*
> - * Temporary, should be replaced with sysctl_lock after icmp6_sysctl()
> - * unlocking.
> - */
> -struct rwlock icmp6_sysctl_lock = RWLOCK_INITIALIZER("icmp6rwl"); 
> -
>  int
>  icmp6_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp,
>      void *newp, size_t newlen)
> @@ -1829,11 +1820,11 @@ icmp6_sysctl(int *name, u_int namelen, v
>  		error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
>  		    &newval, 0, INT_MAX);
>  		if (error == 0 && oldval != newval) {
> -			rw_enter_write(&icmp6_sysctl_lock);
> +			rw_enter_write(&sysctl_lock);
>  			atomic_store_int(&icmp6_redirtimeout, newval);
>  			rt_timer_queue_change(&icmp6_redirect_timeout_q,
>  			    newval);
> -			rw_exit_write(&icmp6_sysctl_lock);
> +			rw_exit_write(&sysctl_lock);
>  		}
>  
>  		return (error);
> @@ -1847,22 +1838,10 @@ icmp6_sysctl(int *name, u_int namelen, v
>  		    atomic_load_int(&ln_hold_total));
>  		break;
>  
> -	case ICMPV6CTL_ND6_DELAY:
> -	case ICMPV6CTL_ND6_UMAXTRIES:
> -	case ICMPV6CTL_ND6_MMAXTRIES:
> -	case ICMPV6CTL_MTUDISC_HIWAT:
> -	case ICMPV6CTL_MTUDISC_LOWAT:
> -		error = sysctl_bounded_arr(icmpv6ctl_vars_unlocked,
> -		    nitems(icmpv6ctl_vars_unlocked), name, namelen,
> -		    oldp, oldlenp, newp, newlen);
> -		break;
> -
>  	default:
> -		NET_LOCK();
>  		error = sysctl_bounded_arr(icmpv6ctl_vars,
>  		    nitems(icmpv6ctl_vars), name, namelen, oldp, oldlenp, newp,
>  		    newlen);
> -		NET_UNLOCK();
>  		break;
>  	}
>  
> Index: sys/netinet6/in6_proto.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6_proto.c,v
> retrieving revision 1.152
> diff -u -p -r1.152 in6_proto.c
> --- sys/netinet6/in6_proto.c	16 Sep 2025 09:19:16 -0000	1.152
> +++ sys/netinet6/in6_proto.c	23 Oct 2025 20:35:03 -0000
> @@ -169,7 +169,7 @@ const struct protosw inet6sw[] = {
>    .pr_type	= SOCK_RAW,
>    .pr_domain	= &inet6domain,
>    .pr_protocol	= IPPROTO_ICMPV6,
> -  .pr_flags	= PR_ATOMIC|PR_ADDR,
> +  .pr_flags	= PR_ATOMIC|PR_ADDR|PR_MPSYSCTL,
>    .pr_input	= icmp6_input,
>    .pr_ctlinput	= rip6_ctlinput,
>    .pr_ctloutput	= rip6_ctloutput,
> @@ -376,5 +376,5 @@ u_long	rip6_recvspace = RIPV6RCVQ;
>  
>  /* ICMPV6 parameters */
>  int	icmp6_redirtimeout = 10 * 60;	/* 10 minutes */
> -int	icmp6errppslim = 100;		/* 100pps */
> +int	icmp6errppslim = 100;		/* [a] 100pps */
>  int	ip6_mtudisc_timeout = IPMTUDISCTIMEOUT;	/* [a] */