Re: [PATCH 3/3] VFS: close race between getcwd() and d_move()

From: Linus Torvalds <torvalds@linux-foundation.org>
To: NeilBrown <neilb@suse.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/3] VFS: close race between getcwd() and d_move()
Date: Thu, 9 Nov 2017 17:40:52 -0800	[thread overview]
Message-ID: <CA+55aFwfH6-V-JGacPhEQRJg7GhKwMno-DY4UKSXLP9gfAeK_w@mail.gmail.com> (raw)
In-Reply-To: <8760ajf6al.fsf@notabene.neil.brown.name>

On Thu, Nov 9, 2017 at 2:14 PM, NeilBrown <neilb@suse.com> wrote:
> On Thu, Nov 09 2017, Linus Torvalds wrote:
>>
>> How nasty would it be to just expand the calls to __d_drop/__d_rehash
>> into __d_move itself, and take both has list locks at the same time
>> (with the usual ordering and checking if it's the same list, of
>> course).
>
> something like this?

Yes.

This looks nicer to me. Partly because I hate those "pass flags to
functions that modify their behavior" kinds of patches. I'd rather see
just straight-line unconditional code with some possible duplication.

That said, your conversion isn't the obvious "no semantic changes"
one. The BUG_ON() conditions you added are a bit odd.  You've taken
the BUG_ON() from __d_rehash() that no longe rmakes any sense (because
we just explicitly unhashed it), and replaced it with a BUG_ON() that
didn't exist before, and is also not the conditionm that __d_drop
actually had (or the condition that means that the hash liost might be
different - ie the whole IS_ROOT() case).

So the patch looks conceptually good., but I worry about the details.
They may be right, but that odd IS_ROOT case in __d_drop really
worries me.  Can we rename one of those disconnected entries?

Of course, that then ties into the other thread, where those
disconnected entries are a bit too special anyway.

I also do wonder if we can avoid all the unhash/rehash games entirely
(and avoid the hash list locking) if it turns out that the dentry and
target hash lists are the same.

I'm not claiming that as an optimization (it's an unusual case), I'm
more thinking that it might fall out fairly naturally from the "lock
both" case, since that one needs to check for the same list anyway.

> Do you like it enough for me to make it into a real patch?

Let's see if Al hates this approach, but I definitely prefer it
assuming my worries are groundless.

> I'd probably move hlist_bl_lock_two() into list_bl.h.

Maybe not - see the above issue where maybe the "same hash" might
actually merit different codepath.

            Linus