linux-kernel - Re: [GIT PULL] io_uring fixes for 5.6-rc

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: <cb853c40-820a-b05e-1a1b-50770565e69c@fb.com>
Date:   Fri, 13 Mar 2020 15:33:09 -0600
From:   Jens Axboe <axboe@...com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        "Paul E. McKenney" <paulmck@...nel.org>, Tejun Heo <tj@...nel.org>
CC:     io-uring <io-uring@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] io_uring fixes for 5.6-rc

On 3/13/20 2:18 PM, Linus Torvalds wrote:
> On Fri, Mar 13, 2020 at 10:50 AM Jens Axboe <axboe@...com> wrote:
>>
>> Just a single fix here, improving the RCU callback ordering from last
>> week. After a bit more perusing by Paul, he poked a hole in the
>> original.
> 
> Ouch.
> 
> If I read this patch correctly, you're now adding a rcu_barrier() onto
> the system workqueue for each io_uring context freeing op.

It's actually not quite that bad, it's for every context that's used
registered file. That will generally be long term use cases, like server
backend kind of stuff, not for short lived or "normal" use cases.

> This makes me worry:
> 
>  - I think system_wq is unordered, so does it even guarantee that the
> rcu_barrier happens after whatever work you're expecting it to be
> after?

The ordering is wrt an rcu callback that's already queued. So we don't
care about ordering of other work at all, we just care about issuing
that rcu_barrier() before we exit + free, so we know that the existing
(if any) rcu callback has run.

> Or is it using a workqueue not because it wants to serialize with any
> other work, but because it needs to use rcu_barrier in a context where
> it can't sleep?

Really just using a workqueue because we already have one for this
particular item, and that takes the latency of needing the rcu barrier
out of the fast path for the application.

> But the commit message does seem to imply that ordering is important..

Only for a previous rcu callback, not for work items!

>  - doesn't this have the potential to flood the system_wq be full of
> flushing things that all could take a while..
> 
> I've pulled it, and it may all be correct, just chalk this message up
> to "Linus got nervous looking at it".

All good, always appreciate extra eyes on it! We could do the
rcu_barrier() inline and just take the hit there, and there's also room
to be a bit smarter and only do the barrier if we know we have to. But
since this is 5.6 material, I didn't want to complicate things further.

-- 
Jens Axboe