xfs
[Top] [All Lists]

Re: [PATCH] xfs: inodes are new until the dentry cache is set up

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: inodes are new until the dentry cache is set up
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Thu, 8 Jan 2015 19:25:33 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150108232328.GM25000@dastard>
References: <1420667576-7592-1-git-send-email-david@xxxxxxxxxxxxx> <20150108153205.GE33789@xxxxxxxxxxxxxxx> <20150108232328.GM25000@dastard>
User-agent: Mutt/1.5.23 (2014-03-12)
On Fri, Jan 09, 2015 at 10:23:28AM +1100, Dave Chinner wrote:
> On Thu, Jan 08, 2015 at 10:32:05AM -0500, Brian Foster wrote:
> > On Thu, Jan 08, 2015 at 08:52:56AM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > > 
> > > Al Viro noticed a generic set of issues to do with filehandle lookup
> > > racing with dentry cache setup. They involve a filehandle lookup
> > > occurring while an inode is being created and the filehandle lookup
> > > racing with the dentry creation for the real file. This can lead to
> > > multiple dentries for the one path being instantiated. There are a
> > > host of other issues around this same set of paths.
> > > 
> > > The underlying cause is that file handle lookup only waits on inode
> > > cache instantiation rather than full dentry cache instantiation. XFS
> > > is mostly immune to the problems discovered due to it's own internal
> > > inode cache, but there are a couple of corner cases where races can
> > > happen.
> > > 
> > > We currently clear the XFS_INEW flag when the inode is fully set up
> > > after insertion into the cache. Newly allocated inodes are inserted
> > > locked and so aren't usable until the allocation transaction
> > > commits. This, however, occurs before the dentry and security
> > > information is fully initialised and hence the inode is unlocked and
> > > available for lookups to find too early.
> > > 
> > > To solve the problem, only clear the XFS_INEW flag for newly created
> > > inodes once the dentry is fully instantiated. This means lookups
> > > will retry until the XFS_INEW flag is removed from the inode and
> > > hence avoids the race conditions in questions.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ....
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index ce80eeb..8be5bb5 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -186,6 +186,8 @@ xfs_generic_create(
> > >   else
> > >           d_instantiate(dentry, inode);
> > >  
> > > + xfs_finish_inode_setup(ip);
> > > +
> > >   out_free_acl:
> > >   if (default_acl)
> > >           posix_acl_release(default_acl);
> > > @@ -194,6 +196,7 @@ xfs_generic_create(
> > >   return error;
> > >  
> > >   out_cleanup_inode:
> > > + xfs_finish_inode_setup(ip);
> > >   if (!tmpfile)
> > >           xfs_cleanup_inode(dir, inode, dentry);
> > >   iput(inode);
> > > @@ -366,9 +369,11 @@ xfs_vn_symlink(
> > >           goto out_cleanup_inode;
> > >  
> > >   d_instantiate(dentry, inode);
> > > + xfs_finish_inode_setup(cip);
> > >   return 0;
> > >  
> > >   out_cleanup_inode:
> > > + xfs_finish_inode_setup(cip);
> > >   xfs_cleanup_inode(dir, inode, dentry);
> > >   iput(inode);
> > >   out:
> > 
> > Ok, but what about post-inode-allocation failure conditions down in
> > xfs_create()? I don't know if there's any real harm in releasing an
> > I_NEW inode, but iput_final() does throw a warning. Same general
> > question applies to xfs_create_tmpfile(), etc..
> 
> Ah, good point, I missed those.
> 
> Where/how are you getting warnings thrown? I'm not seeing anything
> from xfstests runs?
> 

Oh, I hadn't reproduced warnings as such. I was just digging down that
path and came across this in iput_final():

        WARN_ON(inode->i_state & I_NEW);

Brian

> -Dave.
> -- 
> Dave Chinner
> david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>