Re: [PATCH net-next v4 1/2] tcp: keep the route window metric when SO_RCVBUF is set before connect

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Medium] tcp: unordered lockless reads of window_clamp in
  tcp_req_window_clamp()
- [Low] The changelog says: "Without an RTAX_WINDOW metric and without
  TCP_WINDOW_CLAMP nothing changes".

Pre-existing issues:
- [Medium] The patch adds tp->window_clamp_user and comments that
  window_clamp "never goes above a clamp the user set with…

--

tcp: keep the route window metric when SO_RCVBUF is set before connect

This patch adds tp->window_clamp_user to record the clamp set with
TCP_WINDOW_CLAMP. tcp_connect_init(), tcp_openreq_init_rwin() and the SYN
cookie paths now use it, instead of any non-zero window_clamp, to decide
whether to apply the RTAX_WINDOW route metric. tcp_set_rcvbuf() now caps
window_clamp at the user's clamp.

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 99d7237a623d8..64465bc257d18 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1859,7 +1859,15 @@ int tcp_set_rcvlowat(struct sock *sk, int val)
>  
>  void tcp_set_rcvbuf(struct sock *sk, int val)
>  {
> -	tcp_set_window_clamp(sk, tcp_win_from_space(sk, val));
> +	u32 user_clamp = tcp_sk(sk)->window_clamp_user;
> +	int win = tcp_win_from_space(sk, val);
> +
> +	/* window_clamp follows the receive buffer, but never goes above a
> +	 * clamp the user set with TCP_WINDOW_CLAMP.
> +	 */
> +	if (user_clamp)
> +		win = min_t(u32, win, user_clamp);
> +	tcp_set_window_clamp(sk, win);
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but the new comment says
window_clamp never goes above a clamp set with TCP_WINDOW_CLAMP. Only
tcp_set_rcvbuf() enforces that. The other writers of tp->window_clamp
still ignore window_clamp_user.

tcp_measure_rcv_mss() resets window_clamp from sk_rcvbuf every time
scaling_ratio changes, even with SOCK_RCVBUF_LOCK set:

net/ipv4/tcp_input.c:tcp_measure_rcv_mss() {
    ...
			if (old_ratio != tcp_sk(sk)->scaling_ratio) {
				struct tcp_sock *tp = tcp_sk(sk);

				val = tcp_win_from_space(sk, sk->sk_rcvbuf);
				WRITE_ONCE(tp->window_clamp, val);
    ...
}

tcp_rcvbuf_grow() raises it during DRS autotuning:

net/ipv4/tcp_input.c:tcp_rcvbuf_grow() {
    ...
		WRITE_ONCE(tp->window_clamp,
			   tcp_win_from_space(sk, rcvbuf));
    ...
}

tcp_set_rcvlowat() raises a non-zero clamp in any state, including
TCP_LISTEN:

net/ipv4/tcp.c:tcp_set_rcvlowat() {
    ...
		if (tp->window_clamp && tp->window_clamp < val)
			WRITE_ONCE(tp->window_clamp, val);
    ...
}

mptcp_set_rcvlowat() also writes the subflow window_clamp directly.

Take an established socket that sets TCP_WINDOW_CLAMP=X and then a large
SO_RCVBUF. window_clamp stays capped at X only until the next
scaling_ratio change, when tcp_measure_rcv_mss() sets it from the larger
sk_rcvbuf.

Is the changelog statement that SO_RCVBUF no longer grows window_clamp
"above a clamp set with TCP_WINDOW_CLAMP" accurate beyond that short
window?

Now take a listener that sets TCP_WINDOW_CLAMP=X and then SO_RCVLOWAT=Y,
with Y > X and the rcvbuf not locked. This leaves window_clamp=Y and
window_clamp_user=X. tcp_req_window_clamp() then gives Y to request
sockets and SYN cookies as if it were the user's clamp:

	if (READ_ONCE(tp->window_clamp_user))
		return READ_ONCE(tp->window_clamp);

The same things happened before this patch, so this is not a
regression. Should these writers also be capped at window_clamp_user? If
not, could the new comment and the changelog be reworded to cover only
the tcp_set_rcvbuf() path?

[ ... ]

> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index c8865205cdc8b..21b4ae303dd63 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -4137,7 +4137,7 @@ static void tcp_connect_init(struct sock *sk)
>  
>  	tcp_ca_dst_init(sk, dst);
>  
> -	if (!tp->window_clamp)
> +	if (!tp->window_clamp_user)
>  		WRITE_ONCE(tp->window_clamp, dst_metric(dst, RTAX_WINDOW));
>  	tp->advmss = tcp_mss_clamp(tp, tcp_dst_advmss(dst));

[Severity: Low]
The changelog says:

  Without an RTAX_WINDOW metric and without TCP_WINDOW_CLAMP nothing
  changes: before and after this patch the advertised window and scale
  are derived from sk_rcvbuf (tcp_full_space()).

Does that hold for MPTCP subflows? mptcp_set_rcvlowat() writes the
subflow window_clamp directly and never sets window_clamp_user:

net/mptcp/sockopt.c:mptcp_set_rcvlowat() {
    ...
	mptcp_for_each_subflow(mptcp_sk(sk), subflow) {
		...
		WRITE_ONCE(ssk->sk_rcvbuf, space);
		WRITE_ONCE(tcp_sk(ssk)->window_clamp, val);
    ...
}

__mptcp_socket_create() adds msk->first to conn_list as soon as it is
created. That happens on bind(), on listen(), or on any setsockopt that
goes through __mptcp_nmpc_sk(). So SO_RCVLOWAT can reach a subflow that
is still in TCP_CLOSE, or the listener subflow. The write happens when
the rcvbuf is not locked and mptcp_space_from_win(val) > sk_rcvbuf.

Before this patch, and before b025461303d8, that non-zero value was kept
as the clamp. tcp_connect_init() checked "if (!tp->window_clamp)" and
tcp_openreq_init_rwin() used "window_clamp ? : dst_metric(...)".
tcp_select_initial_window() then derived rcv_wscale from it, and the
final window_clamp was capped at val.

After this patch, window_clamp_user is 0 on these subflows.
tcp_connect_init() overwrites window_clamp with
dst_metric(dst, RTAX_WINDOW), which is usually 0, and
tcp_req_window_clamp() ignores the listener subflow's clamp.

With default sysctls and val=1MB, the wscale in the SYN/SYN-ACK seems to
go from about 5 to about 7. The initial window_clamp is also no longer
limited to val.

The new behaviour matches plain TCP, because tcp_set_rcvlowat() only
raises a window_clamp that is already non-zero. This patch has a Fixes:
tag, and this MPTCP change goes back further than b025461303d8. Could
the changelog mention it?

[ ... ]

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1879,6 +1879,20 @@ extern void tcp_openreq_init_rwin(struct request_sock *req,
>  				  const struct sock *sk_listener,
>  				  const struct dst_entry *dst);
>  
> +/* Window clamp for a request socket of listener @tp: the listener's clamp
> + * if the user set one with TCP_WINDOW_CLAMP, else the route window metric.
> + * The SYN-ACK and the SYN cookie paths must make the same choice, or the
> + * child's window scale would not match the one sent in the SYN-ACK.
> + * The listener is not locked here.
> + */
> +static inline u32 tcp_req_window_clamp(const struct tcp_sock *tp,
> +				       const struct dst_entry *dst)
> +{
> +	if (READ_ONCE(tp->window_clamp_user))
> +		return READ_ONCE(tp->window_clamp);
> +	return dst_metric(dst, RTAX_WINDOW);
> +}
> +
>  void tcp_enter_memory_pressure(struct sock *sk);
>  void tcp_leave_memory_pressure(struct sock *sk);
>  

[Severity: Medium]
tcp_req_window_clamp() runs on the listener without the socket lock,
from tcp_openreq_init_rwin() and from cookie_v4_check() and
cookie_v6_check(). It loads window_clamp_user and then window_clamp.
Nothing orders those two loads against the two stores that
setsockopt(TCP_WINDOW_CLAMP) does on the listener.

In do_tcp_setsockopt(), tcp_set_window_clamp() stores the new
window_clamp first, and window_clamp_user is written afterwards with
WRITE_ONCE(). READ_ONCE() and WRITE_ONCE() prevent tearing. They do not
order the two fields against each other. On a weakly ordered
architecture, a SYN handled at the same time on another CPU can see the
new, non-zero window_clamp_user together with the old window_clamp.

If SO_RCVBUF was never set on the listener, the old window_clamp is 0.
tcp_req_window_clamp() then returns 0. Unless SOCK_RCVBUF_LOCK is set,
tcp_select_initial_window() treats a 0 clamp as no clamp at all, so the
request gets neither the user's clamp nor the RTAX_WINDOW metric. If
SO_RCVBUF was set earlier, the reader can instead pick up the old,
larger window_clamp derived from SO_RCVBUF in place of the user's clamp.
The SYN-ACK and the cookie ACK make this choice at different times, so
they can end up with different rcv_wscale values. Avoiding that mismatch
is the point of the new helper.

tcp_disconnect() has the same problem in reverse. It zeroes
window_clamp with a plain store and only then clears window_clamp_user.
A reader that already passed the TCP_LISTEN check can see the old
window_clamp_user and window_clamp == 0. The plain store was there
before this patch. Should it become WRITE_ONCE() now that
tcp_req_window_clamp() reads the field locklessly?

The window is narrow, because it needs a setsockopt() or a disconnect
on a live listener while SYNs arrive. Still, the changelog presents
READ_ONCE()/WRITE_ONCE() as what makes the unlocked listener path safe.

Pairing smp_store_release() on window_clamp_user with smp_load_acquire()
in tcp_req_window_clamp() would cover the setsockopt case. It would not
cover tcp_disconnect(), where the reader can still see the newer
window_clamp of 0. Would it be simpler to make the result robust to a
stale or zero window_clamp? For example, return window_clamp_user
itself, or the minimum of the two with a fallback to window_clamp_user
when window_clamp reads 0. Or is there a reason the listener's
window_clamp has to be used here rather than the stored user value?

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