Re: [RFC][PATCH] do d_instantiate/unlock_new_inode combinations safely

[Dave Chinner <david@fromorbit.com>]

On Thu, May 10, 2018 at 07:20:58PM +0100, Al Viro wrote:
> [in the spirit of "don't put 'em in without posting for review; the
> this is present in vfs.git#for-linus, if you prefer to look in git.
> 
> Background: a bunch of nfsd races fixes from back in 2008 had
> problems with lockdep enabled; in 2012 that got "fixed", unfortunately
> reopening a narrow race window.  The patch below does *NOT* fix
> all filesystems, but it does fix most of the exported local ones
> and it is easy to backport, so it makes for a sane starting point.

Do you have a pointer to the commits/test case for this? XFS has a
fairly significant separation between unlock_new_inode() and dentry
cache instantiation for some paths....

> If anyone has objections, this is your chance to yell.
> ]
> 
> For anything NFS-exported we do _not_ want to unlock new inode
> before it has grown an alias; original set of fixes got the
> ordering right, but missed the nasty complication in case of
> lockdep being enabled - unlock_new_inode() does
>     lockdep_annotate_inode_mutex_key(inode)
> which can only be done before anyone gets a chance to touch
> ->i_mutex.  Unfortunately, flipping the order and doing
> unlock_new_inode() before d_instantiate() opens a window when
> mkdir can race with open-by-fhandle on a guessed fhandle, leading
> to multiple aliases for a directory inode and all the breakage
> that follows from that.
> 
>     Correct solution: a new primitive (d_instantiate_new())
> combining these two in the right order - lockdep annotate, then
> d_instantiate(), then the rest of unlock_new_inode().  All
> combinations of d_instantiate() with unlock_new_inode() should
> be converted to that.

Ok, so this seems to touch only the paths that create new inodes
(mkdir, mknod, etc). Is the lookup path that does:


	unlock_new_inode()
	.....
	d_splice_alias(inode, dentry);

OK?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com