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

[Dave Chinner <david@fromorbit.com>]

On Fri, May 11, 2018 at 01:39:01AM +0100, Al Viro wrote:
> On Fri, May 11, 2018 at 08:56:07AM +1000, Dave Chinner wrote:
> 
> > > 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?
> 
> Yes.  d_splice_alias()
> 	* will do the right thing when it runs into directory inode
> that already has an alias
> 	* is called from ->d_lookup(), which has calling conventions
> allowing to return a preexisting alias
> 
> The race in question is between mkdir() and open-by-fhandle that manages
> to guess an fhandle for directory about to be created.  mkdir() side
> creates a new inode, inserts it into icache (locked) and proceeds towards
> unlock_new_inode()/d_instantiate().  Suppose it loses CPU right after
> unlock_new_inode() and open-by-fhandle picks the inode from icache
> (either having just gotten there, or finally gets woken up after having
> waited for the sucker to get unlocked).  inode is valid, everything's
> set up properply, so we pass it to d_obtain_alias(), which sees that
> there's no exiting dentries, allocates one, rechecks, finds that there's
> still nothing and proceeds to attach its new anon dentry to that inode.
> Now mkdir regains CPU and does d_instantiate().  And we are fucked -
> there are *two* dentries for given directory inode.

Ok, thanks for the description of the race, Al. I understand it now.
:)

> 
> The window is narrow - to have a chance to hit it you need either
> to run it in a VM or have security_d_instantiate() (from d_instantiate())
> to do something slow (ideally - blocking).  It's non-empty, though.
> 
> Doing it in the opposite order (as XFS does on mkdir et.al.) plugs that
> window - open-by-fhandle won't get to the inode until after mkdir has
> attached a dentry to it.  Then d_obtain_alias() will simply return that
> dentry and we are done.  It's only d_instantiate() (or d_add()) that is
> a problem - d_splice_alias() is fine, so on the lookup path we don't
> need anything like that.  d_add_ci() is like d_splice_alias() in that
> respect, so the lookup is OK in case-insensitive variant as well.
> 
> So it would appear that XFS doesn't need to be touched.  HOWEVER,
> lockdep shite *can't* be done after something has had a chance to grab
> the damn rwsem.  I really wonder if
> 		d_instantiate(dentry, inode);
>         xfs_finish_inode_setup(cip);
> doesn't lead to unpleasantness with lockdep enabled:
> 	xfs_finish_inode_setup() -> unlock_new_inode() ->
> lockdep_annotate_inode_mutex_key() -> init_rwsem(&inode->i_rwsem)
> which does wonders if something has already gotten to the inode
> via that dentry and tried e.g. lock_inode() on it.

Could well do. Though it seems fixable.

i.e. we already have code in xfs_setup_inode() that sets the xfs
inode ILOCK rwsem dir/non-dir lockdep class before the new inode is
unlocked - we could just do the i_rwsem lockdep setup there, too.
Then, if we were to factor unlock_new_inode() as Andreas suggested,
we could call __unlock_new_inode() from xfs_finish_inode_setup().

I might be missing something subtle, but that looks to me like it
would work.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com