Re: [PATCH 01/10] VFS: support parallel updates in the one directory.

From: Al Viro <viro@zeniv.linux.org.uk>
To: NeilBrown <neilb@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Daire Byrne <daire@dneg.com>,
	Trond Myklebust <trond.myklebust@hammerspace.com>,
	Chuck Lever <chuck.lever@oracle.com>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
	linux-fsdevel@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/10] VFS: support parallel updates in the one directory.
Date: Thu, 1 Sep 2022 04:44:36 +0100	[thread overview]
Message-ID: <YxAqpGgNi4JTxkbT@ZenIV> (raw)
In-Reply-To: <166199227016.17668.15373771428363682061@noble.neil.brown.name>

On Thu, Sep 01, 2022 at 10:31:10AM +1000, NeilBrown wrote:

> Thanks for this list.

Keep in mind that this list is just off the top of my head - it's
nowhere near complete.

> d_splice_alias() happens at ->lookup time so it is already under a
> shared lock.  I don't see that it depends on i_rwsem - it uses i_lock
> for the important locking.

Nope.

Process 1:
rmdir foo/bar
	foo found and locked exclusive [*]
	dentry of bar found
	->rmdir() instance called
Process 2:
stat foo/splat
	foo found and locked shared [*]
	dentry of splat does not exist anywhere
	dentry allocated, marked in-lookup
	->lookup() instance called
	inode found and passed to d_splice_alias() ...
	... which finds that it's a directory inode ...
	... and foo/bar refers to it.  E.g. it's on NFS and another
client has just done mv bar splat
	__d_unalias() is called, to try and move existing alias (foo/bar)
into the right place.  It sees that no change of parent is involved,
	so it can just proceed to __d_move().
Process 1:
	forms an rmdir request to server, using ->d_name (and possibly
->d_parent) of dentry of foo/bar.  It knows that ->d_name is stable,
since the caller holds foo locked exclusive and all callers of __d_move()
hold the old parent at least shared.

In mainline process 2 will block (or, in case if it deals with different
parent, try to grab the old parent of the existing alias shared and fail
and with -ESTALE).  With your changes process 1 will be holding
foo/ locked shared, so process 2 will succeed and proceed to __d_move(),
right under the nose of process 1 accessing ->d_name.  If the names involved
had been longer than 32 characters, it would risk accessing kfreed memory.
Or fetching the length from old name and pointer from new one, walking
past the end of kmalloc'ed object, etc.

Sure, assuming that we are talking about NFS, server would have probably
failed the RMDIR request - if you managed to form that request without
oopsing, that is.