From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Unlock the ICMPV6CTL_MTUDISC_*WAT cases of icmp6_sysctl()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 12 Aug 2025 16:28:03 +0200

On Tue, Aug 12, 2025 at 04:23:39PM +0300, Vitaliy Makkoveev wrote:
> Also deny negative values for both `icmp6_mtudisc_*wat' variables. They
> are used together, but the usage path looks incomplete. Changing the API
> to disallow disabling watermarks would hurt nobody.

OK bluhm@

> Index: sys/netinet6/icmp6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> diff -u -p -r1.276 icmp6.c
> --- sys/netinet6/icmp6.c	4 Aug 2025 10:44:43 -0000	1.276
> +++ sys/netinet6/icmp6.c	12 Aug 2025 13:15:06 -0000
> @@ -97,6 +97,11 @@
>  #include <net/pfvar.h>
>  #endif
>  
> +/*
> + * Locks used to protect data:
> + *	a	atomic
> + */
> +
>  struct cpumem *icmp6counters;
>  
>  extern int icmp6errppslim;
> @@ -117,8 +122,8 @@ LIST_HEAD(, icmp6_mtudisc_callback) icmp
>  struct rttimer_queue icmp6_mtudisc_timeout_q;
>  
>  /* XXX do these values make any sense? */
> -static int icmp6_mtudisc_hiwat = 1280;
> -static int icmp6_mtudisc_lowat = 256;
> +static int icmp6_mtudisc_hiwat = 1280;	/* [a] */
> +static int icmp6_mtudisc_lowat = 256;	/* [a] */
>  
>  /*
>   * keep track of # of redirect routes.
> @@ -962,16 +967,15 @@ icmp6_mtudisc_update(struct ip6ctlparam 
>  	 */
>  	rtcount = rt_timer_queue_count(&icmp6_mtudisc_timeout_q);
>  	if (validated) {
> -		if (0 <= icmp6_mtudisc_hiwat && rtcount > icmp6_mtudisc_hiwat)
> +		if (rtcount > atomic_load_int(&icmp6_mtudisc_hiwat))
>  			return;
> -		else if (0 <= icmp6_mtudisc_lowat &&
> -		    rtcount > icmp6_mtudisc_lowat) {
> +		else if (rtcount > atomic_load_int(&icmp6_mtudisc_lowat)) {
>  			/*
>  			 * XXX nuke a victim, install the new one.
>  			 */
>  		}
>  	} else {
> -		if (0 <= icmp6_mtudisc_lowat && rtcount > icmp6_mtudisc_lowat)
> +		if (rtcount > atomic_load_int(&icmp6_mtudisc_lowat))
>  			return;
>  	}
>  
> @@ -1776,13 +1780,13 @@ const struct sysctl_bounded_args icmpv6c
>  	{ 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 },
>  	{ ICMPV6CTL_ND6_MAXNUDHINT, &nd6_maxnudhint, 0, INT_MAX },
> -	{ ICMPV6CTL_MTUDISC_HIWAT, &icmp6_mtudisc_hiwat, -1, INT_MAX },
> -	{ ICMPV6CTL_MTUDISC_LOWAT, &icmp6_mtudisc_lowat, -1, INT_MAX },
>  };
>  
>  int
> @@ -1847,6 +1851,8 @@ icmp6_sysctl(int *name, u_int namelen, v
>  	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);
> Index: sys/sys/epoll.h
> ===================================================================
> RCS file: sys/sys/epoll.h
> diff -N sys/sys/epoll.h