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

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]

Message-ID: <ba8f3ef7-a696-0e47-eadb-7772e6aba725@kernel.dk>
Date:   Fri, 14 Feb 2020 18:20:53 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Linus Torvalds <torvalds@...ux-foundation.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-rc2

On 2/14/20 3:07 PM, Linus Torvalds wrote:
> On Fri, Feb 14, 2020 at 8:45 AM Jens Axboe <axboe@...nel.dk> wrote:
>>
>> Here's a set of fixes for io_uring that should go into this release.
> 
> Whaa?
> 
>           for_each_node(node) {
> +                if (!node_online(node))
> +                        continue;
> 
> that's just silly.
> 
> We have 'for_each_online_node()' for this.
> 
> There's something like four patterns of that pointless thing.

Sorry, that definitely should have been for_each_online_node() for
those, guess I didn't think of that when making the change.

> And in io_wq_create(), do you really want to allocate that wqe for
> nodes that aren't online? Right now you _allocate_ the node data for
> them (using a non-node-specific allocation), but then you won't
> actually create the thread for them io_wq_manager().

I was thinking about this a bit, and as far as I know there's no good
way to get notified of nodes coming and going. And I'd really like
to avoid having to add that to the fast path.

So this seemed like the lesser of evils, we setup the wqe just in
case the node does come online, and then rely on the manager
creating the thread when we need it. Not sure what setup was run
to create it, I haven't come across any boxes where we have nodes
that are present but not online.

> Plus if the node online status changes, it looks like you'll mess up
> _anyway_, in that  io_wq_manager() will first create the workers on
> one set of nodes, but then perhaps set the state flags for a
> completely different set of nodes if some onlining/offlining has
> happened.

We'll look into making this more clear and bullet proof.

> I've pulled this, but Jens, you need to be more careful. This all
> looks like completely random state that nobody spent any time thinking
> about.
> 
> Seriously, this "io_uring FIXES ONLY" needs to be stricter than what
> you seem to be doing here. This "fix" is opening up a lot of new
> possibilities for inconsistencies in the data structures.

We'll get it sorted for 5.6. Thanks for pulling.

-- 
Jens Axboe