Index | Thread | Search

From:
Alexander Bluhm <bluhm@openbsd.org>
Subject:
Re: sysctl: unlock IPV6CTL_MAXFRAGPACKETS case of ip6_sysctl()
To:
Vitaliy Makkoveev <mvs@openbsd.org>
Cc:
tech@openbsd.org
Date:
Mon, 21 Jul 2025 11:29:38 +0200

Download raw body.

Thread
On Fri, Jul 18, 2025 at 08:54:12PM +0300, Vitaliy Makkoveev wrote:
> On Fri, Jul 18, 2025 at 03:19:41PM +0200, Alexander Bluhm wrote:
> > On Fri, Jul 18, 2025 at 12:14:30PM +0300, Vitaliy Makkoveev wrote:
> > > Use cached value in both frag6_input() and frag6_slowtimo().
> > 
> > Can you make ip6_maxfragpackets_local u_int and avoid the casts?
> > 
> > otherwise OK bluhm@
> > 
> 
> The stack and the sysctl interface are inconsistent. It is assumed that
> `ip6_maxfragpackets' could be negative, and we accept all fragments in
> this case, but sysctl interface doesn't allow to set negative value to
> `ip6_maxfragpackets'.

The limitation in sysctl that forces ip6_maxfragpackets into 0 to
1000 boundaries is newer than this the ip6_maxfragpackets >= 0
check.

> However, frag6_slowtimo() omits the "ip6_maxfragpackets >= 0" check, and
> seems it doesn't accept such case, so I think we should adjust the
> commentary in the frag6_input().

Accepting unlimited fragemnts is a bad idea anyway.  So I am fine
with removing this check.

> Note, we don't need extra "ip6_maxfragpackets == 0" check because if
> `ip6_maxfragpackets' is 0 the "frag6_nfragpackets >= ip6_maxfragpackets"
> check will be true in any case.
> 
> ok?

OK bluhm@

> Index: sys/netinet6/frag6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/frag6.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 frag6.c
> --- sys/netinet6/frag6.c	8 Jul 2025 00:47:41 -0000	1.93
> +++ sys/netinet6/frag6.c	18 Jul 2025 17:43:20 -0000
> @@ -196,11 +196,9 @@ frag6_input(struct mbuf **mp, int *offp,
>  		 * Enforce upper bound on number of fragmented packets
>  		 * for which we attempt reassembly;
>  		 * If maxfragpackets is 0, never accept fragments.
> -		 * If maxfragpackets is -1, accept all fragments without
> -		 * limitation.
>  		 */
> -		if (ip6_maxfragpackets >= 0 &&
> -		    frag6_nfragpackets >= (u_int)ip6_maxfragpackets) {
> +		if (frag6_nfragpackets >=
> +		    atomic_load_int(&ip6_maxfragpackets)) {
>  			mtx_leave(&frag6_mutex);
>  			goto dropfrag;
>  		}
> @@ -574,6 +572,7 @@ frag6_slowtimo(void)
>  {
>  	struct ip6q_head rmq6;
>  	struct ip6q *q6, *nq6;
> +	u_int ip6_maxfragpackets_local = atomic_load_int(&ip6_maxfragpackets);
>  
>  	TAILQ_INIT(&rmq6);
>  
> @@ -591,7 +590,7 @@ frag6_slowtimo(void)
>  	 * (due to the limit being lowered), drain off
>  	 * enough to get down to the new limit.
>  	 */
> -	while (frag6_nfragpackets > (u_int)ip6_maxfragpackets &&
> +	while (frag6_nfragpackets > ip6_maxfragpackets_local &&
>  	    !TAILQ_EMPTY(&frag6_queue)) {
>  		ip6stat_inc(ip6s_fragoverflow);
>  		frag6_unlink(TAILQ_LAST(&frag6_queue, ip6q_head), &rmq6);
> Index: sys/netinet6/in6_proto.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6_proto.c,v
> retrieving revision 1.136
> diff -u -p -r1.136 in6_proto.c
> --- sys/netinet6/in6_proto.c	18 Jul 2025 08:39:14 -0000	1.136
> +++ sys/netinet6/in6_proto.c	18 Jul 2025 17:43:20 -0000
> @@ -354,7 +354,7 @@ int	ip6_multipath = 0;	/* [a] no using m
>  int	ip6_sendredirects = 1;	/* [a] */
>  int	ip6_defhlim = IPV6_DEFHLIM;	/* [a] */
>  int	ip6_defmcasthlim = IPV6_DEFAULT_MULTICAST_HOPS;
> -int	ip6_maxfragpackets = 200;
> +int	ip6_maxfragpackets = 200;	/* [a] */
>  int	ip6_maxfrags = 200;
>  int	ip6_log_interval = 5;
>  int	ip6_hdrnestlimit = 10;	/* appropriate? */
> Index: sys/netinet6/ip6_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.280
> diff -u -p -r1.280 ip6_input.c
> --- sys/netinet6/ip6_input.c	18 Jul 2025 08:39:14 -0000	1.280
> +++ sys/netinet6/ip6_input.c	18 Jul 2025 17:43:20 -0000
> @@ -1447,10 +1447,10 @@ const struct sysctl_bounded_args ipv6ctl
>  	{ IPV6CTL_MRTPROTO, &ip6_mrtproto, SYSCTL_INT_READONLY },
>  #endif
>  	{ IPV6CTL_DEFHLIM, &ip6_defhlim, 0, 255 },
> +	{ IPV6CTL_MAXFRAGPACKETS, &ip6_maxfragpackets, 0, 1000 },
>  };
>  
>  const struct sysctl_bounded_args ipv6ctl_vars[] = {
> -	{ IPV6CTL_MAXFRAGPACKETS, &ip6_maxfragpackets, 0, 1000 },
>  	{ IPV6CTL_LOG_INTERVAL, &ip6_log_interval, 0, INT_MAX },
>  	{ IPV6CTL_HDRNESTLIMIT, &ip6_hdrnestlimit, 0, 100 },
>  	{ IPV6CTL_DAD_COUNT, &ip6_dad_count, 0, 10 },
> @@ -1570,6 +1570,7 @@ ip6_sysctl(int *name, u_int namelen, voi
>  	case IPV6CTL_MRTPROTO:
>  #endif
>  	case IPV6CTL_DEFHLIM:
> +	case IPV6CTL_MAXFRAGPACKETS:
>  		return (sysctl_bounded_arr(
>  		    ipv6ctl_vars_unlocked, nitems(ipv6ctl_vars_unlocked),
>  		    name, namelen, oldp, oldlenp, newp, newlen));