netdev - Re: [PATCHSET 0/4] Add support for no-lock sockets

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]

Date:   Tue, 12 Apr 2022 18:54:41 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Jens Axboe <axboe@...nel.dk>
Cc:     Eric Dumazet <eric.dumazet@...il.com>, io-uring@...r.kernel.org,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCHSET 0/4] Add support for no-lock sockets

On Tue, Apr 12, 2022 at 6:26 PM Jens Axboe <axboe@...nel.dk> wrote:
>
> On 4/12/22 6:40 PM, Eric Dumazet wrote:
> >
> > On 4/12/22 13:26, Jens Axboe wrote:
> >> Hi,
> >>
> >> If we accept a connection directly, eg without installing a file
> >> descriptor for it, or if we use IORING_OP_SOCKET in direct mode, then
> >> we have a socket for recv/send that we can fully serialize access to.
> >>
> >> With that in mind, we can feasibly skip locking on the socket for TCP
> >> in that case. Some of the testing I've done has shown as much as 15%
> >> of overhead in the lock_sock/release_sock part, with this change then
> >> we see none.
> >>
> >> Comments welcome!
> >>
> > How BH handlers (including TCP timers) and io_uring are going to run
> > safely ? Even if a tcp socket had one user, (private fd opened by a
> > non multi-threaded program), we would still to use the spinlock.
>
> But we don't even hold the spinlock over lock_sock() and release_sock(),
> just the mutex. And we do check for running eg the backlog on release,
> which I believe is done safely and similarly in other places too.

So lets say TCP stack receives a packet in BH handler... it proceeds
using many tcp sock fields.

Then io_uring wants to read/write stuff from another cpu, while BH
handler(s) is(are) not done yet,
and will happily read/change many of the same fields

Writing a 1 and a 0 in a bit field to ensure mutual exclusion is not
going to work,
even with the smp_rmb() and smp_wmb() you added (adding more costs for
non io_uring users
which already pay a high lock tax)

If we want to optimize the lock_sock()/release_sock() for common cases
(a single user thread per TCP socket),
then maybe we can play games with some kind of cmpxchg() games, but
that would be a generic change.