Re: [PATCH net-next 1/2] tcp: don't set window_clamp from SO_RCVBUF before the socket is connected

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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 fix is incomplete.
- [Medium] The commit message misstates and overstates the impact.

--

tcp: don't set window_clamp from SO_RCVBUF before the socket is connected

This patch makes tcp_set_rcvbuf() skip the window_clamp update while the
socket is in TCP_CLOSE or TCP_LISTEN. The aim is to stop a clamp derived
from SO_RCVBUF from overriding the RTAX_WINDOW route metric in
tcp_connect_init() and tcp_openreq_init_rwin().

> tcp_connect_init() and tcp_openreq_init_rwin() treat a non-zero
> window_clamp as a user clamp and, in that case, skip the route window
> metric (RTAX_WINDOW, "ip route ... window N"). A clamp left behind by an
> SO_RCVBUF call made before connect(), or on a listener, therefore
> changes the window and window scale advertised in the SYN and SYN-ACK:
> the route metric is no longer applied, and even without a route metric
> the advertised window and scale come from SO_RCVBUF instead of the
> defaults. This affects any application that sets SO_RCVBUF before
> connect() or listen(), which is more common than using the route metric.

[Severity: Medium]
Is the part about the case without a route metric accurate?

__sock_set_rcvbuf() always sets SOCK_RCVBUF_LOCK and then calls
tcp_set_rcvbuf(sk, sk->sk_rcvbuf). On the old kernel, the clamp left
behind is therefore tcp_win_from_space(sk, sk_rcvbuf), which equals
tcp_full_space(), or SOCK_MIN_RCVBUF / 2 if that is larger.
scaling_ratio cannot change before connect.

With no RTAX_WINDOW metric, the patched kernel sees window_clamp == 0 and
reads a zero metric. Then this block runs:

net/ipv4/tcp_output.c:tcp_connect_init() {
    ...
	if (sk->sk_userlocks & SOCK_RCVBUF_LOCK &&
	    (tp->window_clamp > tcp_full_space(sk) || tp->window_clamp == 0))
		WRITE_ONCE(tp->window_clamp, tcp_full_space(sk));
    ...
}

The old kernel reaches the same tcp_full_space() value through the
"window_clamp > tcp_full_space(sk)" branch. tcp_openreq_init_rwin() does
the same for req->rsk_window_clamp.

Wouldn't both kernels then pass the same window_clamp to
tcp_select_initial_window(), and advertise the same window and wscale in
the SYN and SYN-ACK?

If so, the visible change is limited to two cases:

- routes with an RTAX_WINDOW metric
- TCP_WINDOW_CLAMP followed by SO_RCVBUF on an unconnected socket or
  listener, where SO_RCVBUF no longer overwrites the clamp

The message does not mention the second case. Neither case is "any
application that sets SO_RCVBUF before connect() or listen()".

The message also seems to contradict itself further down:

> The "after" values match a kernel with b025461303d8 reverted; with no
> route metric the SYN window and scale are the defaults in both cases.

In both cases, wouldn't the values come from SO_RCVBUF via
tcp_full_space() rather than from the defaults?

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index f4c9101e5996..e523cb71b905 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1858,6 +1858,14 @@ int tcp_set_rcvlowat(struct sock *sk, int val)
>  
>  void tcp_set_rcvbuf(struct sock *sk, int val)
>  {
> +	/* Without a connection there is no clamp to update: it is chosen
> +	 * from the route's window metric and the buffer size when the
> +	 * connection is set up, see tcp_connect_init() and
> +	 * tcp_openreq_init_rwin().
> +	 */
> +	if ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))
> +		return;

[Severity: High]
Does this check cover every case where SO_RCVBUF is applied before
tcp_connect_init() runs?

tcp_v4_connect() and tcp_v6_connect() switch the socket to TCP_SYN_SENT
before tcp_connect()->tcp_connect_init(). A clamp written in that gap
still passes this check, and there seem to be two ways to get there.

The first is TCP_FASTOPEN_CONNECT with a cached cookie, where connect()
returns early:

net/ipv4/tcp_ipv4.c:tcp_v4_connect() {
    ...
	tcp_set_state(sk, TCP_SYN_SENT);
    ...
	if (tcp_fastopen_defer_connect(sk, &err))
		return err;
    ...
}

If the application calls setsockopt(SO_RCVBUF) before its first write(),
tcp_set_rcvbuf() sees TCP_SYN_SENT and stores a non-zero window_clamp.
tcp_sendmsg_fastopen() later calls tcp_connect()->tcp_connect_init().

The second is a sock_ops BPF program. tcp_connect() runs the callback
before tcp_connect_init():

net/ipv4/tcp_output.c:tcp_connect() {
    ...
	tcp_call_bpf(sk, BPF_SOCK_OPS_TCP_CONNECT_CB, 0, NULL);
    ...
	tcp_connect_init(sk);
    ...
}

sol_socket_sockopt() allows SO_RCVBUF for bpf_setsockopt(). That call
reaches __sock_set_rcvbuf()->tcp_set_rcvbuf() while the socket is in
TCP_SYN_SENT. samples/bpf/tcp_bufs_kern.c, tcp_clamp_kern.c and
tcp_iw_kern.c all use this pattern.

In both cases tcp_connect_init() then skips the route metric:

	if (!tp->window_clamp)
		WRITE_ONCE(tp->window_clamp, dst_metric(dst, RTAX_WINDOW));

The SOCK_RCVBUF_LOCK block afterwards can only lower the clamp to
tcp_full_space().

Doesn't that leave the b025461303d8 regression in place on these paths,
even though this patch carries the Fixes: tag for it? The selftest in this
series only covers SO_RCVBUF set before connect().

Would it work to base the check on whether tcp_connect_init() has run yet?
Another option would be to let tcp_connect_init() and
tcp_openreq_init_rwin() tell a TCP_WINDOW_CLAMP value apart from one
derived from sk_rcvbuf.

> +
>  	tcp_set_window_clamp(sk, tcp_win_from_space(sk, val));
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011111606.14753-1-jain.sm%40gmail.com