linux-kernel - Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

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, 5 Jul 2016 15:12:44 -0400
From:	Oleg Drokin <green@...uxhacker.ru>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	Mailing List <linux-kernel@...r.kernel.org>,
	"<linux-fsdevel@...r.kernel.org>" <linux-fsdevel@...r.kernel.org>
Subject: Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

On Jul 5, 2016, at 2:08 PM, Al Viro wrote:

> On Tue, Jul 05, 2016 at 12:33:09PM -0400, Oleg Drokin wrote:
> 
>> This also makes me question the whole thing some more. We are definitely in lookup
>> when this hits, so the dentry is already new, yet it does not check off as
>> d_in_lookup(). That also means that by skipping the ll_splice_alias we are failing
>> to hash it and that causing needless lookups later?
>> Looking some back into the history of commits, d_in_lookup() is to tell us
>> that we are in the middle of lookup. How can we be in the middle of lookup
>> path then and not have this set on a dentry? We know dentry was not
>> substituted with anything here because we did not call into ll_split_alias().
>> So what's going on then?
> 
> Lookup in directory locked exclusive, that's what...  In unlink(), in your
> testcase.  And yes, this piece of 1/3 is incorrect; what I do not understand
> is the logics of what you are doing with dcache in ll_splice_alias() and
> in its caller ;-/

When d_in_lookup was introduced, it was described as:
    New primitives: d_in_lookup() (a predicate checking if dentry is in
    the in-lookup state) and d_lookup_done() (tells the system that
    we are done with lookup and if it's still marked as in-lookup, it
    should cease to be such).

I don't see where it mentions anything about exclusive vs parallel lookup
that probably led to some confusion.

So with Lustre the dentry can be in three states, really:

1. hashed dentry that's all A-ok to reuse.
2. hashed dentry that's NOT valid (dlm lock went away) - this is distinguished in d_compare by looking at a bit in the fs_data
3. unhashed dentry ( I guess could be both valid and invalid lustre-wise).

So the logic in ll_lookup_it_finish() (part of regular lookup) is this:

If the dentry we have is not hashed - this is a new lookup, so we need to
call into ll_splice_alias() to see if there's a better dentry we need to
reuse that was already rejected by VFS before since we did not have necessary locks,
but we do have them now.
The comment at the top of ll_dcompare() explains why we don't just unhash the
dentry on lock-loss - that apparently leads to a loop around real_lookup for
real-contended dentries.
This is also why we cannot use d_splice_alias here - such cases are possible
for regular files and directories.

Anyway, I guess additional point of confusion here is then why does
ll_lookup_it_finish() need to check for hashedness of the dentry since it's in
lookup, so we should be unhashed here.
I checked the commit history and this check was added along with atomic_open
support, so I imagine we can just move it up into ll_atomic_open and then your
change starts to make sense along with a few other things.