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

[shashank Jain]

On Mon, Oct 12, 2026, netdev-bot+sashiko@xxxxxxxxxx wrote:
> [High] Does this check cover every case where SO_RCVBUF is applied
> before tcp_connect_init() runs?

You are right, it does not. The socket is already in TCP_SYN_SENT before
tcp_connect_init() runs, so SO_RCVBUF applied in that gap - via
TCP_FASTOPEN_CONNECT before the first write, or via bpf_setsockopt() in a
BPF_SOCK_OPS_TCP_CONNECT_CB program (as samples/bpf/tcp_clamp_kern.c and
friends do) - still writes window_clamp and still overrides the route
metric, and a state-based check cannot tell that SYN_SENT apart from a
post-init one. v3 will instead distinguish a user TCP_WINDOW_CLAMP from an
sk_rcvbuf-derived clamp, so tcp_connect_init() and tcp_openreq_init_rwin()
apply the route metric whenever the clamp was not set via
TCP_WINDOW_CLAMP; the selftest will cover the fastopen and sock_ops paths.

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

No, you are correct. With no RTAX_WINDOW metric both the old and the
patched kernel end at tcp_full_space() - the old kernel through the
"window_clamp > tcp_full_space()" branch, the patched one through
"window_clamp == 0" - so the advertised window and wscale are identical
and derived from sk_rcvbuf, not from the defaults; the patch changes
nothing there. The real impact is only (1) routes with an RTAX_WINDOW
metric and (2) a TCP_WINDOW_CLAMP that a later SO_RCVBUF on an unconnected
socket or listener overwrites. v3's changelog will say exactly that and
drop the "any application" / "without a metric" claims.

pw-bot: cr

Thanks for the careful review.
Shashank

On Mon, Oct 12, 2026 at 4:48 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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