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:	Fri, 17 Jun 2016 05:29:14 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Oleg Drokin <green@...uxhacker.ru>
Cc:	Trond Myklebust <trond.myklebust@...marydata.com>,
	linux-nfs@...r.kernel.org,
	Mailing List <linux-kernel@...r.kernel.org>,
	"<linux-fsdevel@...r.kernel.org>" <linux-fsdevel@...r.kernel.org>,
	idryomov@...il.com, sage@...hat.com, zyan@...hat.com
Subject: Re: More parallel atomic_open/d_splice_alias fun with NFS and
 possibly more FSes.

On Fri, Jun 17, 2016 at 12:09:19AM -0400, Oleg Drokin wrote:

>     So they both do d_drop(), the dentry is now unhashed, and they both
>     dive into nfs_lookup().
>     There eventually they both call
> 
> 	res = d_splice_alias(inode, dentry);
> 
>    And so the first lucky one continues on it's merry way with a hashed dentry,
>    but the other less lucky one ends up calling into d_splice_alias() with
>    dentry that's already hashed and hits the very familiar assertion.
> 
>    I took a brief look into ceph and it looks like a very similar thing
>    might happen there with handle_reply() for two parallel replies calling into
>    ceph_fill_trace() and then splice_alias()->d_splice_alias(), since the
>    unhashed check it does is not under any locks, it's unsafe, so the problem
>    might be more generic than just NFS too.
> 
>    So I wonder how to best fix this? Holding some sort of dentry lock across a call
>    into atomic_open in VFS? We cannot just make d_splice_alias() callers call with
>    inode->i_lock held because dentry might be negative.

Oh, lovely...  So basically the problem is that we violate the "no lookups on
the same name in parallel" rule on those fallbacks from foo_atomic_open() to
foo_lookup().  The thing is, a lot of ->atomic_open() instances have such
fallbacks and I wonder if that's a sign that we need to lift some of that
to fs/namei.c...

Hell knows; alternative is to have that d_drop() followed by d_alloc_parallel()
and feeding that dentry to lookup.  I'll play with that a bit and see what's
better; hopefully I'll have something by tomorrow.