Re: [PATCH net 1/5] ipv6: restrict gc_elasticity to 31

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "ipv6: restrict gc_elasticity to 31" says
  only "Cap it at 31", and its reasoning covers only values >= 32.

--

ipv6: restrict gc_elasticity to 31

This commit switches the net.ipv6.route.gc_elasticity sysctl from
proc_dointvec to proc_dointvec_minmax with bounds [0, 31]. The upper bound
avoids an out-of-bounds shift of the u32 ip6_rt_gc_expire in ip6_dst_gc()
that UBSAN reported.

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 94e5ef1c4f83e..6f2efc219f395 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c

[ ... ]

> @@ -6627,7 +6629,9 @@ static const struct ctl_table ipv6_route_table_template[] = {
>  		.data		=	&init_net.ipv6.sysctl.ip6_rt_gc_elasticity,
>  		.maxlen		=	sizeof(int),
>  		.mode		=	0644,
> -		.proc_handler	=	proc_dointvec,
> +		.proc_handler	=	proc_dointvec_minmax,
> +		.extra1		=	SYSCTL_ZERO,

[Severity: Low]
This isn't a bug, but should the commit message mention the new lower
bound too?

The changelog says:

  Cap it at 31. 31 and 32 are equivalent anyway [ ... ]
  Values above 31 add no new behaviour, they only hit the UB.

This only covers values >= 32. With extra1 set to SYSCTL_ZERO, negative
writes that proc_dointvec used to accept will now fail as well.

The lower bound also fixes real UB. In ip6_dst_gc(), rt_elasticity is a
signed int read from the sysctl:

	int rt_elasticity = READ_ONCE(net->ipv6.sysctl.ip6_rt_gc_elasticity);
	[ ... ]
	atomic_set(&net->ipv6.ip6_rt_gc_expire, val - (val >> rt_elasticity));

so a negative value also gives a negative shift count.

"Cap" also reads as if values get clamped. proc_dointvec_minmax actually
rejects writes outside [0, 31] with -EINVAL.

Since this is tagged for stable, could the changelog say that writes
outside [0, 31] are now rejected in both directions? That would help
anyone with a negative or >= 32 value in sysctl.conf who starts seeing
the write fail.

> +		.extra2		=	&ip6_rt_gc_elasticity_max,
>  	},
>  	{
>  		.procname	=	"mtu_expires",

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023091419.167088-1-jiayuan.chen%40linux.dev