netdev - Re: [PATCH net] tcp: Update window clamping condition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <40795735-028e-4838-8275-958407f1305d@redhat.com>
Date: Tue, 13 Aug 2024 11:26:55 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Subash Abhinov Kasiviswanathan <quic_subashab@...cinc.com>,
 edumazet@...gle.com, soheil@...gle.com, ncardwell@...gle.com,
 yyd@...gle.com, ycheng@...gle.com, davem@...emloft.net, kuba@...nel.org,
 netdev@...r.kernel.org, dsahern@...nel.org
Cc: Sean Tranchetti <quic_stranche@...cinc.com>
Subject: Re: [PATCH net] tcp: Update window clamping condition

On 8/9/24 01:06, Subash Abhinov Kasiviswanathan wrote:
> This patch is based on the discussions between Neal Cardwell and
> Eric Dumazet in the link
> https://lore.kernel.org/netdev/20240726204105.1466841-1-quic_subashab@quicinc.com/
> 
> It was correctly pointed out that tp->window_clamp would not be
> updated in cases where net.ipv4.tcp_moderate_rcvbuf=0 or if
> (copied <= tp->rcvq_space.space). While it is expected for most
> setups to leave the sysctl enabled, the latter condition may
> not end up hitting depending on the TCP receive queue size and
> the pattern of arriving data.
> 
> The updated check should be hit only on initial MSS update from
> TCP_MIN_MSS to measured MSS value and subsequently if there was
> an update to a larger value.
> 
> Fixes: 05f76b2d634e ("tcp: Adjust clamping window for applications specifying SO_RCVBUF")
> Signed-off-by: Sean Tranchetti <quic_stranche@...cinc.com>
> Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@...cinc.com>
> ---
>   net/ipv4/tcp_input.c | 28 ++++++++++++----------------
>   1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index e2b9583ed96a..e37488d3453f 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -238,9 +238,14 @@ static void tcp_measure_rcv_mss(struct sock *sk, const struct sk_buff *skb)
>   		 */
>   		if (unlikely(len != icsk->icsk_ack.rcv_mss)) {
>   			u64 val = (u64)skb->len << TCP_RMEM_TO_WIN_SCALE;
> +			u8 old_ratio = tcp_sk(sk)->scaling_ratio;
>   
>   			do_div(val, skb->truesize);
>   			tcp_sk(sk)->scaling_ratio = val ? val : 1;
> +
> +			if (old_ratio != tcp_sk(sk)->scaling_ratio)

Should we do this only for sk->sk_userlocks & SOCK_RCVBUF_LOCK ?

I think that explicitly checking for an ratio increase would be safer: 
even if len increased I guess the ratio could decrease in some edge 
scenarios.

Thanks!

Paolo