netdev - Re: [PATCH net] tcp: Adjust clamping window for applications specifying SO

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] [day] [month] [year] [list]

Message-ID: <bba8b50d-d116-42c4-adde-a8045df36961@quicinc.com>
Date: Fri, 26 Jul 2024 14:43:53 -0600
From: "Subash Abhinov Kasiviswanathan (KS)" <quic_subashab@...cinc.com>
To: Eric Dumazet <edumazet@...gle.com>
CC: <soheil@...gle.com>, <ncardwell@...gle.com>, <yyd@...gle.com>,
        <ycheng@...gle.com>, <davem@...emloft.net>, <kuba@...nel.org>,
        <netdev@...r.kernel.org>, Sean Tranchetti <quic_stranche@...cinc.com>
Subject: Re: [PATCH net] tcp: Adjust clamping window for applications
 specifying SO_RCVBUF


On 7/26/2024 3:40 AM, Eric Dumazet wrote:
> On Thu, Jul 25, 2024 at 11:55 PM Subash Abhinov Kasiviswanathan
> <quic_subashab@...cinc.com> wrote:
>>
>>           */
>>
>> -       if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf) &&
>> -           !(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
>> +       if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf)) {
>>                  u64 rcvwin, grow;
>>                  int rcvbuf;
>>
>> @@ -771,12 +770,24 @@ void tcp_rcv_space_adjust(struct sock *sk)
>>
>>                  rcvbuf = min_t(u64, tcp_space_from_win(sk, rcvwin),
>>                                 READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_rmem[2]));
>> -               if (rcvbuf > sk->sk_rcvbuf) {
>> -                       WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
>> +               if (!(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
>> +                       if (rcvbuf > sk->sk_rcvbuf) {
>> +                               WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
>>
>> -                       /* Make the window clamp follow along.  */
>> -                       WRITE_ONCE(tp->window_clamp,
>> -                                  tcp_win_from_space(sk, rcvbuf));
>> +                               /* Make the window clamp follow along.  */
>> +                               WRITE_ONCE(tp->window_clamp,
>> +                                          tcp_win_from_space(sk, rcvbuf));
>> +                       }
>> +               } else {
>> +                       /* Make the window clamp follow along while being bounded
>> +                        * by SO_RCVBUF.
>> +                        */
>> +                       if (rcvbuf <= sk->sk_rcvbuf) {
> 
> I do not really understand this part.
> I am guessing this test will often be false and your problem won't be fixed.
> You do not handle all  sysctl_tcp_adv_win_scale values (positive and negative)
> 
> I would instead not use "if (rcvbuf <= sk->sk_rcvbuf) {"
> 
> and instead :
> 
> else {
>        int clamp = tcp_win_from_space(sk, min(rcvbuf, sk->sk_rcvbuf));
> 
>        if (clamp > tp->window_clamp)
>              WRITE_ONCE(tp->window_clamp, clamp);
> }
Thanks Eric, I've updated this in v2.