Re: [PATCH 6/8] NFSD: use explicit lock/unlock for directory ops

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2022-07-15 at 12:11 -0400, Jeff Layton wrote:
> On Wed, 2022-07-06 at 14:18 +1000, NeilBrown wrote:
> > When creating or unlinking a name in a directory use explicit
> > inode_lock_nested() instead of fh_lock(), and explicit calls to
> > fh_fill_pre_attrs() and fh_fill_post_attrs().  This is already done for
> > renames.
> > 
> > Also move the 'fill' calls closer to the operation that might change the
> > attributes.  This way they are avoided on some error paths.
> > 
> > Having the locking explicit will simplify proposed future changes to
> > locking for directories.  It also makes it easily visible exactly where
> > pre/post attributes are used - not all callers of fh_lock() actually
> > need the pre/post attributes.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/nfsd/nfs3proc.c |    6 ++++--
> >  fs/nfsd/nfs4proc.c |    6 ++++--
> >  fs/nfsd/nfsproc.c  |    7 ++++---
> >  fs/nfsd/vfs.c      |   30 +++++++++++++++++++-----------
> >  4 files changed, 31 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > index 3a67d0afb885..9629517344ff 100644
> > --- a/fs/nfsd/nfs3proc.c
> > +++ b/fs/nfsd/nfs3proc.c
> > @@ -254,7 +254,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	if (host_err)
> >  		return nfserrno(host_err);
> >  
> > -	fh_lock_nested(fhp, I_MUTEX_PARENT);
> > +	inode_lock_nested(inode, I_MUTEX_PARENT);
> >  
> >  	child = lookup_one_len(argp->name, parent, argp->len);
> >  	if (IS_ERR(child)) {
> > @@ -312,11 +312,13 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	if (!IS_POSIXACL(inode))
> >  		iap->ia_mode &= ~current_umask();
> >  
> > +	fh_fill_pre_attrs(fhp);
> >  	host_err = vfs_create(&init_user_ns, inode, child, iap->ia_mode, true);
> >  	if (host_err < 0) {
> >  		status = nfserrno(host_err);
> >  		goto out;
> >  	}
> > +	fh_fill_post_attrs(fhp);
> >  
> >  	/* A newly created file already has a file size of zero. */
> >  	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
> > @@ -334,7 +336,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	status = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
> >  
> >  out:
> > -	fh_unlock(fhp);
> > +	inode_unlock(inode);
> >  	if (child && !IS_ERR(child))
> >  		dput(child);
> >  	fh_drop_write(fhp);
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 6ec22c69cbec..242f059e6788 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -306,7 +306,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	if (host_err)
> >  		return nfserrno(host_err);
> >  
> > -	fh_lock_nested(fhp, I_MUTEX_PARENT);
> > +	inode_lock_nested(inode, I_MUTEX_PARENT);
> >  
> >  	child = lookup_one_len(open->op_fname, parent, open->op_fnamelen);
> >  	if (IS_ERR(child)) {
> > @@ -385,10 +385,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	if (!IS_POSIXACL(inode))
> >  		iap->ia_mode &= ~current_umask();
> >  
> > +	fh_fill_pre_attrs(fhp);
> >  	status = nfsd4_vfs_create(fhp, child, open);
> >  	if (status != nfs_ok)
> >  		goto out;
> >  	open->op_created = true;
> > +	fh_fill_post_attrs(fhp);
> >  
> >  	/* A newly created file already has a file size of zero. */
> >  	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
> 
> Should the fh_fill_post_attrs call be done after nfsd_create_setattr
> instead in this function? It seems like we're filling out the post-op
> attr here before we're actually done changing things...
> 
> > @@ -406,7 +408,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	status = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
> >  
> >  out:
> > -	fh_unlock(fhp);
> > +	inode_unlock(inode);
> >  	if (child && !IS_ERR(child))
> >  		dput(child);
> >  	fh_drop_write(fhp);
> > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> > index ed24fae09517..427c404bc52b 100644
> > --- a/fs/nfsd/nfsproc.c
> > +++ b/fs/nfsd/nfsproc.c
> > @@ -285,7 +285,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> >  		goto done;
> >  	}
> >  
> > -	fh_lock_nested(dirfhp, I_MUTEX_PARENT);
> > +	inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT);
> >  	dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len);
> >  	if (IS_ERR(dchild)) {
> >  		resp->status = nfserrno(PTR_ERR(dchild));
> > @@ -382,6 +382,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> >  	}
> >  
> >  	resp->status = nfs_ok;
> > +	fh_fill_pre_attrs(dirfhp);
> >  	if (!inode) {
> >  		/* File doesn't exist. Create it and set attrs */
> >  		resp->status = nfsd_create_locked(rqstp, dirfhp, argp->name,
> > @@ -399,10 +400,10 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> >  			resp->status = nfsd_setattr(rqstp, newfhp, attr, 0,
> >  						    (time64_t)0);
> >  	}
> > +	fh_fill_post_attrs(dirfhp);
> >  
> >  out_unlock:
> > -	/* We don't really need to unlock, as fh_put does it. */
> > -	fh_unlock(dirfhp);
> > +	inode_unlock(dirfhp->fh_dentry->d_inode);
> >  	fh_drop_write(dirfhp);
> >  done:
> >  	fh_put(dirfhp);
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 8e050c6d112a..2ca748aa83bb 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1412,7 +1412,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	if (host_err)
> >  		return nfserrno(host_err);
> >  
> > -	fh_lock_nested(fhp, I_MUTEX_PARENT);
> > +	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> >  	dchild = lookup_one_len(fname, dentry, flen);
> >  	host_err = PTR_ERR(dchild);
> >  	if (IS_ERR(dchild)) {
> > @@ -1427,12 +1427,14 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	dput(dchild);
> >  	if (err)
> >  		goto out_unlock;
> > +	fh_fill_pre_attrs(fhp);
> >  	err = nfsd_create_locked(rqstp, fhp, fname, flen, iap, type,
> >  				 rdev, resfhp);
> >  	if (!err && post_create)
> >  		post_create(resfhp, data);
> > +	fh_fill_post_attrs(fhp);
> >  out_unlock:
> > -	fh_unlock(fhp);
> > +	inode_unlock(dentry->d_inode);
> >  	return err;
> >  }
> >  
> > @@ -1505,14 +1507,15 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	if (host_err)
> >  		goto out_nfserr;
> >  
> > -	fh_lock(fhp);
> >  	dentry = fhp->fh_dentry;
> > +	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> >  	dnew = lookup_one_len(fname, dentry, flen);
> >  	host_err = PTR_ERR(dnew);
> >  	if (IS_ERR(dnew)) {
> > -		fh_unlock(fhp);
> > +		inode_unlock(dentry->d_inode);
> >  		goto out_nfserr;
> >  	}
> > +	fh_fill_pre_attrs(fhp);
> >  	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> >  	err = nfserrno(host_err);
> >  	if (!err)
> > @@ -1525,7 +1528,8 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	if (err==0) err = cerr;
> >  	if (!err && post_create)
> >  		post_create(resfhp, data);
> > -	fh_unlock(fhp);
> > +	fh_fill_post_attrs(fhp);
> > +	inode_unlock(dentry->d_inode);
> >  out:
> >  	return err;
> >  
> > @@ -1569,9 +1573,9 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >  		goto out;
> >  	}
> >  
> > -	fh_lock_nested(ffhp, I_MUTEX_PARENT);
> >  	ddir = ffhp->fh_dentry;
> >  	dirp = d_inode(ddir);
> > +	inode_lock_nested(dirp, I_MUTEX_PARENT);
> >  
> >  	dnew = lookup_one_len(name, ddir, len);
> >  	host_err = PTR_ERR(dnew);
> > @@ -1585,8 +1589,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >  	err = nfserr_noent;
> >  	if (d_really_is_negative(dold))
> >  		goto out_dput;
> > +	fh_fill_pre_attrs(ffhp);
> >  	host_err = vfs_link(dold, &init_user_ns, dirp, dnew, NULL);
> > -	fh_unlock(ffhp);
> > +	fh_fill_post_attrs(ffhp);
> > +	inode_unlock(dirp);
> >  	if (!host_err) {
> >  		err = nfserrno(commit_metadata(ffhp));
> >  		if (!err)
> > @@ -1606,7 +1612,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >  out_dput:
> >  	dput(dnew);
> >  out_unlock:
> > -	fh_unlock(ffhp);
> > +	inode_unlock(dirp);
> >  	goto out_drop_write;
> >  }
> >  
> > @@ -1781,9 +1787,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> >  	if (host_err)
> >  		goto out_nfserr;
> >  
> > -	fh_lock_nested(fhp, I_MUTEX_PARENT);
> >  	dentry = fhp->fh_dentry;
> >  	dirp = d_inode(dentry);
> > +	inode_lock_nested(dirp, I_MUTEX_PARENT);
> >  
> >  	rdentry = lookup_one_len(fname, dentry, flen);
> >  	host_err = PTR_ERR(rdentry);
> > @@ -1801,6 +1807,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> >  	if (!type)
> >  		type = d_inode(rdentry)->i_mode & S_IFMT;
> >  
> > +	fh_fill_pre_attrs(fhp);
> >  	if (type != S_IFDIR) {
> >  		if (rdentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK)
> >  			nfsd_close_cached_files(rdentry);
> > @@ -1808,8 +1815,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> >  	} else {
> >  		host_err = vfs_rmdir(&init_user_ns, dirp, rdentry);
> >  	}
> > +	fh_fill_post_attrs(fhp);
> >  
> > -	fh_unlock(fhp);
> > +	inode_unlock(dirp);
> >  	if (!host_err)
> >  		host_err = commit_metadata(fhp);
> >  	dput(rdentry);
> > @@ -1832,7 +1840,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> >  out:
> >  	return err;
> >  out_unlock:
> > -	fh_unlock(fhp);
> > +	inode_unlock(dirp);
> >  	goto out_drop_write;
> >  }
> >  
> > 
> > 
> 


[PATCH] SQUASH: nfsd: ensure we fill in pre-op-attrs in
 nfsd4_create_file

In some cases, they're left uninitialized. This also ensures that the
post_op attrs are properly filled in all cases too.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4proc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 242f059e6788..05652a7dabe8 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -346,6 +346,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 		switch (open->op_createmode) {
 		case NFS4_CREATE_UNCHECKED:
+			fh_fill_pre_attrs(fhp);
 			if (!d_is_reg(child))
 				break;
 
@@ -365,6 +366,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			if (d_inode(child)->i_mtime.tv_sec == v_mtime &&
 			    d_inode(child)->i_atime.tv_sec == v_atime &&
 			    d_inode(child)->i_size == 0) {
+				fh_fill_pre_attrs(fhp);
 				open->op_created = true;
 				break;		/* subtle */
 			}
@@ -374,6 +376,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			if (d_inode(child)->i_mtime.tv_sec == v_mtime &&
 			    d_inode(child)->i_atime.tv_sec == v_atime &&
 			    d_inode(child)->i_size == 0) {
+				fh_fill_pre_attrs(fhp);
 				open->op_created = true;
 				goto set_attr;	/* subtle */
 			}
@@ -385,12 +388,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (!IS_POSIXACL(inode))
 		iap->ia_mode &= ~current_umask();
 
-	fh_fill_pre_attrs(fhp);
 	status = nfsd4_vfs_create(fhp, child, open);
 	if (status != nfs_ok)
 		goto out;
 	open->op_created = true;
-	fh_fill_post_attrs(fhp);
 
 	/* A newly created file already has a file size of zero. */
 	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
@@ -408,6 +409,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	status = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
 
 out:
+	if (status == nfs_ok)
+		fh_fill_post_attrs(fhp);
 	inode_unlock(inode);
 	if (child && !IS_ERR(child))
 		dput(child);
-- 
2.36.1