Re: [PATCH net-next v3] tcp: honor BPF_SOCK_OPS_RWND_INIT on the active connect path

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

Pre-existing issues:
- [Medium] The `SOCK_RCVBUF_LOCK` check executes before the BPF program runs and before `full_space` is inflated, silently defeating BPF-requested windows and bypassing dynamic `SO_RCVBUF` adjustments.
--

--- Patch [1]: [PATCH net-next v3] tcp: honor BPF_SOCK_OPS_RWND_INIT on the active connect path ---
commit 2cf5d1d17ae1c7488533ecaa0fcbb83ae4591ec8
Author: Tejas Birajdar <[email protected]>

tcp: honor BPF_SOCK_OPS_RWND_INIT on the active connect path

This commit updates tcp_connect_init() to honor the initial TCP receive
window requested by a BPF_SOCK_OPS_RWND_INIT program on the active connect
path. It inflates full_space if the program returns a non-zero window,
matching the listener path's behavior, and uses u64 arithmetic to prevent
overflows.

Link: https://lore.kernel.org/netdev/[email protected]/
Link: https://lore.kernel.org/netdev/[email protected]/

> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index d7c1444b5e304..fcaa04e65189b 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c

[ ... ]

> @@ -4137,10 +4138,13 @@ static void tcp_connect_init(struct sock *sk)
>  		WRITE_ONCE(tp->window_clamp, tcp_full_space(sk));
>  
>  	rcv_wnd = tcp_rwnd_init_bpf(sk);
> +	full_space = tcp_full_space(sk);
>  	if (rcv_wnd == 0)
>  		rcv_wnd = dst_metric(dst, RTAX_INITRWND);
> +	else if (full_space < (u64)rcv_wnd * tp->advmss)
> +		full_space = min_t(u64, (u64)rcv_wnd * tp->advmss, INT_MAX);

[Severity: Medium]
This is a pre-existing issue, but does the SOCK_RCVBUF_LOCK check happening
before the BPF program runs defeat the purpose of allowing BPF to adjust
the receive window?

If an application sets SO_RCVBUF prior to connect(), SOCK_RCVBUF_LOCK is
set. In tcp_connect_init(), this caps tp->window_clamp to the old
tcp_full_space() before tcp_rwnd_init_bpf() is invoked.

When the BPF program returns a large requested window and full_space is
inflated, tcp_select_initial_window() bounds space by
min(window_clamp, space).

This caps it right back to the pre-BPF full_space and discards the BPF
request.

Conversely, if the application did not set SO_RCVBUF but the BPF program
adjusts it dynamically (which sets SOCK_RCVBUF_LOCK), the enforcement
check has already passed. This means window_clamp may remain 0 and bypass
the lock's constraints, resulting in an unconstrained window.

The same behavior appears to occur in the listener path in
tcp_openreq_init_rwin() in net/ipv4/tcp_minisocks.c, where
req->rsk_window_clamp is capped to the pre-BPF full_space before the BPF
program executes.

Should the SOCK_RCVBUF_LOCK checks be moved after tcp_rwnd_init_bpf() so
that dynamically adjusted SO_RCVBUF values are properly accounted for?

> -	tcp_select_initial_window(sk, tcp_full_space(sk),
> +	tcp_select_initial_window(sk, full_space,
>  				  tp->advmss - (tp->rx_opt.ts_recent_stamp ? tp->tcp_header_len - sizeof(struct tcphdr) : 0),
>  				  &tp->rcv_wnd,
>  				  &tp->window_clamp,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1