From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Unlock IPV6CTL_LOG_INTERVAL case of ip6_sysctl()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Wed, 23 Jul 2025 14:57:19 +0200

On Mon, Jul 21, 2025 at 06:53:41PM +0300, Vitaliy Makkoveev wrote:
> Both usage paths of ip6_forward() are dead ends. We don't need to cache
> value.
> 
> IPV6CTL_LOG_INTERVAL is also unused by ramdisk.

OK bluhm@

In ip6_mforward() calling getuptime() twice looks wrong.  Writing
to ip6_log_time in parallel is fishy.  Usually we have the ratecheck(9)
API for this.  In practice not much will go wrong.  All these
problems are unrelated to your diff.

> Index: sys/netinet6/in6_proto.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6_proto.c,v
> diff -u -p -r1.137 in6_proto.c
> --- sys/netinet6/in6_proto.c	21 Jul 2025 11:07:31 -0000	1.137
> +++ sys/netinet6/in6_proto.c	21 Jul 2025 12:03:21 -0000
> @@ -356,7 +356,7 @@ int	ip6_defhlim = IPV6_DEFHLIM;	/* [a] *
>  int	ip6_defmcasthlim = IPV6_DEFAULT_MULTICAST_HOPS;
>  int	ip6_maxfragpackets = 200;	/* [a] */
>  int	ip6_maxfrags = 200;
> -int	ip6_log_interval = 5;
> +int	ip6_log_interval = 5;	/* [a] */
>  int	ip6_hdrnestlimit = 10;	/* appropriate? */
>  int	ip6_dad_count = 1;	/* DupAddrDetectionTransmits */
>  int	ip6_dad_pending;	/* number of currently running DADs */
> Index: sys/netinet6/ip6_forward.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_forward.c,v
> diff -u -p -r1.127 ip6_forward.c
> --- sys/netinet6/ip6_forward.c	8 Jul 2025 00:47:41 -0000	1.127
> +++ sys/netinet6/ip6_forward.c	21 Jul 2025 12:03:21 -0000
> @@ -110,7 +110,8 @@ ip6_forward(struct mbuf *m, struct route
>  		ip6stat_inc(ip6s_cantforward);
>  		uptime = getuptime();
>  
> -		if (ip6_log_time + ip6_log_interval < uptime) {
> +		if (ip6_log_time + atomic_load_int(&ip6_log_interval) <
> +		    uptime) {
>  			ip6_log_time = uptime;
>  			inet_ntop(AF_INET6, &ip6->ip6_src, src6, sizeof(src6));
>  			inet_ntop(AF_INET6, &ip6->ip6_dst, dst6, sizeof(dst6));
> @@ -227,7 +228,8 @@ reroute:
>  		ip6stat_inc(ip6s_badscope);
>  		uptime = getuptime();
>  
> -		if (ip6_log_time + ip6_log_interval < uptime) {
> +		if (ip6_log_time + atomic_load_int(&ip6_log_interval) <
> +		    uptime) {
>  			ip6_log_time = uptime;
>  			inet_ntop(AF_INET6, &ip6->ip6_src, src6, sizeof(src6));
>  			inet_ntop(AF_INET6, &ip6->ip6_dst, dst6, sizeof(dst6));
> Index: sys/netinet6/ip6_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> diff -u -p -r1.281 ip6_input.c
> --- sys/netinet6/ip6_input.c	21 Jul 2025 11:07:31 -0000	1.281
> +++ sys/netinet6/ip6_input.c	21 Jul 2025 12:03:21 -0000
> @@ -1448,10 +1448,10 @@ const struct sysctl_bounded_args ipv6ctl
>  #endif
>  	{ IPV6CTL_DEFHLIM, &ip6_defhlim, 0, 255 },
>  	{ IPV6CTL_MAXFRAGPACKETS, &ip6_maxfragpackets, 0, 1000 },
> +	{ IPV6CTL_LOG_INTERVAL, &ip6_log_interval, 0, INT_MAX },
>  };
>  
>  const struct sysctl_bounded_args ipv6ctl_vars[] = {
> -	{ IPV6CTL_LOG_INTERVAL, &ip6_log_interval, 0, INT_MAX },
>  	{ IPV6CTL_HDRNESTLIMIT, &ip6_hdrnestlimit, 0, 100 },
>  	{ IPV6CTL_DAD_COUNT, &ip6_dad_count, 0, 10 },
>  	{ IPV6CTL_AUTO_FLOWLABEL, &ip6_auto_flowlabel, 0, 1 },
> @@ -1571,6 +1571,7 @@ ip6_sysctl(int *name, u_int namelen, voi
>  #endif
>  	case IPV6CTL_DEFHLIM:
>  	case IPV6CTL_MAXFRAGPACKETS:
> +	case IPV6CTL_LOG_INTERVAL:
>  		return (sysctl_bounded_arr(
>  		    ipv6ctl_vars_unlocked, nitems(ipv6ctl_vars_unlocked),
>  		    name, namelen, oldp, oldlenp, newp, newlen));
> Index: sys/netinet6/ip6_mroute.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_mroute.c,v
> diff -u -p -r1.150 ip6_mroute.c
> --- sys/netinet6/ip6_mroute.c	8 Jul 2025 00:47:41 -0000	1.150
> +++ sys/netinet6/ip6_mroute.c	21 Jul 2025 12:03:21 -0000
> @@ -923,7 +923,8 @@ ip6_mforward(struct ip6_hdr *ip6, struct
>  	 */
>  	if (IN6_IS_ADDR_UNSPECIFIED(&ip6->ip6_src)) {
>  		ip6stat_inc(ip6s_cantforward);
> -		if (ip6_log_time + ip6_log_interval < getuptime()) {
> +		if (ip6_log_time + atomic_load_int(&ip6_log_interval) <
> +		    getuptime()) {
>  			char src[INET6_ADDRSTRLEN], dst[INET6_ADDRSTRLEN];
>  
>  			ip6_log_time = getuptime();