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 20:27:14 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Eric Dumazet <edumazet@...gle.com>
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 4/12/22 8:19 PM, Eric Dumazet wrote:
> On Tue, Apr 12, 2022 at 7:12 PM Jens Axboe <axboe@...nel.dk> wrote:
>>
>> On 4/12/22 8:05 PM, Eric Dumazet wrote:
>>> On Tue, Apr 12, 2022 at 7:01 PM Jens Axboe <axboe@...nel.dk> wrote:
>>>>
>>>> On 4/12/22 7:54 PM, Eric Dumazet wrote:
>>>>> 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
>>>>
>>>> But how is that currently protected?
>>>
>>> It is protected by current code.
>>>
>>> What you wrote would break TCP stack quite badly.
>>
>> No offense, but your explanations are severely lacking. By "current
>> code"? So what you're saying is that it's protected by how the code
>> currently works? From how that it currently is? Yeah, that surely
>> explains it.
>>
>>> I suggest you setup/run a syzbot server/farm, then you will have a
>>> hundred reports quite easily.
>>
>> Nowhere am I claiming this is currently perfect, and it should have had
>> an RFC on it. Was hoping for some constructive criticism on how to move
>> this forward, as high frequency TCP currently _sucks_ in the stack.
>> Instead I get useless replies, not very encouraging.
>>
>> I've run this quite extensively on just basic send/receive over sockets,
>> so it's not like it hasn't been run at all. And it's been fine so far,
>> no ill effects observed. If we need to tighten down the locking, perhaps
>> a valid use would be to simply skip the mutex and retain the bh lock for
>> setting owner. As far as I can tell, should still be safe to skip on
>> release, except if we need to process the backlog. And it'd serialize
>> the owner setting with the BH, which seems to be your main objection in.
>> Mostly guessing here, based on the in-depth replies.
>>
>> But it'd be nice if we could have a more constructive dialogue about
>> this, rather than the weird dismisiveness.
>>
>>
> 
> Sure. It would be nice that I have not received such a patch series
> the day I am sick.

I'm sorry that you are sick - but if you are not in a state to reply,
then please just don't. It sets a bad example. It was sent to the list,
not to you personally.

Don't check email then, putting the blame on ME for posting a patchset
while you are sick is uncalled for and rude. If I had a crystal ball, I
would not be spending my time working on the kernel. You know what
would've been a better idea? Replying that you are sick and that you are
sorry for being an ass on the mailing list.

> Jakub, David, Paolo, please provide details to Jens, thanks.

There's no rush here fwiw - I'm heading out on PTO rest of the week,
so we can pick this back up when I get back. I'll check in on emails,
but activity will be sparse.

-- 
Jens Axboe