Re: [PATCH net] tcp: limit tcp_tso_rtt_log to a valid shift count

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch puts the new bound on the wrong sysctl, so it does not
  fix the bug it claims to fix.

--

tcp: limit tcp_tso_rtt_log to a valid shift count

This patch tries to cap the net.ipv4.tcp_tso_rtt_log sysctl at 31. The
reason is that tcp_tso_autosize() right-shifts a u32 by that value. The
patch also documents the accepted range [0, 31] in ip-sysctl.rst.

> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index e3760daa34706..e954311405b41 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c

[ ... ]

> @@ -778,6 +779,7 @@ static const struct ctl_table ipv4_net_table[] = {
>  		.maxlen		= sizeof(u8),
>  		.mode		= 0644,
>  		.proc_handler	= proc_dou8vec_minmax,
> +		.extra2		= &tcp_tso_rtt_log_max,
>  	},
>  	{
>  		.procname	= "ip_early_demux",

[Severity: High]
Was this hunk applied to the wrong ctl_table entry?

The entry just before "ip_early_demux" is "ip_dynaddr". With this patch
applied, ipv4_net_table reads:

	{
		.procname	= "ip_dynaddr",
		.data		= &init_net.ipv4.sysctl_ip_dynaddr,
		.maxlen		= sizeof(u8),
		.mode		= 0644,
		.proc_handler	= proc_dou8vec_minmax,
		.extra2		= &tcp_tso_rtt_log_max,
	},

The "tcp_tso_rtt_log" entry further down is unchanged and still has no
extra2:

	{
		.procname	= "tcp_tso_rtt_log",
		.data		= &init_net.ipv4.sysctl_tcp_tso_rtt_log,
		.maxlen		= sizeof(u8),
		.mode		= 0644,
		.proc_handler	= proc_dou8vec_minmax,
	},

When extra2 is NULL, proc_dou8vec_minmax() falls back to its default
maximum:

	unsigned int min = 0, max = 255U, val;
	...
	if (!tmp.extra2)
		tmp.extra2 = (unsigned int *) &max;

So writes of 32..255 to net.ipv4.tcp_tso_rtt_log are still accepted, and
tcp_tso_autosize() still does:

	r = tcp_min_rtt(tcp_sk(sk)) >> READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_tso_rtt_log);

Is the UBSAN shift-out-of-bounds from the commit message still reachable
through tcp_sendmsg_locked()->tcp_push()->tcp_write_xmit()->tcp_tso_segs()
->tcp_tso_autosize()? The r < BITS_PER_TYPE(sk->sk_gso_max_size) check
after it guards a different shift.

This also means the kernel does not enforce the new "Possible values are
[0, 31], inclusive." line in ip-sysctl.rst. This statement in the commit
message doesn't seem to match the diff:

    values 32 and 255 returned -EINVAL

Separately, net.ipv4.ip_dynaddr now rejects writes of 32..255 with
-EINVAL. Before this patch it accepted any u8 value.
inet_sk_rebuild_header() checks !sysctl_ip_dynaddr, and
inet_sk_reselect_saddr() checks sysctl_ip_dynaddr > 1, so both accept
larger values. With the Cc: stable tag, this change in what the sysctl
accepts would also reach stable kernels.

Should the extra2 line go in the tcp_tso_rtt_log entry instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_53CDCF8AA0022B75CF72A294CB9993D2FF07%40qq.com