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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 6 Jul 2016 00:35:30 -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 11:25 PM, Oleg Drokin wrote:

> 
> On Jul 5, 2016, at 11:20 PM, Al Viro wrote:
> 
>> On Tue, Jul 05, 2016 at 08:29:37PM -0400, Oleg Drokin wrote:
>>>> +		/* Otherwise we just unhash it to be rehashed afresh via
>>>> +		 * lookup if necessary
>>>> +		 */
>>>> +		d_drop(dentry);
>>> 
>>> So we can even drop this part and retain the top condition as it was.
>>> d_add does not care if the dentry we are feeding it was hashed or not,
>>> so do you see any downsides to doing that I wonder?
>> 
>> d_add() on hashed dentry will end up reaching this:
>> static void __d_rehash(struct dentry * entry, struct hlist_bl_head *b)
>> {
>>       BUG_ON(!d_unhashed(entry));
> 
> Ah, ok. Yes, I remember about it now from the older issue with nfs.
> 
> It's still puzzling why I did not hit it yet, but oh well.
> 
> I wonder if handling of negative dentries broke… Time for more investigations.

Ah, the reason was just looking me in the eyes as the first thing we do in
ll_revalidate_dentry():

        if ((lookup_flags & (LOOKUP_OPEN | LOOKUP_CREATE)) ==
            (LOOKUP_OPEN | LOOKUP_CREATE))
                return 0;

Apparently we are trying to protect from the case where dentry is valid when we
check it, but gets pulled under us as soon as we are done (no lustre locking held
around it).
So if we don't do this, we fall into ll_file_open/ll_intent_file_open and form
a request to open the file based on the inode. If this inode is already gone
server-side, we might get ESTALE, since we don't really send the name to the
server in this case so it does not know what is it we are after anyway.

On the userspace side of things the picture is not pretty then, we are doing
open(O_CREAT) and get ESTALE back.

Looking at do_filp_open(), there's actually a retry:
        if (unlikely(filp == ERR_PTR(-ESTALE)))
                filp = path_openat(&nd, op, flags | LOOKUP_REVAL);

But it's only once so for a contended file we can still have some other thread
do a lookup, provide us with a new cached dentry that's similarly pulled
under us next time around.
But I guess the whole idea of this is to let us know the file is contended and
we can just only fail revalidate then.

Hm.. looking at the server - it's even worse, we'd get ENOENT from the server if
the desired inode no longer exist, so a perfect opportunity for the
client to turn that ENOENT into ESTALE if the create flag was set.