Re: [PATCH v2] NFS: nfs_rename() handle -ERESTARTSYS dentry left behind

["Benjamin Coddington" <bcodding@xxxxxxxxxx>]

On 15 Jun 2017, at 8:23, Jeff Layton wrote:

> On Wed, 2017-02-01 at 00:00 -0500, Benjamin Coddington wrote:
> > An interrupted rename will leave the old dentry behind if the rename
> > succeeds.  Fix this by moving the final local work of the rename to
> > rpc_call_done so that the results of the RENAME can always be 
> > handled,
> > even if the original process has already returned with -ERESTARTSYS.
> >
> > Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> > ---
> >  fs/nfs/dir.c | 36 +++++++++++++++++++++++++-----------
> >  1 file changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index fad81041f5ab..fb499a3f21b5 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -2002,6 +2002,29 @@ nfs_link(struct dentry *old_dentry, struct 
> > inode *dir, struct dentry *dentry)
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_link);
> >
> > +static void
> > +nfs_complete_rename(struct rpc_task *task, struct nfs_renamedata 
> > *data)
> > +{
> > +	struct dentry *old_dentry = data->old_dentry;
> > +	struct dentry *new_dentry = data->new_dentry;
> > +	struct inode *old_inode = d_inode(old_dentry);
> > +	struct inode *new_inode = d_inode(new_dentry);
> > +
> > +	nfs_mark_for_revalidate(old_inode);
> > +
> > +	switch (task->tk_status) {
> > +	case 0:
> > +		if (new_inode != NULL)
> > +			nfs_drop_nlink(new_inode);
> > +		d_move(old_dentry, new_dentry);
> > +		nfs_set_verifier(new_dentry,
> > +					nfs_save_change_attribute(data->new_dir));
> > +		break;
> > +	case -ENOENT:
> > +		nfs_dentry_handle_enoent(old_dentry);
> > +	}
> > +}
> > +
> >  /*
> >   * RENAME
> >   * FIXME: Some nfsds, like the Linux user space nfsd, may generate a
> > @@ -2084,7 +2107,8 @@ int nfs_rename(struct inode *old_dir, struct 
> > dentry *old_dentry,
> >  	if (new_inode != NULL)
> >  		NFS_PROTO(new_inode)->return_delegation(new_inode);
> >
> > -	task = nfs_async_rename(old_dir, new_dir, old_dentry, new_dentry, 
> > NULL);
> > +	task = nfs_async_rename(old_dir, new_dir, old_dentry, new_dentry,
> > +					nfs_complete_rename);
> >  	if (IS_ERR(task)) {
> >  		error = PTR_ERR(task);
> >  		goto out;
> > @@ -2094,21 +2118,11 @@ int nfs_rename(struct inode *old_dir, struct 
> > dentry *old_dentry,
> >  	if (error == 0)
> >  		error = task->tk_status;
> >  	rpc_put_task(task);
> > -	nfs_mark_for_revalidate(old_inode);
> >  out:
> >  	if (rehash)
> >  		d_rehash(rehash);
> >  	trace_nfs_rename_exit(old_dir, old_dentry,
> >  			new_dir, new_dentry, error);
> > -	if (!error) {
> > -		if (new_inode != NULL)
> > -			nfs_drop_nlink(new_inode);
> > -		d_move(old_dentry, new_dentry);
> > -		nfs_set_verifier(new_dentry,
> > -					nfs_save_change_attribute(new_dir));
> > -	} else if (error == -ENOENT)
> > -		nfs_dentry_handle_enoent(old_dentry);
> > -
> >  	/* new dentry created? */
> >  	if (dentry)
> >  		dput(dentry);
>
> Oof. I believe this patch is wrong and likely causing dcache
> corruption.. If the task catches a signal here then you'll be doing 
> the
> d_move without holding the i_rwsem, and that isn't safe.
>
> I think we need to go ahead and revert this immediately from mainline
> and v4.11-stable, and do what we discussed with the v1 patch -- have 
> the
> rename completion function detect whether the original process has 
> been
> killed and mark the parents for revalidation if they were.
>
> Thoughts?

I agree this was a mistake and should be reverted..  There's also the 
follow-up fix d4ea7e3c5c0e341c15b073016dbf3ab6c65f12f3.

I'll get testing on the original approach in the v1 - detecting the 
interrupt and revalidating both directories.

Ben
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html