netdev - Re: [PATCH V2 net-next 1/2] tcp: send in-queue bytes in cmsg upon read

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]

Date:   Mon, 30 Apr 2018 12:59:44 -0400
From:   Soheil Hassas Yeganeh <soheil@...gle.com>
To:     David Miller <davem@...emloft.net>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        netdev <netdev@...r.kernel.org>,
        Yuchung Cheng <ycheng@...gle.com>,
        Neal Cardwell <ncardwell@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH V2 net-next 1/2] tcp: send in-queue bytes in cmsg upon read

On Mon, Apr 30, 2018 at 12:10 PM, David Miller <davem@...emloft.net> wrote:
> From: Eric Dumazet <eric.dumazet@...il.com>
> Date: Mon, 30 Apr 2018 09:01:47 -0700
>
>> TCP sockets are read by a single thread really (or synchronized
>> threads), or garbage is ensured, regardless of how the kernel
>> ensures locking while reporting "queue length"
>
> Whatever applications "typically do", we should never return
> garbage, and that is what this code allowing to happen.
>
> Everything else in recvmsg() operates on state under the proper socket
> lock, to ensure consistency.
>
> The only reason we are releasing the socket lock first it to make sure
> the backlog is processed and we have the most update information
> available.
>
> It seems like one is striving for correctness and better accuracy, no?
> :-)
>
> Look, this can be fixed really simply.  And if you are worried about
> unbounded loops if two apps maliciously do recvmsg() in parallel,
> then don't even loop, just fallback to full socket locking and make
> the "non-typical" application pay the price:
>
>         tmp1 = A;
>         tmp2 = B;
>         barrier();
>         tmp3 = A;
>         if (unlikely(tmp1 != tmp3)) {
>                 lock_sock(sk);
>                 tmp1 = A;
>                 tmp2 = B;
>                 release_sock(sk);
>         }
>
> I'm seriously not applying the patch as-is, sorry.  This issue
> must be addressed somehow.

Thank you David for the suggestion. Sure, I'll send a V3 with what you
suggested above.

Thanks,
Soheil