[PATCH 00/13] NFSD: clean up locking

[PATCH 00/13] NFSD: clean up locking

[NeilBrown]

This is the latest version of my series to clean up locking -
particularly of directories - in preparation for proposed patches which
change how directory locking works across the VFS.

I've included Jeff's patches to validate the dentry after getting a
delegation.  The second patch has been changed quite a bit to use
nfsd_lookup_dentry(). I've left Jeff's From: line in place - let me know
if you'd rather I change it.

Setting of ACLs and security labels has been moved from nfs4 code to
nfsd_setattr() which allows quite a lot of code cleanup.

I think I've addressed all the concerns that have been raised, though
maybe not in the way that was suggested.

I've tested this with cthon tests over v2, v3, v4.0, v4.1, and xfstests
on v3 and v4.1, and pynfs 4.0, 4.1.  No problems appeared.

Thanks,
NeilBrown


---

Jeff Layton (2):
      NFSD: drop fh argument from alloc_init_deleg
      NFSD: verify the opened dentry after setting a delegation

NeilBrown (11):
      NFSD: introduce struct nfsd_attrs
      NFSD: set attributes when creating symlinks
      NFSD: add security label to struct nfsd_attrs
      NFSD: add posix ACLs to struct nfsd_attrs
      NFSD: change nfsd_create()/nfsd_symlink() to unlock directory before returning.
      NFSD: always drop directory lock in nfsd_unlink()
      NFSD: only call fh_unlock() once in nfsd_link()
      NFSD: reduce locking in nfsd_lookup()
      NFSD: use explicit lock/unlock for directory ops
      NFSD: use (un)lock_inode instead of fh_(un)lock for file operations
      NFSD: discard fh_locked flag and fh_lock/fh_unlock


 fs/nfsd/acl.h       |   6 +-
 fs/nfsd/nfs2acl.c   |   6 +-
 fs/nfsd/nfs3acl.c   |   4 +-
 fs/nfsd/nfs3proc.c  |  25 ++---
 fs/nfsd/nfs4acl.c   |  46 ++-------
 fs/nfsd/nfs4proc.c  | 153 ++++++++++++-----------------
 fs/nfsd/nfs4state.c |  71 +++++++++++---
 fs/nfsd/nfsfh.c     |  22 ++++-
 fs/nfsd/nfsfh.h     |  58 +----------
 fs/nfsd/nfsproc.c   |  19 ++--
 fs/nfsd/vfs.c       | 230 +++++++++++++++++++++-----------------------
 fs/nfsd/vfs.h       |  31 ++++--
 fs/nfsd/xdr4.h      |   1 +
 13 files changed, 314 insertions(+), 358 deletions(-)

--
Signature

[PATCH 13/13] NFSD: discard fh_locked flag and fh_lock/fh_unlock

[NeilBrown]

As all inode locking is now fully balanced, fh_put() does not need to
call fh_unlock().
fh_lock() and fh_unlock() are no longer used, so discard them.
These are the only real users of ->fh_locked, so discard that too.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfsfh.c |    3 +--
 fs/nfsd/nfsfh.h |   56 ++++---------------------------------------------------
 fs/nfsd/vfs.c   |   17 +----------------
 3 files changed, 6 insertions(+), 70 deletions(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index cd2946a88d72..a5b71526cee0 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -549,7 +549,7 @@ fh_compose(struct svc_fh *fhp, struct svc_export *exp, struct dentry *dentry,
 	if (ref_fh == fhp)
 		fh_put(ref_fh);
 
-	if (fhp->fh_locked || fhp->fh_dentry) {
+	if (fhp->fh_dentry) {
 		printk(KERN_ERR "fh_compose: fh %pd2 not initialized!\n",
 		       dentry);
 	}
@@ -700,7 +700,6 @@ fh_put(struct svc_fh *fhp)
 	struct dentry * dentry = fhp->fh_dentry;
 	struct svc_export * exp = fhp->fh_export;
 	if (dentry) {
-		fh_unlock(fhp);
 		fhp->fh_dentry = NULL;
 		dput(dentry);
 		fh_clear_pre_post_attrs(fhp);
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 28a4f9a94e2c..c3ae6414fc5c 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -81,7 +81,6 @@ typedef struct svc_fh {
 	struct dentry *		fh_dentry;	/* validated dentry */
 	struct svc_export *	fh_export;	/* export pointer */
 
-	bool			fh_locked;	/* inode locked by us */
 	bool			fh_want_write;	/* remount protection taken */
 	bool			fh_no_wcc;	/* no wcc data needed */
 	bool			fh_no_atomic_attr;
@@ -93,7 +92,7 @@ typedef struct svc_fh {
 	bool			fh_post_saved;	/* post-op attrs saved */
 	bool			fh_pre_saved;	/* pre-op attrs saved */
 
-	/* Pre-op attributes saved during fh_lock */
+	/* Pre-op attributes saved when inode is locked */
 	__u64			fh_pre_size;	/* size before operation */
 	struct timespec64	fh_pre_mtime;	/* mtime before oper */
 	struct timespec64	fh_pre_ctime;	/* ctime before oper */
@@ -103,7 +102,7 @@ typedef struct svc_fh {
 	 */
 	u64			fh_pre_change;
 
-	/* Post-op attributes saved in fh_unlock */
+	/* Post-op attributes saved in fh_fill_post_attrs() */
 	struct kstat		fh_post_attr;	/* full attrs after operation */
 	u64			fh_post_change; /* nfsv4 change; see above */
 } svc_fh;
@@ -223,8 +222,8 @@ void	fh_put(struct svc_fh *);
 static __inline__ struct svc_fh *
 fh_copy(struct svc_fh *dst, struct svc_fh *src)
 {
-	WARN_ON(src->fh_dentry || src->fh_locked);
-			
+	WARN_ON(src->fh_dentry);
+
 	*dst = *src;
 	return dst;
 }
@@ -323,51 +322,4 @@ static inline u64 nfsd4_change_attribute(struct kstat *stat,
 extern void fh_fill_pre_attrs(struct svc_fh *fhp);
 extern void fh_fill_post_attrs(struct svc_fh *fhp);
 extern void fh_fill_both_attrs(struct svc_fh *fhp);
-
-/*
- * Lock a file handle/inode
- * NOTE: both fh_lock and fh_unlock are done "by hand" in
- * vfs.c:nfsd_rename as it needs to grab 2 i_mutex's at once
- * so, any changes here should be reflected there.
- */
-
-static inline void
-fh_lock_nested(struct svc_fh *fhp, unsigned int subclass)
-{
-	struct dentry	*dentry = fhp->fh_dentry;
-	struct inode	*inode;
-
-	BUG_ON(!dentry);
-
-	if (fhp->fh_locked) {
-		printk(KERN_WARNING "fh_lock: %pd2 already locked!\n",
-			dentry);
-		return;
-	}
-
-	inode = d_inode(dentry);
-	inode_lock_nested(inode, subclass);
-	fh_fill_pre_attrs(fhp);
-	fhp->fh_locked = true;
-}
-
-static inline void
-fh_lock(struct svc_fh *fhp)
-{
-	fh_lock_nested(fhp, I_MUTEX_NORMAL);
-}
-
-/*
- * Unlock a file handle/inode
- */
-static inline void
-fh_unlock(struct svc_fh *fhp)
-{
-	if (fhp->fh_locked) {
-		fh_fill_post_attrs(fhp);
-		inode_unlock(d_inode(fhp->fh_dentry));
-		fhp->fh_locked = false;
-	}
-}
-
 #endif /* _LINUX_NFSD_NFSFH_H */
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 1d96d89a4801..4a1717683b96 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1264,13 +1264,6 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	dirp = d_inode(dentry);
 
 	dchild = dget(resfhp->fh_dentry);
-	if (!fhp->fh_locked) {
-		WARN_ONCE(1, "nfsd_create: parent %pd2 not locked!\n",
-				dentry);
-		err = nfserr_io;
-		goto out;
-	}
-
 	err = nfsd_permission(rqstp, fhp->fh_export, dentry, NFSD_MAY_CREATE);
 	if (err)
 		goto out;
@@ -1623,10 +1616,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 		goto out;
 	}
 
-	/* cannot use fh_lock as we need deadlock protective ordering
-	 * so do it by hand */
 	trap = lock_rename(tdentry, fdentry);
-	ffhp->fh_locked = tfhp->fh_locked = true;
 	fh_fill_pre_attrs(ffhp);
 	fh_fill_pre_attrs(tfhp);
 
@@ -1682,17 +1672,12 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 	dput(odentry);
  out_nfserr:
 	err = nfserrno(host_err);
-	/*
-	 * We cannot rely on fh_unlock on the two filehandles,
-	 * as that would do the wrong thing if the two directories
-	 * were the same, so again we do it by hand.
-	 */
+
 	if (!close_cached) {
 		fh_fill_post_attrs(ffhp);
 		fh_fill_post_attrs(tfhp);
 	}
 	unlock_rename(tdentry, fdentry);
-	ffhp->fh_locked = tfhp->fh_locked = false;
 	fh_drop_write(ffhp);
 
 	/*

[PATCH 01/13] NFSD: drop fh argument from alloc_init_deleg

[NeilBrown]

From: Jeff Layton <jlayton@kernel.org>

Currently, we pass the fh of the opened file down through several
functions so that alloc_init_deleg can pass it to delegation_blocked.
The filehandle of the open file is available in the nfs4_file however,
so there's no need to pass it in a separate argument.

Drop the argument from alloc_init_deleg, nfs4_open_delegation and
nfs4_set_delegation.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e46e3392d557..279c7a1502c9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1131,7 +1131,6 @@ static void block_delegations(struct knfsd_fh *fh)
 
 static struct nfs4_delegation *
 alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
-		 struct svc_fh *current_fh,
 		 struct nfs4_clnt_odstate *odstate)
 {
 	struct nfs4_delegation *dp;
@@ -1141,7 +1140,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
 	n = atomic_long_inc_return(&num_delegations);
 	if (n < 0 || n > max_delegations)
 		goto out_dec;
-	if (delegation_blocked(&current_fh->fh_handle))
+	if (delegation_blocked(&fp->fi_fhandle))
 		goto out_dec;
 	dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab, nfs4_free_deleg));
 	if (dp == NULL)
@@ -5290,7 +5289,7 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
 }
 
 static struct nfs4_delegation *
-nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
+nfs4_set_delegation(struct nfs4_client *clp,
 		    struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate)
 {
 	int status = 0;
@@ -5335,7 +5334,7 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
 		return ERR_PTR(status);
 
 	status = -ENOMEM;
-	dp = alloc_init_deleg(clp, fp, fh, odstate);
+	dp = alloc_init_deleg(clp, fp, odstate);
 	if (!dp)
 		goto out_delegees;
 
@@ -5403,8 +5402,7 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
  * proper support for them.
  */
 static void
-nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
-			struct nfs4_ol_stateid *stp)
+nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
 {
 	struct nfs4_delegation *dp;
 	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
@@ -5436,7 +5434,7 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
 		default:
 			goto out_no_deleg;
 	}
-	dp = nfs4_set_delegation(clp, fh, stp->st_stid.sc_file, stp->st_clnt_odstate);
+	dp = nfs4_set_delegation(clp, stp->st_stid.sc_file, stp->st_clnt_odstate);
 	if (IS_ERR(dp))
 		goto out_no_deleg;
 
@@ -5568,7 +5566,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	* Attempt to hand out a delegation. No error return, because the
 	* OPEN succeeds even if we fail.
 	*/
-	nfs4_open_delegation(current_fh, open, stp);
+	nfs4_open_delegation(open, stp);
 nodeleg:
 	status = nfs_ok;
 	trace_nfsd_open(&stp->st_stid.sc_stateid);

[PATCH 12/13] NFSD: use (un)lock_inode instead of fh_(un)lock for file operations

[NeilBrown]

When locking a file to access ACLs and xattrs etc, use explicit locking
with inode_lock() instead of fh_lock().  This means that the calls to
fh_fill_pre/post_attr() are also explicit which improves readability and
allows us to place them only where they are needed.  Only the xattr
calls need pre/post information.

When locking a file we don't need I_MUTEX_PARENT as the file is not a
parent of anything, so we can use inode_lock() directly rather than the
inode_lock_nested() call that fh_lock() uses.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs2acl.c   |    6 +++---
 fs/nfsd/nfs3acl.c   |    4 ++--
 fs/nfsd/nfs4state.c |    9 +++++----
 fs/nfsd/vfs.c       |   25 ++++++++++++-------------
 4 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index b5760801d377..9edd3c1a30fb 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -111,7 +111,7 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst *rqstp)
 	if (error)
 		goto out_errno;
 
-	fh_lock(fh);
+	inode_lock(inode);
 
 	error = set_posix_acl(&init_user_ns, inode, ACL_TYPE_ACCESS,
 			      argp->acl_access);
@@ -122,7 +122,7 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst *rqstp)
 	if (error)
 		goto out_drop_lock;
 
-	fh_unlock(fh);
+	inode_unlock(inode);
 
 	fh_drop_write(fh);
 
@@ -136,7 +136,7 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst *rqstp)
 	return rpc_success;
 
 out_drop_lock:
-	fh_unlock(fh);
+	inode_unlock(inode);
 	fh_drop_write(fh);
 out_errno:
 	resp->status = nfserrno(error);
diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
index 35b2ebda14da..9446c6743664 100644
--- a/fs/nfsd/nfs3acl.c
+++ b/fs/nfsd/nfs3acl.c
@@ -101,7 +101,7 @@ static __be32 nfsd3_proc_setacl(struct svc_rqst *rqstp)
 	if (error)
 		goto out_errno;
 
-	fh_lock(fh);
+	inode_lock(inode);
 
 	error = set_posix_acl(&init_user_ns, inode, ACL_TYPE_ACCESS,
 			      argp->acl_access);
@@ -111,7 +111,7 @@ static __be32 nfsd3_proc_setacl(struct svc_rqst *rqstp)
 			      argp->acl_default);
 
 out_drop_lock:
-	fh_unlock(fh);
+	inode_unlock(inode);
 	fh_drop_write(fh);
 out_errno:
 	resp->status = nfserrno(error);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 45df1e85ff32..b9be12b3cebd 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7397,21 +7397,22 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 static __be32 nfsd_test_lock(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file_lock *lock)
 {
 	struct nfsd_file *nf;
+	struct inode *inode;
 	__be32 err;
 
 	err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
 	if (err)
 		return err;
-	fh_lock(fhp); /* to block new leases till after test_lock: */
-	err = nfserrno(nfsd_open_break_lease(fhp->fh_dentry->d_inode,
-							NFSD_MAY_READ));
+	inode = fhp->fh_dentry->d_inode;
+	inode_lock(inode); /* to block new leases till after test_lock: */
+	err = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
 	if (err)
 		goto out;
 	lock->fl_file = nf->nf_file;
 	err = nfserrno(vfs_test_lock(nf->nf_file, lock));
 	lock->fl_file = NULL;
 out:
-	fh_unlock(fhp);
+	inode_unlock(inode);
 	nfsd_file_put(nf);
 	return err;
 }
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index f2cb9b047766..1d96d89a4801 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -416,7 +416,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			return err;
 	}
 
-	fh_lock(fhp);
+	inode_lock(inode);
 	if (size_change) {
 		/*
 		 * RFC5661, Section 18.30.4:
@@ -463,7 +463,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		attr->acl_failed = set_posix_acl(&init_user_ns,
 						 inode, ACL_TYPE_DEFAULT,
 						 attr->dpacl);
-	fh_unlock(fhp);
+	inode_unlock(inode);
 	if (size_change)
 		put_write_access(inode);
 out:
@@ -2145,12 +2145,8 @@ nfsd_listxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char **bufp,
 }
 
 /*
- * Removexattr and setxattr need to call fh_lock to both lock the inode
- * and set the change attribute. Since the top-level vfs_removexattr
- * and vfs_setxattr calls already do their own inode_lock calls, call
- * the _locked variant. Pass in a NULL pointer for delegated_inode,
- * and let the client deal with NFS4ERR_DELAY (same as with e.g.
- * setattr and remove).
+ * Pass in a NULL pointer for delegated_inode, and let the client deal
+ * with NFS4ERR_DELAY (same as with e.g.  setattr and remove).
  */
 __be32
 nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name)
@@ -2166,12 +2162,14 @@ nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name)
 	if (ret)
 		return nfserrno(ret);
 
-	fh_lock(fhp);
+	inode_lock(fhp->fh_dentry->d_inode);
+	fh_fill_pre_attrs(fhp);
 
 	ret = __vfs_removexattr_locked(&init_user_ns, fhp->fh_dentry,
 				       name, NULL);
 
-	fh_unlock(fhp);
+	fh_fill_post_attrs(fhp);
+	inode_unlock(fhp->fh_dentry->d_inode);
 	fh_drop_write(fhp);
 
 	return nfsd_xattr_errno(ret);
@@ -2191,12 +2189,13 @@ nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name,
 	ret = fh_want_write(fhp);
 	if (ret)
 		return nfserrno(ret);
-	fh_lock(fhp);
+	inode_lock(fhp->fh_dentry->d_inode);
+	fh_fill_pre_attrs(fhp);
 
 	ret = __vfs_setxattr_locked(&init_user_ns, fhp->fh_dentry, name, buf,
 				    len, flags, NULL);
-
-	fh_unlock(fhp);
+	fh_fill_post_attrs(fhp);
+	inode_unlock(fhp->fh_dentry->d_inode);
 	fh_drop_write(fhp);
 
 	return nfsd_xattr_errno(ret);

Re: [PATCH 12/13] NFSD: use (un)lock_inode instead of fh_(un)lock for file operations

[Chuck Lever III]

> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> 
> When locking a file to access ACLs and xattrs etc, use explicit locking
> with inode_lock() instead of fh_lock().  This means that the calls to
> fh_fill_pre/post_attr() are also explicit which improves readability and
> allows us to place them only where they are needed.  Only the xattr
> calls need pre/post information.
> 
> When locking a file we don't need I_MUTEX_PARENT as the file is not a
> parent of anything, so we can use inode_lock() directly rather than the
> inode_lock_nested() call that fh_lock() uses.
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs2acl.c   |    6 +++---
> fs/nfsd/nfs3acl.c   |    4 ++--
> fs/nfsd/nfs4state.c |    9 +++++----
> fs/nfsd/vfs.c       |   25 ++++++++++++-------------
> 4 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> index b5760801d377..9edd3c1a30fb 100644
> --- a/fs/nfsd/nfs2acl.c
> +++ b/fs/nfsd/nfs2acl.c
> @@ -111,7 +111,7 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst *rqstp)
> 	if (error)
> 		goto out_errno;
> 
> -	fh_lock(fh);
> +	inode_lock(inode);
> 
> 	error = set_posix_acl(&init_user_ns, inode, ACL_TYPE_ACCESS,
> 			      argp->acl_access);
> @@ -122,7 +122,7 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst *rqstp)
> 	if (error)
> 		goto out_drop_lock;
> 
> -	fh_unlock(fh);
> +	inode_unlock(inode);
> 
> 	fh_drop_write(fh);
> 
> @@ -136,7 +136,7 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst *rqstp)
> 	return rpc_success;
> 
> out_drop_lock:
> -	fh_unlock(fh);
> +	inode_unlock(inode);
> 	fh_drop_write(fh);
> out_errno:
> 	resp->status = nfserrno(error);
> diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
> index 35b2ebda14da..9446c6743664 100644
> --- a/fs/nfsd/nfs3acl.c
> +++ b/fs/nfsd/nfs3acl.c
> @@ -101,7 +101,7 @@ static __be32 nfsd3_proc_setacl(struct svc_rqst *rqstp)
> 	if (error)
> 		goto out_errno;
> 
> -	fh_lock(fh);
> +	inode_lock(inode);
> 
> 	error = set_posix_acl(&init_user_ns, inode, ACL_TYPE_ACCESS,
> 			      argp->acl_access);
> @@ -111,7 +111,7 @@ static __be32 nfsd3_proc_setacl(struct svc_rqst *rqstp)
> 			      argp->acl_default);
> 
> out_drop_lock:
> -	fh_unlock(fh);
> +	inode_unlock(inode);
> 	fh_drop_write(fh);
> out_errno:
> 	resp->status = nfserrno(error);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 45df1e85ff32..b9be12b3cebd 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7397,21 +7397,22 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> static __be32 nfsd_test_lock(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file_lock *lock)
> {
> 	struct nfsd_file *nf;
> +	struct inode *inode;
> 	__be32 err;
> 
> 	err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
> 	if (err)
> 		return err;
> -	fh_lock(fhp); /* to block new leases till after test_lock: */
> -	err = nfserrno(nfsd_open_break_lease(fhp->fh_dentry->d_inode,
> -							NFSD_MAY_READ));
> +	inode = fhp->fh_dentry->d_inode;
> +	inode_lock(inode); /* to block new leases till after test_lock: */
> +	err = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> 	if (err)
> 		goto out;
> 	lock->fl_file = nf->nf_file;
> 	err = nfserrno(vfs_test_lock(nf->nf_file, lock));
> 	lock->fl_file = NULL;
> out:
> -	fh_unlock(fhp);
> +	inode_unlock(inode);
> 	nfsd_file_put(nf);
> 	return err;
> }
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index f2cb9b047766..1d96d89a4801 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -416,7 +416,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 			return err;
> 	}
> 
> -	fh_lock(fhp);
> +	inode_lock(inode);
> 	if (size_change) {
> 		/*
> 		 * RFC5661, Section 18.30.4:
> @@ -463,7 +463,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 		attr->acl_failed = set_posix_acl(&init_user_ns,
> 						 inode, ACL_TYPE_DEFAULT,
> 						 attr->dpacl);
> -	fh_unlock(fhp);
> +	inode_unlock(inode);
> 	if (size_change)
> 		put_write_access(inode);
> out:
> @@ -2145,12 +2145,8 @@ nfsd_listxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char **bufp,
> }
> 
> /*
> - * Removexattr and setxattr need to call fh_lock to both lock the inode
> - * and set the change attribute. Since the top-level vfs_removexattr
> - * and vfs_setxattr calls already do their own inode_lock calls, call
> - * the _locked variant. Pass in a NULL pointer for delegated_inode,
> - * and let the client deal with NFS4ERR_DELAY (same as with e.g.
> - * setattr and remove).
> + * Pass in a NULL pointer for delegated_inode, and let the client deal
> + * with NFS4ERR_DELAY (same as with e.g.  setattr and remove).
>  */
> __be32
> nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name)

A kerneldoc comment would be nicer.


> @@ -2166,12 +2162,14 @@ nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name)
> 	if (ret)
> 		return nfserrno(ret);
> 
> -	fh_lock(fhp);
> +	inode_lock(fhp->fh_dentry->d_inode);
> +	fh_fill_pre_attrs(fhp);
> 
> 	ret = __vfs_removexattr_locked(&init_user_ns, fhp->fh_dentry,
> 				       name, NULL);
> 
> -	fh_unlock(fhp);
> +	fh_fill_post_attrs(fhp);
> +	inode_unlock(fhp->fh_dentry->d_inode);
> 	fh_drop_write(fhp);
> 
> 	return nfsd_xattr_errno(ret);
> @@ -2191,12 +2189,13 @@ nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name,
> 	ret = fh_want_write(fhp);
> 	if (ret)
> 		return nfserrno(ret);
> -	fh_lock(fhp);
> +	inode_lock(fhp->fh_dentry->d_inode);
> +	fh_fill_pre_attrs(fhp);
> 
> 	ret = __vfs_setxattr_locked(&init_user_ns, fhp->fh_dentry, name, buf,
> 				    len, flags, NULL);
> -
> -	fh_unlock(fhp);
> +	fh_fill_post_attrs(fhp);
> +	inode_unlock(fhp->fh_dentry->d_inode);
> 	fh_drop_write(fhp);
> 
> 	return nfsd_xattr_errno(ret);
> 
> 

--
Chuck Lever

[PATCH 04/13] NFSD: set attributes when creating symlinks

[NeilBrown]

The NFS protocol includes attributes when creating symlinks.
Linux does store attributes for symlinks and allows them to be set,
though they are not used for permission checking.

NFSD currently doesn't set standard (struct iattr) attributes when
creating symlinks, but for NFSv4 it does set ACLs and security labels.
This is inconsistent.

To improve consistency, pass the provided attributes into nfsd_symlink()
and call nfsd_create_setattr() to set them.

We ignore any error from nfsd_create_setattr().  It isn't really clear
what should be done if a file is successfully created, but the
attributes cannot be set.  NFS doesn't allow partial success to be
reported.  Reporting failure is probably more misleading than reporting
success, so the status is ignored.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs3proc.c |    3 ++-
 fs/nfsd/nfs4proc.c |    2 +-
 fs/nfsd/nfsproc.c  |    3 ++-
 fs/nfsd/vfs.c      |   11 ++++++-----
 fs/nfsd/vfs.h      |    5 +++--
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 289eb844d086..5e369096e42f 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
 {
 	struct nfsd3_symlinkargs *argp = rqstp->rq_argp;
 	struct nfsd3_diropres *resp = rqstp->rq_resp;
+	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
 
 	if (argp->tlen == 0) {
 		resp->status = nfserr_inval;
@@ -417,7 +418,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
 	fh_copy(&resp->dirfh, &argp->ffh);
 	fh_init(&resp->fh, NFS3_FHSIZE);
 	resp->status = nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
-				    argp->flen, argp->tname, &resp->fh);
+				    argp->flen, argp->tname, &attrs, &resp->fh);
 	kfree(argp->tname);
 out:
 	return rpc_success;
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ba750d76f515..ee72c94732f0 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -809,7 +809,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	case NF4LNK:
 		status = nfsd_symlink(rqstp, &cstate->current_fh,
 				      create->cr_name, create->cr_namelen,
-				      create->cr_data, &resfh);
+				      create->cr_data, &attrs, &resfh);
 		break;
 
 	case NF4BLK:
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 594d6f85c89f..d09d516188d2 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
 {
 	struct nfsd_symlinkargs *argp = rqstp->rq_argp;
 	struct nfsd_stat *resp = rqstp->rq_resp;
+	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
 	struct svc_fh	newfh;
 
 	if (argp->tlen > NFS_MAXPATHLEN) {
@@ -495,7 +496,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
 
 	fh_init(&newfh, NFS_FHSIZE);
 	resp->status = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
-				    argp->tname, &newfh);
+				    argp->tname, &attrs, &newfh);
 
 	kfree(argp->tname);
 	fh_put(&argp->ffh);
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index a85dc4dd4f3a..91c9ea09f921 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1451,9 +1451,9 @@ nfsd_readlink(struct svc_rqst *rqstp, struct svc_fh *fhp, char *buf, int *lenp)
  */
 __be32
 nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
-				char *fname, int flen,
-				char *path,
-				struct svc_fh *resfhp)
+	     char *fname, int flen,
+	     char *path, struct nfsd_attrs *attrs,
+	     struct svc_fh *resfhp)
 {
 	struct dentry	*dentry, *dnew;
 	__be32		err, cerr;
@@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
 	err = nfserrno(host_err);
+	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
+	if (!err)
+		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
 	fh_unlock(fhp);
 	if (!err)
 		err = nfserrno(commit_metadata(fhp));
-
 	fh_drop_write(fhp);
 
-	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
 	dput(dnew);
 	if (err==0) err = cerr;
 out:
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 9bb0e3957982..f3f43ca3ac6b 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -114,8 +114,9 @@ __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
 				char *, int *);
 __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
-				char *name, int len, char *path,
-				struct svc_fh *res);
+			     char *name, int len, char *path,
+			     struct nfsd_attrs *attrs,
+			     struct svc_fh *res);
 __be32		nfsd_link(struct svc_rqst *, struct svc_fh *,
 				char *, int, struct svc_fh *);
 ssize_t		nfsd_copy_file_range(struct file *, u64,

Re: [PATCH 04/13] NFSD: set attributes when creating symlinks

[Chuck Lever III]

> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> 
> The NFS protocol includes attributes when creating symlinks.
> Linux does store attributes for symlinks and allows them to be set,
> though they are not used for permission checking.
> 
> NFSD currently doesn't set standard (struct iattr) attributes when
> creating symlinks, but for NFSv4 it does set ACLs and security labels.
> This is inconsistent.
> 
> To improve consistency, pass the provided attributes into nfsd_symlink()
> and call nfsd_create_setattr() to set them.
> 
> We ignore any error from nfsd_create_setattr().  It isn't really clear
> what should be done if a file is successfully created, but the
> attributes cannot be set.  NFS doesn't allow partial success to be
> reported.  Reporting failure is probably more misleading than reporting
> success, so the status is ignored.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>

While I was trying to get more information about how
security labels are supposed to be applied to symlinks,
Jeff, Bruce, and I had a brief private discussion about
it. Jeff has the impression that NFSD should not apply
ACLs/security labels to symlinks, and we believe that
would be somewhat of a change in behavior.

Now is a good time to hoist that private discussion to
the list, so I'm mentioning it here. I'm thinking it
would be appropriate to introduce that change in this
series, and probably right here in this patch, if we
agree that is the right thing to do going forward.

Otherwise, after a brief glance at the series, it looks
better to me than the previous approach. I will try to
dig in later this week so we can get this work merged at
the end of the upcoming merge window.


> ---
> fs/nfsd/nfs3proc.c |    3 ++-
> fs/nfsd/nfs4proc.c |    2 +-
> fs/nfsd/nfsproc.c  |    3 ++-
> fs/nfsd/vfs.c      |   11 ++++++-----
> fs/nfsd/vfs.h      |    5 +++--
> 5 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 289eb844d086..5e369096e42f 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> {
> 	struct nfsd3_symlinkargs *argp = rqstp->rq_argp;
> 	struct nfsd3_diropres *resp = rqstp->rq_resp;
> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> 
> 	if (argp->tlen == 0) {
> 		resp->status = nfserr_inval;
> @@ -417,7 +418,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> 	fh_copy(&resp->dirfh, &argp->ffh);
> 	fh_init(&resp->fh, NFS3_FHSIZE);
> 	resp->status = nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
> -				    argp->flen, argp->tname, &resp->fh);
> +				    argp->flen, argp->tname, &attrs, &resp->fh);
> 	kfree(argp->tname);
> out:
> 	return rpc_success;
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index ba750d76f515..ee72c94732f0 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -809,7 +809,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	case NF4LNK:
> 		status = nfsd_symlink(rqstp, &cstate->current_fh,
> 				      create->cr_name, create->cr_namelen,
> -				      create->cr_data, &resfh);
> +				      create->cr_data, &attrs, &resfh);
> 		break;
> 
> 	case NF4BLK:
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 594d6f85c89f..d09d516188d2 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> {
> 	struct nfsd_symlinkargs *argp = rqstp->rq_argp;
> 	struct nfsd_stat *resp = rqstp->rq_resp;
> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> 	struct svc_fh	newfh;
> 
> 	if (argp->tlen > NFS_MAXPATHLEN) {
> @@ -495,7 +496,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> 
> 	fh_init(&newfh, NFS_FHSIZE);
> 	resp->status = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
> -				    argp->tname, &newfh);
> +				    argp->tname, &attrs, &newfh);
> 
> 	kfree(argp->tname);
> 	fh_put(&argp->ffh);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index a85dc4dd4f3a..91c9ea09f921 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1451,9 +1451,9 @@ nfsd_readlink(struct svc_rqst *rqstp, struct svc_fh *fhp, char *buf, int *lenp)
>  */
> __be32
> nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -				char *fname, int flen,
> -				char *path,
> -				struct svc_fh *resfhp)
> +	     char *fname, int flen,
> +	     char *path, struct nfsd_attrs *attrs,
> +	     struct svc_fh *resfhp)
> {
> 	struct dentry	*dentry, *dnew;
> 	__be32		err, cerr;
> @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 
> 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> 	err = nfserrno(host_err);
> +	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> +	if (!err)
> +		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> 	fh_unlock(fhp);
> 	if (!err)
> 		err = nfserrno(commit_metadata(fhp));
> -
> 	fh_drop_write(fhp);
> 
> -	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> 	dput(dnew);
> 	if (err==0) err = cerr;
> out:
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 9bb0e3957982..f3f43ca3ac6b 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -114,8 +114,9 @@ __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
> 				char *, int *);
> __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
> -				char *name, int len, char *path,
> -				struct svc_fh *res);
> +			     char *name, int len, char *path,
> +			     struct nfsd_attrs *attrs,
> +			     struct svc_fh *res);
> __be32		nfsd_link(struct svc_rqst *, struct svc_fh *,
> 				char *, int, struct svc_fh *);
> ssize_t		nfsd_copy_file_range(struct file *, u64,
> 
> 

--
Chuck Lever

Re: [PATCH 04/13] NFSD: set attributes when creating symlinks

[Jeff Layton]

On Tue, 2022-07-26 at 12:40 +0000, Chuck Lever III wrote:
> 
> > On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> > 
> > The NFS protocol includes attributes when creating symlinks.
> > Linux does store attributes for symlinks and allows them to be set,
> > though they are not used for permission checking.
> > 
> > NFSD currently doesn't set standard (struct iattr) attributes when
> > creating symlinks, but for NFSv4 it does set ACLs and security labels.
> > This is inconsistent.
> > 
> > To improve consistency, pass the provided attributes into nfsd_symlink()
> > and call nfsd_create_setattr() to set them.
> > 
> > We ignore any error from nfsd_create_setattr().  It isn't really clear
> > what should be done if a file is successfully created, but the
> > attributes cannot be set.  NFS doesn't allow partial success to be
> > reported.  Reporting failure is probably more misleading than reporting
> > success, so the status is ignored.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> 
> While I was trying to get more information about how
> security labels are supposed to be applied to symlinks,
> Jeff, Bruce, and I had a brief private discussion about
> it. Jeff has the impression that NFSD should not apply
> ACLs/security labels to symlinks, and we believe that
> would be somewhat of a change in behavior.
> 
> Now is a good time to hoist that private discussion to
> the list, so I'm mentioning it here. I'm thinking it
> would be appropriate to introduce that change in this
> series, and probably right here in this patch, if we
> agree that is the right thing to do going forward.
> 
> Otherwise, after a brief glance at the series, it looks
> better to me than the previous approach. I will try to
> dig in later this week so we can get this work merged at
> the end of the upcoming merge window.
> 

My take was a bit more nuanced... ;)

The kernel clearly doesn't do any mode/acl/label enforcement when
traversing symlinks, but I think we do at least need to allow security
labels to be set on them. We can set labels on symlinks on local
filesystems. Installing software in (e.g.) SELinux labeled environments
might go awry if we don't allow this. It could also make a difference in
who is allowed to unlink them too.

NFSv4 ACLs are more ambiguous. It's probably not terribly harmful to
allow them to be set on symlinks, but the value of doing so is not
clear. We could probably just ignore them and no one would care. OTOH,
maybe DELETE permission actually matters here? Do we enforce that in any
way today?

These might be good questions to take to the IETF list...


> 
> > ---
> > fs/nfsd/nfs3proc.c |    3 ++-
> > fs/nfsd/nfs4proc.c |    2 +-
> > fs/nfsd/nfsproc.c  |    3 ++-
> > fs/nfsd/vfs.c      |   11 ++++++-----
> > fs/nfsd/vfs.h      |    5 +++--
> > 5 files changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > index 289eb844d086..5e369096e42f 100644
> > --- a/fs/nfsd/nfs3proc.c
> > +++ b/fs/nfsd/nfs3proc.c
> > @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> > {
> > 	struct nfsd3_symlinkargs *argp = rqstp->rq_argp;
> > 	struct nfsd3_diropres *resp = rqstp->rq_resp;
> > +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> > 
> > 	if (argp->tlen == 0) {
> > 		resp->status = nfserr_inval;
> > @@ -417,7 +418,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> > 	fh_copy(&resp->dirfh, &argp->ffh);
> > 	fh_init(&resp->fh, NFS3_FHSIZE);
> > 	resp->status = nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
> > -				    argp->flen, argp->tname, &resp->fh);
> > +				    argp->flen, argp->tname, &attrs, &resp->fh);
> > 	kfree(argp->tname);
> > out:
> > 	return rpc_success;
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index ba750d76f515..ee72c94732f0 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -809,7 +809,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > 	case NF4LNK:
> > 		status = nfsd_symlink(rqstp, &cstate->current_fh,
> > 				      create->cr_name, create->cr_namelen,
> > -				      create->cr_data, &resfh);
> > +				      create->cr_data, &attrs, &resfh);
> > 		break;
> > 
> > 	case NF4BLK:
> > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> > index 594d6f85c89f..d09d516188d2 100644
> > --- a/fs/nfsd/nfsproc.c
> > +++ b/fs/nfsd/nfsproc.c
> > @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> > {
> > 	struct nfsd_symlinkargs *argp = rqstp->rq_argp;
> > 	struct nfsd_stat *resp = rqstp->rq_resp;
> > +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> > 	struct svc_fh	newfh;
> > 
> > 	if (argp->tlen > NFS_MAXPATHLEN) {
> > @@ -495,7 +496,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> > 
> > 	fh_init(&newfh, NFS_FHSIZE);
> > 	resp->status = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
> > -				    argp->tname, &newfh);
> > +				    argp->tname, &attrs, &newfh);
> > 
> > 	kfree(argp->tname);
> > 	fh_put(&argp->ffh);
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index a85dc4dd4f3a..91c9ea09f921 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1451,9 +1451,9 @@ nfsd_readlink(struct svc_rqst *rqstp, struct svc_fh *fhp, char *buf, int *lenp)
> >  */
> > __be32
> > nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > -				char *fname, int flen,
> > -				char *path,
> > -				struct svc_fh *resfhp)
> > +	     char *fname, int flen,
> > +	     char *path, struct nfsd_attrs *attrs,
> > +	     struct svc_fh *resfhp)
> > {
> > 	struct dentry	*dentry, *dnew;
> > 	__be32		err, cerr;
> > @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 
> > 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> > 	err = nfserrno(host_err);
> > +	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> > +	if (!err)
> > +		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> > 	fh_unlock(fhp);
> > 	if (!err)
> > 		err = nfserrno(commit_metadata(fhp));
> > -
> > 	fh_drop_write(fhp);
> > 
> > -	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> > 	dput(dnew);
> > 	if (err==0) err = cerr;
> > out:
> > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> > index 9bb0e3957982..f3f43ca3ac6b 100644
> > --- a/fs/nfsd/vfs.h
> > +++ b/fs/nfsd/vfs.h
> > @@ -114,8 +114,9 @@ __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
> > 				char *, int *);
> > __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
> > -				char *name, int len, char *path,
> > -				struct svc_fh *res);
> > +			     char *name, int len, char *path,
> > +			     struct nfsd_attrs *attrs,
> > +			     struct svc_fh *res);
> > __be32		nfsd_link(struct svc_rqst *, struct svc_fh *,
> > 				char *, int, struct svc_fh *);
> > ssize_t		nfsd_copy_file_range(struct file *, u64,
> > 
> > 
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 04/13] NFSD: set attributes when creating symlinks

[Chuck Lever III]

> On Jul 26, 2022, at 8:53 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Tue, 2022-07-26 at 12:40 +0000, Chuck Lever III wrote:
>> 
>>> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
>>> 
>>> The NFS protocol includes attributes when creating symlinks.
>>> Linux does store attributes for symlinks and allows them to be set,
>>> though they are not used for permission checking.
>>> 
>>> NFSD currently doesn't set standard (struct iattr) attributes when
>>> creating symlinks, but for NFSv4 it does set ACLs and security labels.
>>> This is inconsistent.
>>> 
>>> To improve consistency, pass the provided attributes into nfsd_symlink()
>>> and call nfsd_create_setattr() to set them.
>>> 
>>> We ignore any error from nfsd_create_setattr().  It isn't really clear
>>> what should be done if a file is successfully created, but the
>>> attributes cannot be set.  NFS doesn't allow partial success to be
>>> reported.  Reporting failure is probably more misleading than reporting
>>> success, so the status is ignored.
>>> 
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>> 
>> While I was trying to get more information about how
>> security labels are supposed to be applied to symlinks,
>> Jeff, Bruce, and I had a brief private discussion about
>> it. Jeff has the impression that NFSD should not apply
>> ACLs/security labels to symlinks, and we believe that
>> would be somewhat of a change in behavior.
>> 
>> Now is a good time to hoist that private discussion to
>> the list, so I'm mentioning it here. I'm thinking it
>> would be appropriate to introduce that change in this
>> series, and probably right here in this patch, if we
>> agree that is the right thing to do going forward.
>> 
>> Otherwise, after a brief glance at the series, it looks
>> better to me than the previous approach. I will try to
>> dig in later this week so we can get this work merged at
>> the end of the upcoming merge window.
>> 
> 
> My take was a bit more nuanced... ;)

I was summarizing. Please feel free to correct me.


> The kernel clearly doesn't do any mode/acl/label enforcement when
> traversing symlinks, but I think we do at least need to allow security
> labels to be set on them. We can set labels on symlinks on local
> filesystems. Installing software in (e.g.) SELinux labeled environments
> might go awry if we don't allow this. It could also make a difference in
> who is allowed to unlink them too.
> 
> NFSv4 ACLs are more ambiguous. It's probably not terribly harmful to
> allow them to be set on symlinks, but the value of doing so is not
> clear. We could probably just ignore them and no one would care. OTOH,
> maybe DELETE permission actually matters here? Do we enforce that in any
> way today?

Sure -- we can always punt on making behavior changes now,
and simply continue to support what has been supported.
I'm not aware of many regression or functional tests in
this area, so it's going to be difficult to proceed with
modifications that alter behavior.


> These might be good questions to take to the IETF list...

Agreed. I made some informal inquiries to a few folks directly,
but asking publicly wouldn't hurt. It also wouldn't hurt to
ask implementors of other NFSv4 implementations.


>>> ---
>>> fs/nfsd/nfs3proc.c |    3 ++-
>>> fs/nfsd/nfs4proc.c |    2 +-
>>> fs/nfsd/nfsproc.c  |    3 ++-
>>> fs/nfsd/vfs.c      |   11 ++++++-----
>>> fs/nfsd/vfs.h      |    5 +++--
>>> 5 files changed, 14 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
>>> index 289eb844d086..5e369096e42f 100644
>>> --- a/fs/nfsd/nfs3proc.c
>>> +++ b/fs/nfsd/nfs3proc.c
>>> @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
>>> {
>>> 	struct nfsd3_symlinkargs *argp = rqstp->rq_argp;
>>> 	struct nfsd3_diropres *resp = rqstp->rq_resp;
>>> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
>>> 
>>> 	if (argp->tlen == 0) {
>>> 		resp->status = nfserr_inval;
>>> @@ -417,7 +418,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
>>> 	fh_copy(&resp->dirfh, &argp->ffh);
>>> 	fh_init(&resp->fh, NFS3_FHSIZE);
>>> 	resp->status = nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
>>> -				    argp->flen, argp->tname, &resp->fh);
>>> +				    argp->flen, argp->tname, &attrs, &resp->fh);
>>> 	kfree(argp->tname);
>>> out:
>>> 	return rpc_success;
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index ba750d76f515..ee72c94732f0 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -809,7 +809,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> 	case NF4LNK:
>>> 		status = nfsd_symlink(rqstp, &cstate->current_fh,
>>> 				      create->cr_name, create->cr_namelen,
>>> -				      create->cr_data, &resfh);
>>> +				      create->cr_data, &attrs, &resfh);
>>> 		break;
>>> 
>>> 	case NF4BLK:
>>> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
>>> index 594d6f85c89f..d09d516188d2 100644
>>> --- a/fs/nfsd/nfsproc.c
>>> +++ b/fs/nfsd/nfsproc.c
>>> @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
>>> {
>>> 	struct nfsd_symlinkargs *argp = rqstp->rq_argp;
>>> 	struct nfsd_stat *resp = rqstp->rq_resp;
>>> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
>>> 	struct svc_fh	newfh;
>>> 
>>> 	if (argp->tlen > NFS_MAXPATHLEN) {
>>> @@ -495,7 +496,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
>>> 
>>> 	fh_init(&newfh, NFS_FHSIZE);
>>> 	resp->status = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
>>> -				    argp->tname, &newfh);
>>> +				    argp->tname, &attrs, &newfh);
>>> 
>>> 	kfree(argp->tname);
>>> 	fh_put(&argp->ffh);
>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index a85dc4dd4f3a..91c9ea09f921 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -1451,9 +1451,9 @@ nfsd_readlink(struct svc_rqst *rqstp, struct svc_fh *fhp, char *buf, int *lenp)
>>> */
>>> __be32
>>> nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>> -				char *fname, int flen,
>>> -				char *path,
>>> -				struct svc_fh *resfhp)
>>> +	     char *fname, int flen,
>>> +	     char *path, struct nfsd_attrs *attrs,
>>> +	     struct svc_fh *resfhp)
>>> {
>>> 	struct dentry	*dentry, *dnew;
>>> 	__be32		err, cerr;
>>> @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>> 
>>> 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
>>> 	err = nfserrno(host_err);
>>> +	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
>>> +	if (!err)
>>> +		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
>>> 	fh_unlock(fhp);
>>> 	if (!err)
>>> 		err = nfserrno(commit_metadata(fhp));
>>> -
>>> 	fh_drop_write(fhp);
>>> 
>>> -	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
>>> 	dput(dnew);
>>> 	if (err==0) err = cerr;
>>> out:
>>> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
>>> index 9bb0e3957982..f3f43ca3ac6b 100644
>>> --- a/fs/nfsd/vfs.h
>>> +++ b/fs/nfsd/vfs.h
>>> @@ -114,8 +114,9 @@ __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>> __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
>>> 				char *, int *);
>>> __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
>>> -				char *name, int len, char *path,
>>> -				struct svc_fh *res);
>>> +			     char *name, int len, char *path,
>>> +			     struct nfsd_attrs *attrs,
>>> +			     struct svc_fh *res);
>>> __be32		nfsd_link(struct svc_rqst *, struct svc_fh *,
>>> 				char *, int, struct svc_fh *);
>>> ssize_t		nfsd_copy_file_range(struct file *, u64,
>>> 
>>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever

Re: [PATCH 04/13] NFSD: set attributes when creating symlinks

[NeilBrown]

On Tue, 26 Jul 2022, Chuck Lever III wrote:
> 
> > On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> > 
> > The NFS protocol includes attributes when creating symlinks.
> > Linux does store attributes for symlinks and allows them to be set,
> > though they are not used for permission checking.
> > 
> > NFSD currently doesn't set standard (struct iattr) attributes when
> > creating symlinks, but for NFSv4 it does set ACLs and security labels.
> > This is inconsistent.
> > 
> > To improve consistency, pass the provided attributes into nfsd_symlink()
> > and call nfsd_create_setattr() to set them.
> > 
> > We ignore any error from nfsd_create_setattr().  It isn't really clear
> > what should be done if a file is successfully created, but the
> > attributes cannot be set.  NFS doesn't allow partial success to be
> > reported.  Reporting failure is probably more misleading than reporting
> > success, so the status is ignored.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> 
> While I was trying to get more information about how
> security labels are supposed to be applied to symlinks,
> Jeff, Bruce, and I had a brief private discussion about
> it. Jeff has the impression that NFSD should not apply
> ACLs/security labels to symlinks, and we believe that
> would be somewhat of a change in behavior.
> 
> Now is a good time to hoist that private discussion to
> the list, so I'm mentioning it here. I'm thinking it
> would be appropriate to introduce that change in this
> series, and probably right here in this patch, if we
> agree that is the right thing to do going forward.

One option would be to avoid changing behaviour at this point.
That could be achieved by a one-line change in nfsd_symlink() in this
patch to clear the ia_valid field.  That way iattr attrs would still not
be set on symlinks, but ACLs/security labels would.
As and when we change our mind on what we *should* be doing, any such
change can then happen right there in nfsd_symlink() by making
modifications to the nfsd_attrs content, so it would be explicit in the
code. 

My feeling on policy is that it is our job to implement the NFSv4
protocol.  If the protocol sends attributes, and if the filesystem
allows them to be set, when who are we to stand in the way?
I appreciate that might be an overly-narrow perspective.

> 
> Otherwise, after a brief glance at the series, it looks
> better to me than the previous approach. I will try to
> dig in later this week so we can get this work merged at
> the end of the upcoming merge window.

Thanks,
NeilBrown

> 
> 
> > ---
> > fs/nfsd/nfs3proc.c |    3 ++-
> > fs/nfsd/nfs4proc.c |    2 +-
> > fs/nfsd/nfsproc.c  |    3 ++-
> > fs/nfsd/vfs.c      |   11 ++++++-----
> > fs/nfsd/vfs.h      |    5 +++--
> > 5 files changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > index 289eb844d086..5e369096e42f 100644
> > --- a/fs/nfsd/nfs3proc.c
> > +++ b/fs/nfsd/nfs3proc.c
> > @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> > {
> > 	struct nfsd3_symlinkargs *argp = rqstp->rq_argp;
> > 	struct nfsd3_diropres *resp = rqstp->rq_resp;
> > +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> > 
> > 	if (argp->tlen == 0) {
> > 		resp->status = nfserr_inval;
> > @@ -417,7 +418,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> > 	fh_copy(&resp->dirfh, &argp->ffh);
> > 	fh_init(&resp->fh, NFS3_FHSIZE);
> > 	resp->status = nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
> > -				    argp->flen, argp->tname, &resp->fh);
> > +				    argp->flen, argp->tname, &attrs, &resp->fh);
> > 	kfree(argp->tname);
> > out:
> > 	return rpc_success;
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index ba750d76f515..ee72c94732f0 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -809,7 +809,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > 	case NF4LNK:
> > 		status = nfsd_symlink(rqstp, &cstate->current_fh,
> > 				      create->cr_name, create->cr_namelen,
> > -				      create->cr_data, &resfh);
> > +				      create->cr_data, &attrs, &resfh);
> > 		break;
> > 
> > 	case NF4BLK:
> > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> > index 594d6f85c89f..d09d516188d2 100644
> > --- a/fs/nfsd/nfsproc.c
> > +++ b/fs/nfsd/nfsproc.c
> > @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> > {
> > 	struct nfsd_symlinkargs *argp = rqstp->rq_argp;
> > 	struct nfsd_stat *resp = rqstp->rq_resp;
> > +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> > 	struct svc_fh	newfh;
> > 
> > 	if (argp->tlen > NFS_MAXPATHLEN) {
> > @@ -495,7 +496,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> > 
> > 	fh_init(&newfh, NFS_FHSIZE);
> > 	resp->status = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
> > -				    argp->tname, &newfh);
> > +				    argp->tname, &attrs, &newfh);
> > 
> > 	kfree(argp->tname);
> > 	fh_put(&argp->ffh);
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index a85dc4dd4f3a..91c9ea09f921 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1451,9 +1451,9 @@ nfsd_readlink(struct svc_rqst *rqstp, struct svc_fh *fhp, char *buf, int *lenp)
> >  */
> > __be32
> > nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > -				char *fname, int flen,
> > -				char *path,
> > -				struct svc_fh *resfhp)
> > +	     char *fname, int flen,
> > +	     char *path, struct nfsd_attrs *attrs,
> > +	     struct svc_fh *resfhp)
> > {
> > 	struct dentry	*dentry, *dnew;
> > 	__be32		err, cerr;
> > @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 
> > 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> > 	err = nfserrno(host_err);
> > +	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> > +	if (!err)
> > +		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> > 	fh_unlock(fhp);
> > 	if (!err)
> > 		err = nfserrno(commit_metadata(fhp));
> > -
> > 	fh_drop_write(fhp);
> > 
> > -	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> > 	dput(dnew);
> > 	if (err==0) err = cerr;
> > out:
> > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> > index 9bb0e3957982..f3f43ca3ac6b 100644
> > --- a/fs/nfsd/vfs.h
> > +++ b/fs/nfsd/vfs.h
> > @@ -114,8 +114,9 @@ __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
> > 				char *, int *);
> > __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
> > -				char *name, int len, char *path,
> > -				struct svc_fh *res);
> > +			     char *name, int len, char *path,
> > +			     struct nfsd_attrs *attrs,
> > +			     struct svc_fh *res);
> > __be32		nfsd_link(struct svc_rqst *, struct svc_fh *,
> > 				char *, int, struct svc_fh *);
> > ssize_t		nfsd_copy_file_range(struct file *, u64,
> > 
> > 
> 
> --
> Chuck Lever
> 
> 
> 
> 

Re: [PATCH 04/13] NFSD: set attributes when creating symlinks

[Chuck Lever III]

> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> 
> The NFS protocol includes attributes when creating symlinks.
> Linux does store attributes for symlinks and allows them to be set,
> though they are not used for permission checking.
> 
> NFSD currently doesn't set standard (struct iattr) attributes when
> creating symlinks, but for NFSv4 it does set ACLs and security labels.
> This is inconsistent.
> 
> To improve consistency, pass the provided attributes into nfsd_symlink()
> and call nfsd_create_setattr() to set them.

This patch actually introduces a behavior change, if I'm reading
it correctly. NFSD will now permit an NFSv4 client to specify
attributes on creation, correct? I'm wondering if there will be
fallout for our test cases.

In any event, not an objection to this patch, but wanted the
behavior modification to be noted at least in the review comments.


> We ignore any error from nfsd_create_setattr().  It isn't really clear
> what should be done if a file is successfully created, but the
> attributes cannot be set.  NFS doesn't allow partial success to be
> reported.  Reporting failure is probably more misleading than reporting
> success, so the status is ignored.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs3proc.c |    3 ++-
> fs/nfsd/nfs4proc.c |    2 +-
> fs/nfsd/nfsproc.c  |    3 ++-
> fs/nfsd/vfs.c      |   11 ++++++-----
> fs/nfsd/vfs.h      |    5 +++--
> 5 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 289eb844d086..5e369096e42f 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> {
> 	struct nfsd3_symlinkargs *argp = rqstp->rq_argp;
> 	struct nfsd3_diropres *resp = rqstp->rq_resp;
> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> 
> 	if (argp->tlen == 0) {
> 		resp->status = nfserr_inval;
> @@ -417,7 +418,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> 	fh_copy(&resp->dirfh, &argp->ffh);
> 	fh_init(&resp->fh, NFS3_FHSIZE);
> 	resp->status = nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
> -				    argp->flen, argp->tname, &resp->fh);
> +				    argp->flen, argp->tname, &attrs, &resp->fh);
> 	kfree(argp->tname);
> out:
> 	return rpc_success;
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index ba750d76f515..ee72c94732f0 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -809,7 +809,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	case NF4LNK:
> 		status = nfsd_symlink(rqstp, &cstate->current_fh,
> 				      create->cr_name, create->cr_namelen,
> -				      create->cr_data, &resfh);
> +				      create->cr_data, &attrs, &resfh);
> 		break;
> 
> 	case NF4BLK:
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 594d6f85c89f..d09d516188d2 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> {
> 	struct nfsd_symlinkargs *argp = rqstp->rq_argp;
> 	struct nfsd_stat *resp = rqstp->rq_resp;
> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> 	struct svc_fh	newfh;
> 
> 	if (argp->tlen > NFS_MAXPATHLEN) {
> @@ -495,7 +496,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> 
> 	fh_init(&newfh, NFS_FHSIZE);
> 	resp->status = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
> -				    argp->tname, &newfh);
> +				    argp->tname, &attrs, &newfh);
> 
> 	kfree(argp->tname);
> 	fh_put(&argp->ffh);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index a85dc4dd4f3a..91c9ea09f921 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1451,9 +1451,9 @@ nfsd_readlink(struct svc_rqst *rqstp, struct svc_fh *fhp, char *buf, int *lenp)
>  */
> __be32
> nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -				char *fname, int flen,
> -				char *path,
> -				struct svc_fh *resfhp)
> +	     char *fname, int flen,
> +	     char *path, struct nfsd_attrs *attrs,
> +	     struct svc_fh *resfhp)

It would be nice if nfsd_symlink() had a kdoc comment like the one
that nfsd_create_setattr() has.


> {
> 	struct dentry	*dentry, *dnew;
> 	__be32		err, cerr;
> @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 
> 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> 	err = nfserrno(host_err);
> +	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> +	if (!err)
> +		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> 	fh_unlock(fhp);
> 	if (!err)
> 		err = nfserrno(commit_metadata(fhp));
> -
> 	fh_drop_write(fhp);
> 
> -	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> 	dput(dnew);
> 	if (err==0) err = cerr;
> out:
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 9bb0e3957982..f3f43ca3ac6b 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -114,8 +114,9 @@ __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
> 				char *, int *);
> __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
> -				char *name, int len, char *path,
> -				struct svc_fh *res);
> +			     char *name, int len, char *path,
> +			     struct nfsd_attrs *attrs,
> +			     struct svc_fh *res);
> __be32		nfsd_link(struct svc_rqst *, struct svc_fh *,
> 				char *, int, struct svc_fh *);
> ssize_t		nfsd_copy_file_range(struct file *, u64,
> 
> 

--
Chuck Lever

Re: [PATCH 04/13] NFSD: set attributes when creating symlinks

[NeilBrown]

On Fri, 29 Jul 2022, Chuck Lever III wrote:
> 
> > On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> > 
> > The NFS protocol includes attributes when creating symlinks.
> > Linux does store attributes for symlinks and allows them to be set,
> > though they are not used for permission checking.
> > 
> > NFSD currently doesn't set standard (struct iattr) attributes when
> > creating symlinks, but for NFSv4 it does set ACLs and security labels.
> > This is inconsistent.
> > 
> > To improve consistency, pass the provided attributes into nfsd_symlink()
> > and call nfsd_create_setattr() to set them.
> 
> This patch actually introduces a behavior change, if I'm reading
> it correctly. NFSD will now permit an NFSv4 client to specify
> attributes on creation, correct? I'm wondering if there will be
> fallout for our test cases.
> 
> In any event, not an objection to this patch, but wanted the
> behavior modification to be noted at least in the review comments.
> 

Yes, this patch changes behaviour for v2, v3, and v4.  Whatever
attributes are received from the client when creating a symlink are
passed on to the filesystem.
With the Linux client, the only attributes are
	attr.ia_mode = S_IFLNK | S_IRWXUGO;
	attr.ia_valid = ATTR_MODE;
so the final outcome will be unchanged.
Other clients might sent different attributes, and if they did they
probably expect them to be honoured.

As you say, making this explicit in the commit comment is appropriate.
Maybe:

  NOTE: this results in a behaviour change for all NFS versions when the
  client sends non-default attributes with a SYMLINK request.

Thanks,
NeilBrown


> 
> > We ignore any error from nfsd_create_setattr().  It isn't really clear
> > what should be done if a file is successfully created, but the
> > attributes cannot be set.  NFS doesn't allow partial success to be
> > reported.  Reporting failure is probably more misleading than reporting
> > success, so the status is ignored.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > fs/nfsd/nfs3proc.c |    3 ++-
> > fs/nfsd/nfs4proc.c |    2 +-
> > fs/nfsd/nfsproc.c  |    3 ++-
> > fs/nfsd/vfs.c      |   11 ++++++-----
> > fs/nfsd/vfs.h      |    5 +++--
> > 5 files changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > index 289eb844d086..5e369096e42f 100644
> > --- a/fs/nfsd/nfs3proc.c
> > +++ b/fs/nfsd/nfs3proc.c
> > @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> > {
> > 	struct nfsd3_symlinkargs *argp = rqstp->rq_argp;
> > 	struct nfsd3_diropres *resp = rqstp->rq_resp;
> > +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> > 
> > 	if (argp->tlen == 0) {
> > 		resp->status = nfserr_inval;
> > @@ -417,7 +418,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> > 	fh_copy(&resp->dirfh, &argp->ffh);
> > 	fh_init(&resp->fh, NFS3_FHSIZE);
> > 	resp->status = nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
> > -				    argp->flen, argp->tname, &resp->fh);
> > +				    argp->flen, argp->tname, &attrs, &resp->fh);
> > 	kfree(argp->tname);
> > out:
> > 	return rpc_success;
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index ba750d76f515..ee72c94732f0 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -809,7 +809,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > 	case NF4LNK:
> > 		status = nfsd_symlink(rqstp, &cstate->current_fh,
> > 				      create->cr_name, create->cr_namelen,
> > -				      create->cr_data, &resfh);
> > +				      create->cr_data, &attrs, &resfh);
> > 		break;
> > 
> > 	case NF4BLK:
> > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> > index 594d6f85c89f..d09d516188d2 100644
> > --- a/fs/nfsd/nfsproc.c
> > +++ b/fs/nfsd/nfsproc.c
> > @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> > {
> > 	struct nfsd_symlinkargs *argp = rqstp->rq_argp;
> > 	struct nfsd_stat *resp = rqstp->rq_resp;
> > +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> > 	struct svc_fh	newfh;
> > 
> > 	if (argp->tlen > NFS_MAXPATHLEN) {
> > @@ -495,7 +496,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> > 
> > 	fh_init(&newfh, NFS_FHSIZE);
> > 	resp->status = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
> > -				    argp->tname, &newfh);
> > +				    argp->tname, &attrs, &newfh);
> > 
> > 	kfree(argp->tname);
> > 	fh_put(&argp->ffh);
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index a85dc4dd4f3a..91c9ea09f921 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1451,9 +1451,9 @@ nfsd_readlink(struct svc_rqst *rqstp, struct svc_fh *fhp, char *buf, int *lenp)
> >  */
> > __be32
> > nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > -				char *fname, int flen,
> > -				char *path,
> > -				struct svc_fh *resfhp)
> > +	     char *fname, int flen,
> > +	     char *path, struct nfsd_attrs *attrs,
> > +	     struct svc_fh *resfhp)
> 
> It would be nice if nfsd_symlink() had a kdoc comment like the one
> that nfsd_create_setattr() has.
> 
> 
> > {
> > 	struct dentry	*dentry, *dnew;
> > 	__be32		err, cerr;
> > @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 
> > 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> > 	err = nfserrno(host_err);
> > +	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> > +	if (!err)
> > +		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> > 	fh_unlock(fhp);
> > 	if (!err)
> > 		err = nfserrno(commit_metadata(fhp));
> > -
> > 	fh_drop_write(fhp);
> > 
> > -	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> > 	dput(dnew);
> > 	if (err==0) err = cerr;
> > out:
> > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> > index 9bb0e3957982..f3f43ca3ac6b 100644
> > --- a/fs/nfsd/vfs.h
> > +++ b/fs/nfsd/vfs.h
> > @@ -114,8 +114,9 @@ __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
> > 				char *, int *);
> > __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
> > -				char *name, int len, char *path,
> > -				struct svc_fh *res);
> > +			     char *name, int len, char *path,
> > +			     struct nfsd_attrs *attrs,
> > +			     struct svc_fh *res);
> > __be32		nfsd_link(struct svc_rqst *, struct svc_fh *,
> > 				char *, int, struct svc_fh *);
> > ssize_t		nfsd_copy_file_range(struct file *, u64,
> > 
> > 
> 
> --
> Chuck Lever
> 
> 
> 
> 

[PATCH 11/13] NFSD: use explicit lock/unlock for directory ops

[NeilBrown]

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.

For the v2-only code in nfsproc.c, drop the fill calls as they aren't
needed.

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.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs3proc.c |    6 ++++--
 fs/nfsd/nfs4proc.c |    6 ++++--
 fs/nfsd/nfsproc.c  |    5 ++---
 fs/nfsd/vfs.c      |   30 +++++++++++++++++++-----------
 4 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 774e4a2ab9b1..c2f992b4387a 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -256,7 +256,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)) {
@@ -314,11 +314,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))
@@ -336,7 +338,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
 
 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 48e4efb39a9c..90af82d49119 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -264,7 +264,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (is_create_with_attrs(open))
 		nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs);
 
-	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)) {
@@ -348,10 +348,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))
@@ -373,7 +375,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (attrs.acl_failed)
 		open->op_bmval[0] &= ~FATTR4_WORD0_ACL;
 out:
-	fh_unlock(fhp);
+	inode_unlock(inode);
 	nfsd_attrs_free(&attrs);
 	if (child && !IS_ERR(child))
 		dput(child);
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index d09d516188d2..4cff332f58bb 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -287,7 +287,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));
@@ -403,8 +403,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 	}
 
 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 8ebad4a99552..f2cb9b047766 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1369,7 +1369,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)) {
@@ -1384,10 +1384,12 @@ 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, attrs, type,
 				 rdev, resfhp);
+	fh_fill_post_attrs(fhp);
 out_unlock:
-	fh_unlock(fhp);
+	inode_unlock(dentry->d_inode);
 	return err;
 }
 
@@ -1460,20 +1462,22 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		goto out;
 	}
 
-	fh_lock(fhp);
 	dentry = fhp->fh_dentry;
+	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
 	dnew = lookup_one_len(fname, dentry, flen);
 	if (IS_ERR(dnew)) {
 		err = nfserrno(PTR_ERR(dnew));
-		fh_unlock(fhp);
+		inode_unlock(dentry->d_inode);
 		goto out_drop_write;
 	}
+	fh_fill_pre_attrs(fhp);
 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
 	err = nfserrno(host_err);
 	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
 	if (!err)
 		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
-	fh_unlock(fhp);
+	fh_fill_post_attrs(fhp);
+	inode_unlock(dentry->d_inode);
 	if (!err)
 		err = nfserrno(commit_metadata(fhp));
 	dput(dnew);
@@ -1519,9 +1523,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);
 	if (IS_ERR(dnew)) {
@@ -1534,8 +1538,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)
@@ -1555,7 +1561,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;
 }
 
@@ -1730,9 +1736,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);
@@ -1750,6 +1756,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);
@@ -1757,8 +1764,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);
@@ -1781,7 +1789,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;
 }
 

Re: [PATCH 11/13] NFSD: use explicit lock/unlock for directory ops

[Chuck Lever III]

> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> 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.

IIUC, the antecedent of "This is already done" is only "explicit
calls to fh_fill_pre_attrs() and fh_fill_post_attrs()" ?

> 
> Also move the 'fill' calls closer to the operation that might change the
> attributes.  This way they are avoided on some error paths.
> 
> For the v2-only code in nfsproc.c, drop the fill calls as they aren't
> needed.

This feels like 3 independent changes to me. At least the v2 change
should be moved to a separate patch. Relocating the "fill attrs" calls
seems like it could cause noticeable behavior changes, so maybe it
belongs also in a separate patch?


> Having the locking explicit will simplify proposed future changes to

^Having^Making ?


> 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.
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs3proc.c |    6 ++++--
> fs/nfsd/nfs4proc.c |    6 ++++--
> fs/nfsd/nfsproc.c  |    5 ++---
> fs/nfsd/vfs.c      |   30 +++++++++++++++++++-----------
> 4 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 774e4a2ab9b1..c2f992b4387a 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -256,7 +256,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)) {
> @@ -314,11 +314,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))
> @@ -336,7 +338,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
> 
> 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 48e4efb39a9c..90af82d49119 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -264,7 +264,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	if (is_create_with_attrs(open))
> 		nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs);
> 
> -	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)) {
> @@ -348,10 +348,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))
> @@ -373,7 +375,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	if (attrs.acl_failed)
> 		open->op_bmval[0] &= ~FATTR4_WORD0_ACL;
> out:
> -	fh_unlock(fhp);
> +	inode_unlock(inode);
> 	nfsd_attrs_free(&attrs);
> 	if (child && !IS_ERR(child))
> 		dput(child);
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index d09d516188d2..4cff332f58bb 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -287,7 +287,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));
> @@ -403,8 +403,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> 	}
> 
> 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 8ebad4a99552..f2cb9b047766 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1369,7 +1369,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)) {
> @@ -1384,10 +1384,12 @@ 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, attrs, type,
> 				 rdev, resfhp);
> +	fh_fill_post_attrs(fhp);
> out_unlock:
> -	fh_unlock(fhp);
> +	inode_unlock(dentry->d_inode);
> 	return err;
> }
> 
> @@ -1460,20 +1462,22 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 		goto out;
> 	}
> 
> -	fh_lock(fhp);
> 	dentry = fhp->fh_dentry;
> +	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> 	dnew = lookup_one_len(fname, dentry, flen);
> 	if (IS_ERR(dnew)) {
> 		err = nfserrno(PTR_ERR(dnew));
> -		fh_unlock(fhp);
> +		inode_unlock(dentry->d_inode);
> 		goto out_drop_write;
> 	}
> +	fh_fill_pre_attrs(fhp);
> 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> 	err = nfserrno(host_err);
> 	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> 	if (!err)
> 		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> -	fh_unlock(fhp);
> +	fh_fill_post_attrs(fhp);
> +	inode_unlock(dentry->d_inode);
> 	if (!err)
> 		err = nfserrno(commit_metadata(fhp));
> 	dput(dnew);
> @@ -1519,9 +1523,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);
> 	if (IS_ERR(dnew)) {
> @@ -1534,8 +1538,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)
> @@ -1555,7 +1561,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;
> }
> 
> @@ -1730,9 +1736,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);
> @@ -1750,6 +1756,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);
> @@ -1757,8 +1764,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);
> @@ -1781,7 +1789,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;
> }
> 
> 
> 

--
Chuck Lever

Re: [PATCH 11/13] NFSD: use explicit lock/unlock for directory ops

[NeilBrown]

On Fri, 29 Jul 2022, Chuck Lever III wrote:
> 
> > On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> 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.
> 
> IIUC, the antecedent of "This is already done" is only "explicit
> calls to fh_fill_pre_attrs() and fh_fill_post_attrs()" ?

The "This" is both explicit locking and explicit fh_fill* calls.
In rename it isn't inode_lock_nested(), it is lock_rename()
Maybe
   This is already done for renames, with lock_rename() as the explicit
   locking.
??

> 
> > 
> > Also move the 'fill' calls closer to the operation that might change the
> > attributes.  This way they are avoided on some error paths.
> > 
> > For the v2-only code in nfsproc.c, drop the fill calls as they aren't
> > needed.
> 
> This feels like 3 independent changes to me. At least the v2 change
> should be moved to a separate patch. Relocating the "fill attrs" calls
> seems like it could cause noticeable behavior changes, so maybe it
> belongs also in a separate patch?

Three intimately related changes that could be applied in sequence.
What would be gained by having separate patches?
To my mind, the primary issue is review effort.  Mixing unrelated
changes make review of each change harder, so keep them separate.
Mixing related changes is less of a problem as the abstraction that you
need to keep front-of-mind are fewer.
On the flip side, every patch added increases the review burden.  If I
wanted to move all calls to foo(), I wouldn't have one patch for each
call.
I think that patch is easy to review as it stands, but if you think not
I guess it could be split.

Allowing bisect to isolate the problem precisely is another reason for
keeping patches small.  If a bug were found to be caused by this patch I
doubt it would be at all hard to localise which part of the patch caused
it.

> 
> 
> > Having the locking explicit will simplify proposed future changes to
> 
> ^Having^Making ?

Yes, that's probably a little clearer.

Thanks,
NeilBrown

> 
> 
> > 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.
> > 
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > fs/nfsd/nfs3proc.c |    6 ++++--
> > fs/nfsd/nfs4proc.c |    6 ++++--
> > fs/nfsd/nfsproc.c  |    5 ++---
> > fs/nfsd/vfs.c      |   30 +++++++++++++++++++-----------
> > 4 files changed, 29 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > index 774e4a2ab9b1..c2f992b4387a 100644
> > --- a/fs/nfsd/nfs3proc.c
> > +++ b/fs/nfsd/nfs3proc.c
> > @@ -256,7 +256,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)) {
> > @@ -314,11 +314,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))
> > @@ -336,7 +338,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
> > 
> > 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 48e4efb39a9c..90af82d49119 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -264,7 +264,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 	if (is_create_with_attrs(open))
> > 		nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs);
> > 
> > -	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)) {
> > @@ -348,10 +348,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))
> > @@ -373,7 +375,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 	if (attrs.acl_failed)
> > 		open->op_bmval[0] &= ~FATTR4_WORD0_ACL;
> > out:
> > -	fh_unlock(fhp);
> > +	inode_unlock(inode);
> > 	nfsd_attrs_free(&attrs);
> > 	if (child && !IS_ERR(child))
> > 		dput(child);
> > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> > index d09d516188d2..4cff332f58bb 100644
> > --- a/fs/nfsd/nfsproc.c
> > +++ b/fs/nfsd/nfsproc.c
> > @@ -287,7 +287,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));
> > @@ -403,8 +403,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> > 	}
> > 
> > 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 8ebad4a99552..f2cb9b047766 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1369,7 +1369,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)) {
> > @@ -1384,10 +1384,12 @@ 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, attrs, type,
> > 				 rdev, resfhp);
> > +	fh_fill_post_attrs(fhp);
> > out_unlock:
> > -	fh_unlock(fhp);
> > +	inode_unlock(dentry->d_inode);
> > 	return err;
> > }
> > 
> > @@ -1460,20 +1462,22 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 		goto out;
> > 	}
> > 
> > -	fh_lock(fhp);
> > 	dentry = fhp->fh_dentry;
> > +	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> > 	dnew = lookup_one_len(fname, dentry, flen);
> > 	if (IS_ERR(dnew)) {
> > 		err = nfserrno(PTR_ERR(dnew));
> > -		fh_unlock(fhp);
> > +		inode_unlock(dentry->d_inode);
> > 		goto out_drop_write;
> > 	}
> > +	fh_fill_pre_attrs(fhp);
> > 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> > 	err = nfserrno(host_err);
> > 	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> > 	if (!err)
> > 		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> > -	fh_unlock(fhp);
> > +	fh_fill_post_attrs(fhp);
> > +	inode_unlock(dentry->d_inode);
> > 	if (!err)
> > 		err = nfserrno(commit_metadata(fhp));
> > 	dput(dnew);
> > @@ -1519,9 +1523,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);
> > 	if (IS_ERR(dnew)) {
> > @@ -1534,8 +1538,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)
> > @@ -1555,7 +1561,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;
> > }
> > 
> > @@ -1730,9 +1736,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);
> > @@ -1750,6 +1756,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);
> > @@ -1757,8 +1764,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);
> > @@ -1781,7 +1789,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;
> > }
> > 
> > 
> > 
> 
> --
> Chuck Lever
> 
> 
> 
> 

Re: [PATCH 11/13] NFSD: use explicit lock/unlock for directory ops

[Chuck Lever III]

> On Jul 28, 2022, at 9:27 PM, NeilBrown <neilb@suse.de> wrote:
> 
> On Fri, 29 Jul 2022, Chuck Lever III wrote:
>> 
>>> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
>>> 
>>> Also move the 'fill' calls closer to the operation that might change the
>>> attributes.  This way they are avoided on some error paths.
>>> 
>>> For the v2-only code in nfsproc.c, drop the fill calls as they aren't
>>> needed.
>> 
>> This feels like 3 independent changes to me. At least the v2 change
>> should be moved to a separate patch. Relocating the "fill attrs" calls
>> seems like it could cause noticeable behavior changes, so maybe it
>> belongs also in a separate patch?
> 
> Three intimately related changes that could be applied in sequence.
> What would be gained by having separate patches?
> To my mind, the primary issue is review effort.  Mixing unrelated
> changes make review of each change harder, so keep them separate.
> Mixing related changes is less of a problem as the abstraction that you
> need to keep front-of-mind are fewer.
> On the flip side, every patch added increases the review burden.  If I
> wanted to move all calls to foo(), I wouldn't have one patch for each
> call.
> I think that patch is easy to review as it stands, but if you think not
> I guess it could be split.
> 
> Allowing bisect to isolate the problem precisely is another reason for
> keeping patches small.  If a bug were found to be caused by this patch I
> doubt it would be at all hard to localise which part of the patch caused
> it.

I adjusted the patch description and left the content as a single
patch. I was initially confused by "drop the fill calls"... the
patch is not physically removing code.

--
Chuck Lever

Re: [PATCH 11/13] NFSD: use explicit lock/unlock for directory ops

[Chuck Lever III]

> On Jul 29, 2022, at 10:31 AM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
>> On Jul 28, 2022, at 9:27 PM, NeilBrown <neilb@suse.de> wrote:
>> 
>> On Fri, 29 Jul 2022, Chuck Lever III wrote:
>>> 
>>>> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
>>>> 
>>>> Also move the 'fill' calls closer to the operation that might change the
>>>> attributes.  This way they are avoided on some error paths.
>>>> 
>>>> For the v2-only code in nfsproc.c, drop the fill calls as they aren't
>>>> needed.
>>> 
>>> This feels like 3 independent changes to me. At least the v2 change
>>> should be moved to a separate patch. Relocating the "fill attrs" calls
>>> seems like it could cause noticeable behavior changes, so maybe it
>>> belongs also in a separate patch?
>> 
>> Three intimately related changes that could be applied in sequence.
>> What would be gained by having separate patches?
>> To my mind, the primary issue is review effort.  Mixing unrelated
>> changes make review of each change harder, so keep them separate.
>> Mixing related changes is less of a problem as the abstraction that you
>> need to keep front-of-mind are fewer.
>> On the flip side, every patch added increases the review burden.  If I
>> wanted to move all calls to foo(), I wouldn't have one patch for each
>> call.
>> I think that patch is easy to review as it stands, but if you think not
>> I guess it could be split.
>> 
>> Allowing bisect to isolate the problem precisely is another reason for
>> keeping patches small.  If a bug were found to be caused by this patch I
>> doubt it would be at all hard to localise which part of the patch caused
>> it.
> 
> I adjusted the patch description and left the content as a single
> patch. I was initially confused by "drop the fill calls"... the
> patch is not physically removing code.

I should say that the issue for me was bisectability, it was easy
enough to review the patch.


--
Chuck Lever

[PATCH 02/13] NFSD: verify the opened dentry after setting a delegation

[NeilBrown]

From: Jeff Layton <jlayton@kernel.org>

Between opening a file and setting a delegation on it, someone could
rename or unlink the dentry. If this happens, we do not want to grant a
delegation on the open.

On a CLAIM_NULL open, we're opening by filename, and we may (in the
non-create case) or may not (in the create case) be holding i_rwsem
when attempting to set a delegation.  The latter case allows a
race.

After getting a lease, redo the lookup of the file being opened and
validate that the resulting dentry matches the one in the open file
description.

To properly redo the lookup we need an rqst pointer to pass to
nfsd_lookup_dentry(), so make sure that is available.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4proc.c  |    1 +
 fs/nfsd/nfs4state.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++-----
 fs/nfsd/xdr4.h      |    1 +
 3 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 5af9f8d1feb6..d57f91fa9f70 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -547,6 +547,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		open->op_openowner);
 
 	open->op_filp = NULL;
+	open->op_rqstp = rqstp;
 
 	/* This check required by spec. */
 	if (open->op_create && open->op_claim_type != NFS4_OPEN_CLAIM_NULL)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 279c7a1502c9..8f64af3e38d8 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5288,11 +5288,44 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
 	return 0;
 }
 
+/*
+ * It's possible that between opening the dentry and setting the delegation,
+ * that it has been renamed or unlinked. Redo the lookup to verify that this
+ * hasn't happened.
+ */
+static int
+nfsd4_verify_deleg_dentry(struct nfsd4_open *open, struct nfs4_file *fp,
+			  struct svc_fh *parent)
+{
+	struct svc_export *exp;
+	struct dentry *child;
+	__be32 err;
+
+	/* parent may already be locked, and it may get unlocked by
+	 * this call, but that is safe.
+	 */
+	err = nfsd_lookup_dentry(open->op_rqstp, parent,
+				 open->op_fname, open->op_fnamelen,
+				 &exp, &child);
+
+	if (err)
+		return -EAGAIN;
+
+	dput(child);
+	if (child != file_dentry(fp->fi_deleg_file->nf_file))
+		return -EAGAIN;
+
+	return 0;
+}
+
 static struct nfs4_delegation *
-nfs4_set_delegation(struct nfs4_client *clp,
-		    struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate)
+nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
+		    struct svc_fh *parent)
 {
 	int status = 0;
+	struct nfs4_client *clp = stp->st_stid.sc_client;
+	struct nfs4_file *fp = stp->st_stid.sc_file;
+	struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate;
 	struct nfs4_delegation *dp;
 	struct nfsd_file *nf;
 	struct file_lock *fl;
@@ -5347,6 +5380,13 @@ nfs4_set_delegation(struct nfs4_client *clp,
 		locks_free_lock(fl);
 	if (status)
 		goto out_clnt_odstate;
+
+	if (parent) {
+		status = nfsd4_verify_deleg_dentry(open, fp, parent);
+		if (status)
+			goto out_unlock;
+	}
+
 	status = nfsd4_check_conflicting_opens(clp, fp);
 	if (status)
 		goto out_unlock;
@@ -5402,11 +5442,13 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
  * proper support for them.
  */
 static void
-nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
+nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
+		     struct svc_fh *currentfh)
 {
 	struct nfs4_delegation *dp;
 	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
 	struct nfs4_client *clp = stp->st_stid.sc_client;
+	struct svc_fh *parent = NULL;
 	int cb_up;
 	int status = 0;
 
@@ -5420,6 +5462,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
 				goto out_no_deleg;
 			break;
 		case NFS4_OPEN_CLAIM_NULL:
+			parent = currentfh;
+			fallthrough;
 		case NFS4_OPEN_CLAIM_FH:
 			/*
 			 * Let's not give out any delegations till everyone's
@@ -5434,7 +5478,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
 		default:
 			goto out_no_deleg;
 	}
-	dp = nfs4_set_delegation(clp, stp->st_stid.sc_file, stp->st_clnt_odstate);
+	dp = nfs4_set_delegation(open, stp, parent);
 	if (IS_ERR(dp))
 		goto out_no_deleg;
 
@@ -5566,7 +5610,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	* Attempt to hand out a delegation. No error return, because the
 	* OPEN succeeds even if we fail.
 	*/
-	nfs4_open_delegation(open, stp);
+	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
 nodeleg:
 	status = nfs_ok;
 	trace_nfsd_open(&stp->st_stid.sc_stateid);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 7b744011f2d3..189f0600dbe8 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -279,6 +279,7 @@ struct nfsd4_open {
 	struct nfs4_clnt_odstate *op_odstate; /* used during processing */
 	struct nfs4_acl *op_acl;
 	struct xdr_netobj op_label;
+	struct svc_rqst *op_rqstp;
 };
 
 struct nfsd4_open_confirm {

Re: [PATCH 02/13] NFSD: verify the opened dentry after setting a delegation

[Jeff Layton]

On Tue, 2022-07-26 at 16:45 +1000, NeilBrown wrote:
> From: Jeff Layton <jlayton@kernel.org>
> 
> Between opening a file and setting a delegation on it, someone could
> rename or unlink the dentry. If this happens, we do not want to grant a
> delegation on the open.
> 
> On a CLAIM_NULL open, we're opening by filename, and we may (in the
> non-create case) or may not (in the create case) be holding i_rwsem
> when attempting to set a delegation.  The latter case allows a
> race.
> 
> After getting a lease, redo the lookup of the file being opened and
> validate that the resulting dentry matches the one in the open file
> description.
> 
> To properly redo the lookup we need an rqst pointer to pass to
> nfsd_lookup_dentry(), so make sure that is available.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4proc.c  |    1 +
>  fs/nfsd/nfs4state.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  fs/nfsd/xdr4.h      |    1 +
>  3 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 5af9f8d1feb6..d57f91fa9f70 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -547,6 +547,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		open->op_openowner);
>  
>  	open->op_filp = NULL;
> +	open->op_rqstp = rqstp;
>  
>  	/* This check required by spec. */
>  	if (open->op_create && open->op_claim_type != NFS4_OPEN_CLAIM_NULL)
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 279c7a1502c9..8f64af3e38d8 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5288,11 +5288,44 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
>  	return 0;
>  }
>  
> +/*
> + * It's possible that between opening the dentry and setting the delegation,
> + * that it has been renamed or unlinked. Redo the lookup to verify that this
> + * hasn't happened.
> + */
> +static int
> +nfsd4_verify_deleg_dentry(struct nfsd4_open *open, struct nfs4_file *fp,
> +			  struct svc_fh *parent)
> +{
> +	struct svc_export *exp;
> +	struct dentry *child;
> +	__be32 err;
> +
> +	/* parent may already be locked, and it may get unlocked by
> +	 * this call, but that is safe.
> +	 */
> +	err = nfsd_lookup_dentry(open->op_rqstp, parent,
> +				 open->op_fname, open->op_fnamelen,
> +				 &exp, &child);
> +

Note that in the middle of this series, you may end up triggering the
printk in fh_lock_nested if you call nfsd_lookup_dentry and the fh is
already locked. I assume that gets resolved by the end of the series
however.

> +	if (err)
> +		return -EAGAIN;
> +
> +	dput(child);
> +	if (child != file_dentry(fp->fi_deleg_file->nf_file))
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
>  static struct nfs4_delegation *
> -nfs4_set_delegation(struct nfs4_client *clp,
> -		    struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate)
> +nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> +		    struct svc_fh *parent)
>  {
>  	int status = 0;
> +	struct nfs4_client *clp = stp->st_stid.sc_client;
> +	struct nfs4_file *fp = stp->st_stid.sc_file;
> +	struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate;
>  	struct nfs4_delegation *dp;
>  	struct nfsd_file *nf;
>  	struct file_lock *fl;
> @@ -5347,6 +5380,13 @@ nfs4_set_delegation(struct nfs4_client *clp,
>  		locks_free_lock(fl);
>  	if (status)
>  		goto out_clnt_odstate;
> +
> +	if (parent) {
> +		status = nfsd4_verify_deleg_dentry(open, fp, parent);
> +		if (status)
> +			goto out_unlock;
> +	}
> +
>  	status = nfsd4_check_conflicting_opens(clp, fp);
>  	if (status)
>  		goto out_unlock;
> @@ -5402,11 +5442,13 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
>   * proper support for them.
>   */
>  static void
> -nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
> +nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> +		     struct svc_fh *currentfh)
>  {
>  	struct nfs4_delegation *dp;
>  	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
>  	struct nfs4_client *clp = stp->st_stid.sc_client;
> +	struct svc_fh *parent = NULL;
>  	int cb_up;
>  	int status = 0;
>  
> @@ -5420,6 +5462,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
>  				goto out_no_deleg;
>  			break;
>  		case NFS4_OPEN_CLAIM_NULL:
> +			parent = currentfh;
> +			fallthrough;
>  		case NFS4_OPEN_CLAIM_FH:
>  			/*
>  			 * Let's not give out any delegations till everyone's
> @@ -5434,7 +5478,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp)
>  		default:
>  			goto out_no_deleg;
>  	}
> -	dp = nfs4_set_delegation(clp, stp->st_stid.sc_file, stp->st_clnt_odstate);
> +	dp = nfs4_set_delegation(open, stp, parent);
>  	if (IS_ERR(dp))
>  		goto out_no_deleg;
>  
> @@ -5566,7 +5610,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>  	* Attempt to hand out a delegation. No error return, because the
>  	* OPEN succeeds even if we fail.
>  	*/
> -	nfs4_open_delegation(open, stp);
> +	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
>  nodeleg:
>  	status = nfs_ok;
>  	trace_nfsd_open(&stp->st_stid.sc_stateid);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 7b744011f2d3..189f0600dbe8 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -279,6 +279,7 @@ struct nfsd4_open {
>  	struct nfs4_clnt_odstate *op_odstate; /* used during processing */
>  	struct nfs4_acl *op_acl;
>  	struct xdr_netobj op_label;
> +	struct svc_rqst *op_rqstp;
>  };
>  
>  struct nfsd4_open_confirm {
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 09/13] NFSD: only call fh_unlock() once in nfsd_link()

[NeilBrown]

On non-error paths, nfsd_link() calls fh_unlock() twice.  This is safe
because fh_unlock() records that the unlock has been done and doesn't
repeat it.
However it makes the code a little confusing and interferes with changes
that are planned for directory locking.

So rearrange the code to ensure fh_unlock() is called exactly once if
fh_lock() was called.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/vfs.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index f15ceed6d184..06b1408db08b 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1530,9 +1530,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 	dirp = d_inode(ddir);
 
 	dnew = lookup_one_len(name, ddir, len);
-	host_err = PTR_ERR(dnew);
-	if (IS_ERR(dnew))
-		goto out_nfserr;
+	if (IS_ERR(dnew)) {
+		err = nfserrno(PTR_ERR(dnew));
+		goto out_unlock;
+	}
 
 	dold = tfhp->fh_dentry;
 
@@ -1551,17 +1552,17 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 		else
 			err = nfserrno(host_err);
 	}
-out_dput:
 	dput(dnew);
-out_unlock:
-	fh_unlock(ffhp);
+out_drop_write:
 	fh_drop_write(tfhp);
 out:
 	return err;
 
-out_nfserr:
-	err = nfserrno(host_err);
-	goto out_unlock;
+out_dput:
+	dput(dnew);
+out_unlock:
+	fh_unlock(ffhp);
+	goto out_drop_write;
 }
 
 static void

[PATCH 06/13] NFSD: add posix ACLs to struct nfsd_attrs

[NeilBrown]

pacl and dpacl pointers are added to struct nfsd_attrs, which requires
that we have an nfsd_attrs_free() function to free them.
Those nfsv4 functions that can set ACLs now set up these pointers
based on the passed in NFSv4 ACL.

nfsd_setattr() sets the acls as appropriate.

Errors are handled as with security labels.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/acl.h      |    6 ++++--
 fs/nfsd/nfs4acl.c  |   46 +++++++---------------------------------------
 fs/nfsd/nfs4proc.c |   46 ++++++++++++++++------------------------------
 fs/nfsd/vfs.c      |    8 ++++++++
 fs/nfsd/vfs.h      |   10 ++++++++++
 5 files changed, 45 insertions(+), 71 deletions(-)

diff --git a/fs/nfsd/acl.h b/fs/nfsd/acl.h
index ba14d2f4b64f..4b7324458a94 100644
--- a/fs/nfsd/acl.h
+++ b/fs/nfsd/acl.h
@@ -38,6 +38,8 @@
 struct nfs4_acl;
 struct svc_fh;
 struct svc_rqst;
+struct nfsd_attrs;
+enum nfs_ftype4;
 
 int nfs4_acl_bytes(int entries);
 int nfs4_acl_get_whotype(char *, u32);
@@ -45,7 +47,7 @@ __be32 nfs4_acl_write_who(struct xdr_stream *xdr, int who);
 
 int nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry,
 		struct nfs4_acl **acl);
-__be32 nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		struct nfs4_acl *acl);
+__be32 nfsd4_acl_to_attr(enum nfs_ftype4 type, struct nfs4_acl *acl,
+			 struct nfsd_attrs *attr);
 
 #endif /* LINUX_NFS4_ACL_H */
diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
index eaa3a0cf38f1..8a5b847b6c73 100644
--- a/fs/nfsd/nfs4acl.c
+++ b/fs/nfsd/nfs4acl.c
@@ -751,58 +751,26 @@ static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl,
 	return ret;
 }
 
-__be32
-nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		struct nfs4_acl *acl)
+__be32 nfsd4_acl_to_attr(enum nfs_ftype4 type, struct nfs4_acl *acl,
+			 struct nfsd_attrs *attr)
 {
-	__be32 error;
 	int host_error;
-	struct dentry *dentry;
-	struct inode *inode;
-	struct posix_acl *pacl = NULL, *dpacl = NULL;
 	unsigned int flags = 0;
 
-	/* Get inode */
-	error = fh_verify(rqstp, fhp, 0, NFSD_MAY_SATTR);
-	if (error)
-		return error;
-
-	dentry = fhp->fh_dentry;
-	inode = d_inode(dentry);
+	if (!acl)
+		return nfs_ok;
 
-	if (S_ISDIR(inode->i_mode))
+	if (type == NF4DIR)
 		flags = NFS4_ACL_DIR;
 
-	host_error = nfs4_acl_nfsv4_to_posix(acl, &pacl, &dpacl, flags);
+	host_error = nfs4_acl_nfsv4_to_posix(acl, &attr->pacl, &attr->dpacl,
+					     flags);
 	if (host_error == -EINVAL)
 		return nfserr_attrnotsupp;
-	if (host_error < 0)
-		goto out_nfserr;
-
-	fh_lock(fhp);
-
-	host_error = set_posix_acl(&init_user_ns, inode, ACL_TYPE_ACCESS, pacl);
-	if (host_error < 0)
-		goto out_drop_lock;
-
-	if (S_ISDIR(inode->i_mode)) {
-		host_error = set_posix_acl(&init_user_ns, inode,
-					   ACL_TYPE_DEFAULT, dpacl);
-	}
-
-out_drop_lock:
-	fh_unlock(fhp);
-
-	posix_acl_release(pacl);
-	posix_acl_release(dpacl);
-out_nfserr:
-	if (host_error == -EOPNOTSUPP)
-		return nfserr_attrnotsupp;
 	else
 		return nfserrno(host_error);
 }
 
-
 static short
 ace2type(struct nfs4_ace *ace)
 {
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 83d2b645b0d6..c49cd04cb567 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -128,26 +128,6 @@ is_create_with_attrs(struct nfsd4_open *open)
 		    || open->op_createmode == NFS4_CREATE_EXCLUSIVE4_1);
 }
 
-/*
- * if error occurs when setting the acl, just clear the acl bit
- * in the returned attr bitmap.
- */
-static void
-do_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		struct nfs4_acl *acl, u32 *bmval)
-{
-	__be32 status;
-
-	status = nfsd4_set_nfs4_acl(rqstp, fhp, acl);
-	if (status)
-		/*
-		 * We should probably fail the whole open at this point,
-		 * but we've already created the file, so it's too late;
-		 * So this seems the least of evils:
-		 */
-		bmval[0] &= ~FATTR4_WORD0_ACL;
-}
-
 static inline void
 fh_dup2(struct svc_fh *dst, struct svc_fh *src)
 {
@@ -281,6 +261,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (host_err)
 		return nfserrno(host_err);
 
+	if (is_create_with_attrs(open))
+		nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs);
+
 	fh_lock_nested(fhp, I_MUTEX_PARENT);
 
 	child = lookup_one_len(open->op_fname, parent, open->op_fnamelen);
@@ -382,8 +365,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	if (attrs.label_failed)
 		open->op_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
+	if (attrs.acl_failed)
+		open->op_bmval[0] &= ~FATTR4_WORD0_ACL;
 out:
 	fh_unlock(fhp);
+	nfsd_attrs_free(&attrs);
 	if (child && !IS_ERR(child))
 		dput(child);
 	fh_drop_write(fhp);
@@ -446,9 +432,6 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	if (status)
 		goto out;
 
-	if (is_create_with_attrs(open) && open->op_acl != NULL)
-		do_set_nfs4_acl(rqstp, *resfh, open->op_acl, open->op_bmval);
-
 	nfsd4_set_open_owner_reply_cache(cstate, open, *resfh);
 	accmode = NFSD_MAY_NOP;
 	if (open->op_created ||
@@ -779,6 +762,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		return status;
 
+	status = nfsd4_acl_to_attr(create->cr_type, create->cr_acl, &attrs);
 	current->fs->umask = create->cr_umask;
 	switch (create->cr_type) {
 	case NF4LNK:
@@ -837,10 +821,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	if (attrs.label_failed)
 		create->cr_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
-
-	if (create->cr_acl != NULL)
-		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
-				create->cr_bmval);
+	if (attrs.label_failed)
+		create->cr_bmval[0] &= ~FATTR4_WORD0_ACL;
 
 	fh_unlock(&cstate->current_fh);
 	set_change_info(&create->cr_cinfo, &cstate->current_fh);
@@ -849,6 +831,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	fh_put(&resfh);
 out_umask:
 	current->fs->umask = 0;
+	nfsd_attrs_free(&attrs);
 	return status;
 }
 
@@ -1123,6 +1106,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		.iattr = &setattr->sa_iattr,
 		.label = &setattr->sa_label,
 	};
+	struct inode *inode;
 	__be32 status = nfs_ok;
 	int err;
 
@@ -1145,9 +1129,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		goto out;
 
-	if (setattr->sa_acl != NULL)
-		status = nfsd4_set_nfs4_acl(rqstp, &cstate->current_fh,
-					    setattr->sa_acl);
+	inode = cstate->current_fh.fh_dentry->d_inode;
+	status = nfsd4_acl_to_attr(S_ISDIR(inode->i_mode) ? NF4DIR : NF4REG,
+				   setattr->sa_acl, &attrs);
+
 	if (status)
 		goto out;
 	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
@@ -1155,6 +1140,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (!status)
 		status = attrs.label_failed;
 out:
+	nfsd_attrs_free(&attrs);
 	fh_drop_write(&cstate->current_fh);
 	return status;
 }
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index e7a18bedf499..4bb05586a258 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -461,6 +461,14 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (attr->label && attr->label->len)
 		attr->label_failed = security_inode_setsecctx(
 			dentry, attr->label->data, attr->label->len);
+	if (attr->pacl)
+		attr->acl_failed = set_posix_acl(&init_user_ns,
+						 inode, ACL_TYPE_ACCESS,
+						 attr->pacl);
+	if (!attr->acl_failed && attr->dpacl && S_ISDIR(inode->i_mode))
+		attr->acl_failed = set_posix_acl(&init_user_ns,
+						 inode, ACL_TYPE_DEFAULT,
+						 attr->dpacl);
 	fh_unlock(fhp);
 	if (size_change)
 		put_write_access(inode);
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 8464e04af1ea..9343aac0bd15 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -6,6 +6,8 @@
 #ifndef LINUX_NFSD_VFS_H
 #define LINUX_NFSD_VFS_H
 
+#include <linux/fs.h>
+#include <linux/posix_acl.h>
 #include "nfsfh.h"
 #include "nfsd.h"
 
@@ -45,9 +47,17 @@ typedef int (*nfsd_filldir_t)(void *, const char *, int, loff_t, u64, unsigned);
 struct nfsd_attrs {
 	struct iattr *iattr;
 	struct xdr_netobj *label;
+	struct posix_acl *pacl, *dpacl;
 	int label_failed;
+	int acl_failed;
 };
 
+static inline void nfsd_attrs_free(struct nfsd_attrs *attrs)
+{
+	posix_acl_release(attrs->pacl);
+	posix_acl_release(attrs->dpacl);
+}
+
 int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 		                struct svc_export **expp);
 __be32		nfsd_lookup(struct svc_rqst *, struct svc_fh *,

Re: [PATCH 06/13] NFSD: add posix ACLs to struct nfsd_attrs

[Chuck Lever III]

> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> 
> pacl and dpacl pointers are added to struct nfsd_attrs, which requires
> that we have an nfsd_attrs_free() function to free them.
> Those nfsv4 functions that can set ACLs now set up these pointers
> based on the passed in NFSv4 ACL.
> 
> nfsd_setattr() sets the acls as appropriate.
> 
> Errors are handled as with security labels.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/acl.h      |    6 ++++--
> fs/nfsd/nfs4acl.c  |   46 +++++++---------------------------------------
> fs/nfsd/nfs4proc.c |   46 ++++++++++++++++------------------------------
> fs/nfsd/vfs.c      |    8 ++++++++
> fs/nfsd/vfs.h      |   10 ++++++++++
> 5 files changed, 45 insertions(+), 71 deletions(-)
> 
> diff --git a/fs/nfsd/acl.h b/fs/nfsd/acl.h
> index ba14d2f4b64f..4b7324458a94 100644
> --- a/fs/nfsd/acl.h
> +++ b/fs/nfsd/acl.h
> @@ -38,6 +38,8 @@
> struct nfs4_acl;
> struct svc_fh;
> struct svc_rqst;
> +struct nfsd_attrs;
> +enum nfs_ftype4;
> 
> int nfs4_acl_bytes(int entries);
> int nfs4_acl_get_whotype(char *, u32);
> @@ -45,7 +47,7 @@ __be32 nfs4_acl_write_who(struct xdr_stream *xdr, int who);
> 
> int nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry,
> 		struct nfs4_acl **acl);
> -__be32 nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct nfs4_acl *acl);
> +__be32 nfsd4_acl_to_attr(enum nfs_ftype4 type, struct nfs4_acl *acl,
> +			 struct nfsd_attrs *attr);
> 
> #endif /* LINUX_NFS4_ACL_H */
> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> index eaa3a0cf38f1..8a5b847b6c73 100644
> --- a/fs/nfsd/nfs4acl.c
> +++ b/fs/nfsd/nfs4acl.c
> @@ -751,58 +751,26 @@ static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl,
> 	return ret;
> }
> 
> -__be32
> -nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct nfs4_acl *acl)
> +__be32 nfsd4_acl_to_attr(enum nfs_ftype4 type, struct nfs4_acl *acl,
> +			 struct nfsd_attrs *attr)
> {
> -	__be32 error;
> 	int host_error;
> -	struct dentry *dentry;
> -	struct inode *inode;
> -	struct posix_acl *pacl = NULL, *dpacl = NULL;
> 	unsigned int flags = 0;
> 
> -	/* Get inode */
> -	error = fh_verify(rqstp, fhp, 0, NFSD_MAY_SATTR);
> -	if (error)
> -		return error;
> -
> -	dentry = fhp->fh_dentry;
> -	inode = d_inode(dentry);
> +	if (!acl)
> +		return nfs_ok;
> 
> -	if (S_ISDIR(inode->i_mode))
> +	if (type == NF4DIR)
> 		flags = NFS4_ACL_DIR;
> 
> -	host_error = nfs4_acl_nfsv4_to_posix(acl, &pacl, &dpacl, flags);
> +	host_error = nfs4_acl_nfsv4_to_posix(acl, &attr->pacl, &attr->dpacl,
> +					     flags);
> 	if (host_error == -EINVAL)
> 		return nfserr_attrnotsupp;
> -	if (host_error < 0)
> -		goto out_nfserr;
> -
> -	fh_lock(fhp);
> -
> -	host_error = set_posix_acl(&init_user_ns, inode, ACL_TYPE_ACCESS, pacl);
> -	if (host_error < 0)
> -		goto out_drop_lock;
> -
> -	if (S_ISDIR(inode->i_mode)) {
> -		host_error = set_posix_acl(&init_user_ns, inode,
> -					   ACL_TYPE_DEFAULT, dpacl);
> -	}
> -
> -out_drop_lock:
> -	fh_unlock(fhp);
> -
> -	posix_acl_release(pacl);
> -	posix_acl_release(dpacl);
> -out_nfserr:
> -	if (host_error == -EOPNOTSUPP)
> -		return nfserr_attrnotsupp;
> 	else
> 		return nfserrno(host_error);
> }
> 
> -
> static short
> ace2type(struct nfs4_ace *ace)
> {
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 83d2b645b0d6..c49cd04cb567 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -128,26 +128,6 @@ is_create_with_attrs(struct nfsd4_open *open)
> 		    || open->op_createmode == NFS4_CREATE_EXCLUSIVE4_1);
> }
> 
> -/*
> - * if error occurs when setting the acl, just clear the acl bit
> - * in the returned attr bitmap.
> - */
> -static void
> -do_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct nfs4_acl *acl, u32 *bmval)
> -{
> -	__be32 status;
> -
> -	status = nfsd4_set_nfs4_acl(rqstp, fhp, acl);
> -	if (status)
> -		/*
> -		 * We should probably fail the whole open at this point,
> -		 * but we've already created the file, so it's too late;
> -		 * So this seems the least of evils:
> -		 */
> -		bmval[0] &= ~FATTR4_WORD0_ACL;
> -}
> -
> static inline void
> fh_dup2(struct svc_fh *dst, struct svc_fh *src)
> {
> @@ -281,6 +261,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	if (host_err)
> 		return nfserrno(host_err);
> 
> +	if (is_create_with_attrs(open))
> +		nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs);
> +
> 	fh_lock_nested(fhp, I_MUTEX_PARENT);
> 
> 	child = lookup_one_len(open->op_fname, parent, open->op_fnamelen);
> @@ -382,8 +365,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 
> 	if (attrs.label_failed)
> 		open->op_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> +	if (attrs.acl_failed)
> +		open->op_bmval[0] &= ~FATTR4_WORD0_ACL;
> out:
> 	fh_unlock(fhp);
> +	nfsd_attrs_free(&attrs);
> 	if (child && !IS_ERR(child))
> 		dput(child);
> 	fh_drop_write(fhp);
> @@ -446,9 +432,6 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> 	if (status)
> 		goto out;
> 
> -	if (is_create_with_attrs(open) && open->op_acl != NULL)
> -		do_set_nfs4_acl(rqstp, *resfh, open->op_acl, open->op_bmval);
> -
> 	nfsd4_set_open_owner_reply_cache(cstate, open, *resfh);
> 	accmode = NFSD_MAY_NOP;
> 	if (open->op_created ||
> @@ -779,6 +762,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	if (status)
> 		return status;
> 
> +	status = nfsd4_acl_to_attr(create->cr_type, create->cr_acl, &attrs);
> 	current->fs->umask = create->cr_umask;
> 	switch (create->cr_type) {
> 	case NF4LNK:
> @@ -837,10 +821,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 
> 	if (attrs.label_failed)
> 		create->cr_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> -
> -	if (create->cr_acl != NULL)
> -		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
> -				create->cr_bmval);
> +	if (attrs.label_failed)
> +		create->cr_bmval[0] &= ~FATTR4_WORD0_ACL;

I think this needs to be "if (attrs.acl_failed)". I've fixed this
in my tree.


> 	fh_unlock(&cstate->current_fh);
> 	set_change_info(&create->cr_cinfo, &cstate->current_fh);
> @@ -849,6 +831,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	fh_put(&resfh);
> out_umask:
> 	current->fs->umask = 0;
> +	nfsd_attrs_free(&attrs);
> 	return status;
> }
> 
> @@ -1123,6 +1106,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 		.iattr = &setattr->sa_iattr,
> 		.label = &setattr->sa_label,
> 	};
> +	struct inode *inode;
> 	__be32 status = nfs_ok;
> 	int err;
> 
> @@ -1145,9 +1129,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	if (status)
> 		goto out;
> 
> -	if (setattr->sa_acl != NULL)
> -		status = nfsd4_set_nfs4_acl(rqstp, &cstate->current_fh,
> -					    setattr->sa_acl);
> +	inode = cstate->current_fh.fh_dentry->d_inode;
> +	status = nfsd4_acl_to_attr(S_ISDIR(inode->i_mode) ? NF4DIR : NF4REG,
> +				   setattr->sa_acl, &attrs);
> +
> 	if (status)
> 		goto out;
> 	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> @@ -1155,6 +1140,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	if (!status)
> 		status = attrs.label_failed;
> out:
> +	nfsd_attrs_free(&attrs);
> 	fh_drop_write(&cstate->current_fh);
> 	return status;
> }
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index e7a18bedf499..4bb05586a258 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -461,6 +461,14 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	if (attr->label && attr->label->len)
> 		attr->label_failed = security_inode_setsecctx(
> 			dentry, attr->label->data, attr->label->len);
> +	if (attr->pacl)
> +		attr->acl_failed = set_posix_acl(&init_user_ns,
> +						 inode, ACL_TYPE_ACCESS,
> +						 attr->pacl);
> +	if (!attr->acl_failed && attr->dpacl && S_ISDIR(inode->i_mode))
> +		attr->acl_failed = set_posix_acl(&init_user_ns,
> +						 inode, ACL_TYPE_DEFAULT,
> +						 attr->dpacl);
> 	fh_unlock(fhp);
> 	if (size_change)
> 		put_write_access(inode);
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 8464e04af1ea..9343aac0bd15 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -6,6 +6,8 @@
> #ifndef LINUX_NFSD_VFS_H
> #define LINUX_NFSD_VFS_H
> 
> +#include <linux/fs.h>
> +#include <linux/posix_acl.h>
> #include "nfsfh.h"
> #include "nfsd.h"
> 
> @@ -45,9 +47,17 @@ typedef int (*nfsd_filldir_t)(void *, const char *, int, loff_t, u64, unsigned);
> struct nfsd_attrs {
> 	struct iattr *iattr;
> 	struct xdr_netobj *label;
> +	struct posix_acl *pacl, *dpacl;
> 	int label_failed;
> +	int acl_failed;
> };
> 
> +static inline void nfsd_attrs_free(struct nfsd_attrs *attrs)
> +{
> +	posix_acl_release(attrs->pacl);
> +	posix_acl_release(attrs->dpacl);
> +}
> +
> int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
> 		                struct svc_export **expp);
> __be32		nfsd_lookup(struct svc_rqst *, struct svc_fh *,
> 
> 

--
Chuck Lever

Re: [PATCH 06/13] NFSD: add posix ACLs to struct nfsd_attrs

[NeilBrown]

On Sat, 30 Jul 2022, Chuck Lever III wrote:
> > @@ -837,10 +821,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > 
> > 	if (attrs.label_failed)
> > 		create->cr_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> > -
> > -	if (create->cr_acl != NULL)
> > -		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
> > -				create->cr_bmval);
> > +	if (attrs.label_failed)
> > +		create->cr_bmval[0] &= ~FATTR4_WORD0_ACL;
> 
> I think this needs to be "if (attrs.acl_failed)". I've fixed this
> in my tree.

Yes, of course :-(.
Thanks,
NeilBrown

[PATCH 10/13] NFSD: reduce locking in nfsd_lookup()

[NeilBrown]

nfsd_lookup() takes an exclusive lock on the parent inode, but no
callers want the lock and it may not be needed at all if the
result is in the dcache.

Change nfsd_lookup_dentry() to not take the lock, and call
lookup_one_len_locked() which takes lock only if needed.

nfsd4_open() currently expects the lock to still be held, but that isn't
necessary as nfsd_validate_delegated_dentry() provides required
guarantees without the lock.

NOTE: NFSv4 requires directory changeinfo for OPEN even when a create
  wasn't requested and no change happened.  Now that nfsd_lookup()
  doesn't use fh_lock(), we need to explicitly fill the attributes
  when no create happens.  A new fh_fill_both_attrs() is provided
  for that task.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4proc.c  |   20 ++++++++++++--------
 fs/nfsd/nfs4state.c |    3 ---
 fs/nfsd/nfsfh.c     |   19 +++++++++++++++++++
 fs/nfsd/nfsfh.h     |    2 +-
 fs/nfsd/vfs.c       |   34 ++++++++++++++--------------------
 5 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 1aa6ae4ec2f5..48e4efb39a9c 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -302,6 +302,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (d_really_is_positive(child)) {
 		status = nfs_ok;
 
+		/* NFSv4 protocol requires change attributes even though
+		 * no change happened.
+		 */
+		fh_fill_both_attrs(fhp);
+
 		switch (open->op_createmode) {
 		case NFS4_CREATE_UNCHECKED:
 			if (!d_is_reg(child))
@@ -417,15 +422,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 		if (nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
 			open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
 						FATTR4_WORD1_TIME_MODIFY);
-	} else
-		/*
-		 * Note this may exit with the parent still locked.
-		 * We will hold the lock until nfsd4_open's final
-		 * lookup, to prevent renames or unlinks until we've had
-		 * a chance to an acquire a delegation if appropriate.
-		 */
+	} else {
 		status = nfsd_lookup(rqstp, current_fh,
 				     open->op_fname, open->op_fnamelen, *resfh);
+		if (!status)
+			/* NFSv4 protocol requires change attributes even though
+			 * no change happened.
+			 */
+			fh_fill_both_attrs(current_fh);
+	}
 	if (status)
 		goto out;
 	status = nfsd_check_obj_isreg(*resfh);
@@ -1043,7 +1048,6 @@ nfsd4_secinfo(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 				    &exp, &dentry);
 	if (err)
 		return err;
-	fh_unlock(&cstate->current_fh);
 	if (d_really_is_negative(dentry)) {
 		exp_put(exp);
 		err = nfserr_noent;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c2ca37d0a616..45df1e85ff32 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5304,9 +5304,6 @@ nfsd4_verify_deleg_dentry(struct nfsd4_open *open, struct nfs4_file *fp,
 	struct dentry *child;
 	__be32 err;
 
-	/* parent may already be locked, and it may get unlocked by
-	 * this call, but that is safe.
-	 */
 	err = nfsd_lookup_dentry(open->op_rqstp, parent,
 				 open->op_fname, open->op_fnamelen,
 				 &exp, &child);
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 5e2ed4b2a925..cd2946a88d72 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -672,6 +672,25 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
 			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
 }
 
+/**
+ * fh_fill_both_attrs - Fill pre-op and post-op attributes
+ * @fhp: file handle to be updated
+ *
+ * This is used when the directory wasn't changed, but wcc attributes
+ * are needed anyway.
+ */
+void fh_fill_both_attrs(struct svc_fh *fhp)
+{
+	fh_fill_post_attrs(fhp);
+	if (!fhp->fh_post_saved)
+		return;
+	fhp->fh_pre_change = fhp->fh_post_change;
+	fhp->fh_pre_mtime = fhp->fh_post_attr.mtime;
+	fhp->fh_pre_ctime = fhp->fh_post_attr.ctime;
+	fhp->fh_pre_size = fhp->fh_post_attr.size;
+	fhp->fh_pre_saved = true;
+}
+
 /*
  * Release a file handle.
  */
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index fb9d358a267e..28a4f9a94e2c 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -322,7 +322,7 @@ static inline u64 nfsd4_change_attribute(struct kstat *stat,
 
 extern void fh_fill_pre_attrs(struct svc_fh *fhp);
 extern void fh_fill_post_attrs(struct svc_fh *fhp);
-
+extern void fh_fill_both_attrs(struct svc_fh *fhp);
 
 /*
  * Lock a file handle/inode
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 06b1408db08b..8ebad4a99552 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -199,27 +199,13 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
 				goto out_nfserr;
 		}
 	} else {
-		/*
-		 * In the nfsd4_open() case, this may be held across
-		 * subsequent open and delegation acquisition which may
-		 * need to take the child's i_mutex:
-		 */
-		fh_lock_nested(fhp, I_MUTEX_PARENT);
-		dentry = lookup_one_len(name, dparent, len);
+		dentry = lookup_one_len_unlocked(name, dparent, len);
 		host_err = PTR_ERR(dentry);
 		if (IS_ERR(dentry))
 			goto out_nfserr;
 		if (nfsd_mountpoint(dentry, exp)) {
-			/*
-			 * We don't need the i_mutex after all.  It's
-			 * still possible we could open this (regular
-			 * files can be mountpoints too), but the
-			 * i_mutex is just there to prevent renames of
-			 * something that we might be about to delegate,
-			 * and a mountpoint won't be renamed:
-			 */
-			fh_unlock(fhp);
-			if ((host_err = nfsd_cross_mnt(rqstp, &dentry, &exp))) {
+			host_err = nfsd_cross_mnt(rqstp, &dentry, &exp);
+			if (host_err) {
 				dput(dentry);
 				goto out_nfserr;
 			}
@@ -234,7 +220,15 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfserrno(host_err);
 }
 
-/*
+/**
+ * nfsd_lookup - look up a single path component for nfsd
+ *
+ * @rqstp:   the request context
+ * @ftp:     the file handle of the directory
+ * @name:    the component name, or %NULL to look up parent
+ * @len:     length of name to examine
+ * @resfh:   pointer to pre-initialised filehandle to hold result.
+ *
  * Look up one component of a pathname.
  * N.B. After this call _both_ fhp and resfh need an fh_put
  *
@@ -244,11 +238,11 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
  * returned. Otherwise the covered directory is returned.
  * NOTE: this mountpoint crossing is not supported properly by all
  *   clients and is explicitly disallowed for NFSv3
- *      NeilBrown <neilb@cse.unsw.edu.au>
+ *
  */
 __be32
 nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
-				unsigned int len, struct svc_fh *resfh)
+	    unsigned int len, struct svc_fh *resfh)
 {
 	struct svc_export	*exp;
 	struct dentry		*dentry;

Re: [PATCH 10/13] NFSD: reduce locking in nfsd_lookup()

[Chuck Lever III]

> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> 
> nfsd_lookup() takes an exclusive lock on the parent inode, but no
> callers want the lock and it may not be needed at all if the
> result is in the dcache.
> 
> Change nfsd_lookup_dentry() to not take the lock, and call
> lookup_one_len_locked() which takes lock only if needed.
> 
> nfsd4_open() currently expects the lock to still be held, but that isn't
> necessary as nfsd_validate_delegated_dentry() provides required
> guarantees without the lock.
> 
> NOTE: NFSv4 requires directory changeinfo for OPEN even when a create
>  wasn't requested and no change happened.  Now that nfsd_lookup()
>  doesn't use fh_lock(), we need to explicitly fill the attributes
>  when no create happens.  A new fh_fill_both_attrs() is provided
>  for that task.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs4proc.c  |   20 ++++++++++++--------
> fs/nfsd/nfs4state.c |    3 ---
> fs/nfsd/nfsfh.c     |   19 +++++++++++++++++++
> fs/nfsd/nfsfh.h     |    2 +-
> fs/nfsd/vfs.c       |   34 ++++++++++++++--------------------
> 5 files changed, 46 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 1aa6ae4ec2f5..48e4efb39a9c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -302,6 +302,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	if (d_really_is_positive(child)) {
> 		status = nfs_ok;
> 
> +		/* NFSv4 protocol requires change attributes even though
> +		 * no change happened.
> +		 */
> +		fh_fill_both_attrs(fhp);
> +
> 		switch (open->op_createmode) {
> 		case NFS4_CREATE_UNCHECKED:
> 			if (!d_is_reg(child))
> @@ -417,15 +422,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> 		if (nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
> 			open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
> 						FATTR4_WORD1_TIME_MODIFY);
> -	} else
> -		/*
> -		 * Note this may exit with the parent still locked.
> -		 * We will hold the lock until nfsd4_open's final
> -		 * lookup, to prevent renames or unlinks until we've had
> -		 * a chance to an acquire a delegation if appropriate.
> -		 */
> +	} else {
> 		status = nfsd_lookup(rqstp, current_fh,
> 				     open->op_fname, open->op_fnamelen, *resfh);
> +		if (!status)
> +			/* NFSv4 protocol requires change attributes even though
> +			 * no change happened.
> +			 */
> +			fh_fill_both_attrs(current_fh);
> +	}
> 	if (status)
> 		goto out;
> 	status = nfsd_check_obj_isreg(*resfh);
> @@ -1043,7 +1048,6 @@ nfsd4_secinfo(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 				    &exp, &dentry);
> 	if (err)
> 		return err;
> -	fh_unlock(&cstate->current_fh);
> 	if (d_really_is_negative(dentry)) {
> 		exp_put(exp);
> 		err = nfserr_noent;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c2ca37d0a616..45df1e85ff32 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5304,9 +5304,6 @@ nfsd4_verify_deleg_dentry(struct nfsd4_open *open, struct nfs4_file *fp,
> 	struct dentry *child;
> 	__be32 err;
> 
> -	/* parent may already be locked, and it may get unlocked by
> -	 * this call, but that is safe.
> -	 */
> 	err = nfsd_lookup_dentry(open->op_rqstp, parent,
> 				 open->op_fname, open->op_fnamelen,
> 				 &exp, &child);
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 5e2ed4b2a925..cd2946a88d72 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -672,6 +672,25 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
> 			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> }
> 
> +/**
> + * fh_fill_both_attrs - Fill pre-op and post-op attributes
> + * @fhp: file handle to be updated
> + *
> + * This is used when the directory wasn't changed, but wcc attributes
> + * are needed anyway.
> + */
> +void fh_fill_both_attrs(struct svc_fh *fhp)
> +{
> +	fh_fill_post_attrs(fhp);
> +	if (!fhp->fh_post_saved)
> +		return;
> +	fhp->fh_pre_change = fhp->fh_post_change;
> +	fhp->fh_pre_mtime = fhp->fh_post_attr.mtime;
> +	fhp->fh_pre_ctime = fhp->fh_post_attr.ctime;
> +	fhp->fh_pre_size = fhp->fh_post_attr.size;
> +	fhp->fh_pre_saved = true;
> +}
> +
> /*
>  * Release a file handle.
>  */
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index fb9d358a267e..28a4f9a94e2c 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -322,7 +322,7 @@ static inline u64 nfsd4_change_attribute(struct kstat *stat,
> 
> extern void fh_fill_pre_attrs(struct svc_fh *fhp);
> extern void fh_fill_post_attrs(struct svc_fh *fhp);
> -
> +extern void fh_fill_both_attrs(struct svc_fh *fhp);
> 
> /*
>  * Lock a file handle/inode
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 06b1408db08b..8ebad4a99552 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -199,27 +199,13 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 				goto out_nfserr;
> 		}
> 	} else {
> -		/*
> -		 * In the nfsd4_open() case, this may be held across
> -		 * subsequent open and delegation acquisition which may
> -		 * need to take the child's i_mutex:
> -		 */
> -		fh_lock_nested(fhp, I_MUTEX_PARENT);
> -		dentry = lookup_one_len(name, dparent, len);
> +		dentry = lookup_one_len_unlocked(name, dparent, len);
> 		host_err = PTR_ERR(dentry);
> 		if (IS_ERR(dentry))
> 			goto out_nfserr;
> 		if (nfsd_mountpoint(dentry, exp)) {
> -			/*
> -			 * We don't need the i_mutex after all.  It's
> -			 * still possible we could open this (regular
> -			 * files can be mountpoints too), but the
> -			 * i_mutex is just there to prevent renames of
> -			 * something that we might be about to delegate,
> -			 * and a mountpoint won't be renamed:
> -			 */
> -			fh_unlock(fhp);
> -			if ((host_err = nfsd_cross_mnt(rqstp, &dentry, &exp))) {
> +			host_err = nfsd_cross_mnt(rqstp, &dentry, &exp);
> +			if (host_err) {
> 				dput(dentry);
> 				goto out_nfserr;
> 			}
> @@ -234,7 +220,15 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	return nfserrno(host_err);
> }
> 
> -/*
> +/**
> + * nfsd_lookup - look up a single path component for nfsd
> + *
> + * @rqstp:   the request context
> + * @ftp:     the file handle of the directory

/home/cel/src/linux/klimt/fs/nfsd/vfs.c:246: warning: Function parameter or member 'fhp' not described in 'nfsd_lookup'
/home/cel/src/linux/klimt/fs/nfsd/vfs.c:246: warning: Excess function parameter 'ftp' description in 'nfsd_lookup'


> + * @name:    the component name, or %NULL to look up parent
> + * @len:     length of name to examine
> + * @resfh:   pointer to pre-initialised filehandle to hold result.
> + *
>  * Look up one component of a pathname.
>  * N.B. After this call _both_ fhp and resfh need an fh_put
>  *
> @@ -244,11 +238,11 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  * returned. Otherwise the covered directory is returned.
>  * NOTE: this mountpoint crossing is not supported properly by all
>  *   clients and is explicitly disallowed for NFSv3
> - *      NeilBrown <neilb@cse.unsw.edu.au>
> + *
>  */
> __be32
> nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
> -				unsigned int len, struct svc_fh *resfh)
> +	    unsigned int len, struct svc_fh *resfh)
> {
> 	struct svc_export	*exp;
> 	struct dentry		*dentry;
> 
> 

--
Chuck Lever

[PATCH 03/13] NFSD: introduce struct nfsd_attrs

[NeilBrown]

The attributes that nfsd might want to set on a file include 'struct
iattr' as well as an ACL and security label.
The latter two are passed around quite separately from the first, in
part because they are only needed for NFSv4.  This leads to some
clumsiness in the code, such as the attributes NOT being set in
nfsd_create_setattr().

We need to keep the directory locked until all attributes are set to
ensure the file is never visibile without all its attributes.  This need
combined with the inconsistent handling of attributes leads to more
clumsiness.

As a first step towards tidying this up, introduce 'struct nfsd_attrs'.
This is passed (by reference) to vfs.c functions that work with
attributes, and is assembled by the various nfs*proc functions which
call them.  As yet only iattr is included, but future patches will
expand this.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs3proc.c  |   12 ++++++++----
 fs/nfsd/nfs4proc.c  |   17 ++++++++++-------
 fs/nfsd/nfs4state.c |    5 ++++-
 fs/nfsd/nfsproc.c   |   11 +++++++----
 fs/nfsd/vfs.c       |   22 +++++++++++++---------
 fs/nfsd/vfs.h       |   12 ++++++++----
 6 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 981a3a7a6e16..289eb844d086 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -67,12 +67,13 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp)
 {
 	struct nfsd3_sattrargs *argp = rqstp->rq_argp;
 	struct nfsd3_attrstat *resp = rqstp->rq_resp;
+	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
 
 	dprintk("nfsd: SETATTR(3)  %s\n",
 				SVCFH_fmt(&argp->fh));
 
 	fh_copy(&resp->fh, &argp->fh);
-	resp->status = nfsd_setattr(rqstp, &resp->fh, &argp->attrs,
+	resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs,
 				    argp->check_guard, argp->guardtime);
 	return rpc_success;
 }
@@ -233,6 +234,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 {
 	struct iattr *iap = &argp->attrs;
 	struct dentry *parent, *child;
+	struct nfsd_attrs attrs = { .iattr = iap };
 	__u32 v_mtime, v_atime;
 	struct inode *inode;
 	__be32 status;
@@ -331,7 +333,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	}
 
 set_attr:
-	status = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
+	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
 
 out:
 	fh_unlock(fhp);
@@ -368,6 +370,7 @@ nfsd3_proc_mkdir(struct svc_rqst *rqstp)
 {
 	struct nfsd3_createargs *argp = rqstp->rq_argp;
 	struct nfsd3_diropres *resp = rqstp->rq_resp;
+	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
 
 	dprintk("nfsd: MKDIR(3)    %s %.*s\n",
 				SVCFH_fmt(&argp->fh),
@@ -378,7 +381,7 @@ nfsd3_proc_mkdir(struct svc_rqst *rqstp)
 	fh_copy(&resp->dirfh, &argp->fh);
 	fh_init(&resp->fh, NFS3_FHSIZE);
 	resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
-				   &argp->attrs, S_IFDIR, 0, &resp->fh);
+				   &attrs, S_IFDIR, 0, &resp->fh);
 	fh_unlock(&resp->dirfh);
 	return rpc_success;
 }
@@ -428,6 +431,7 @@ nfsd3_proc_mknod(struct svc_rqst *rqstp)
 {
 	struct nfsd3_mknodargs *argp = rqstp->rq_argp;
 	struct nfsd3_diropres  *resp = rqstp->rq_resp;
+	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
 	int type;
 	dev_t	rdev = 0;
 
@@ -453,7 +457,7 @@ nfsd3_proc_mknod(struct svc_rqst *rqstp)
 
 	type = nfs3_ftypes[argp->ftype];
 	resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
-				   &argp->attrs, type, rdev, &resp->fh);
+				   &attrs, type, rdev, &resp->fh);
 	fh_unlock(&resp->dirfh);
 out:
 	return rpc_success;
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index d57f91fa9f70..ba750d76f515 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -286,6 +286,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		  struct svc_fh *resfhp, struct nfsd4_open *open)
 {
 	struct iattr *iap = &open->op_iattr;
+	struct nfsd_attrs attrs = { .iattr = iap };
 	struct dentry *parent, *child;
 	__u32 v_mtime, v_atime;
 	struct inode *inode;
@@ -404,7 +405,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	}
 
 set_attr:
-	status = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
+	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
 
 out:
 	fh_unlock(fhp);
@@ -787,6 +788,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	     union nfsd4_op_u *u)
 {
 	struct nfsd4_create *create = &u->create;
+	struct nfsd_attrs attrs = { .iattr = &create->cr_iattr };
 	struct svc_fh resfh;
 	__be32 status;
 	dev_t rdev;
@@ -818,7 +820,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			goto out_umask;
 		status = nfsd_create(rqstp, &cstate->current_fh,
 				     create->cr_name, create->cr_namelen,
-				     &create->cr_iattr, S_IFBLK, rdev, &resfh);
+				     &attrs, S_IFBLK, rdev, &resfh);
 		break;
 
 	case NF4CHR:
@@ -829,26 +831,26 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			goto out_umask;
 		status = nfsd_create(rqstp, &cstate->current_fh,
 				     create->cr_name, create->cr_namelen,
-				     &create->cr_iattr, S_IFCHR, rdev, &resfh);
+				     &attrs, S_IFCHR, rdev, &resfh);
 		break;
 
 	case NF4SOCK:
 		status = nfsd_create(rqstp, &cstate->current_fh,
 				     create->cr_name, create->cr_namelen,
-				     &create->cr_iattr, S_IFSOCK, 0, &resfh);
+				     &attrs, S_IFSOCK, 0, &resfh);
 		break;
 
 	case NF4FIFO:
 		status = nfsd_create(rqstp, &cstate->current_fh,
 				     create->cr_name, create->cr_namelen,
-				     &create->cr_iattr, S_IFIFO, 0, &resfh);
+				     &attrs, S_IFIFO, 0, &resfh);
 		break;
 
 	case NF4DIR:
 		create->cr_iattr.ia_valid &= ~ATTR_SIZE;
 		status = nfsd_create(rqstp, &cstate->current_fh,
 				     create->cr_name, create->cr_namelen,
-				     &create->cr_iattr, S_IFDIR, 0, &resfh);
+				     &attrs, S_IFDIR, 0, &resfh);
 		break;
 
 	default:
@@ -1142,6 +1144,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	      union nfsd4_op_u *u)
 {
 	struct nfsd4_setattr *setattr = &u->setattr;
+	struct nfsd_attrs attrs = { .iattr = &setattr->sa_iattr };
 	__be32 status = nfs_ok;
 	int err;
 
@@ -1174,7 +1177,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 				&setattr->sa_label);
 	if (status)
 		goto out;
-	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
+	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
 				0, (time64_t)0);
 out:
 	fh_drop_write(&cstate->current_fh);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8f64af3e38d8..c2ca37d0a616 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5060,11 +5060,14 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
 		.ia_valid = ATTR_SIZE,
 		.ia_size = 0,
 	};
+	struct nfsd_attrs attrs = {
+		.iattr = &iattr,
+	};
 	if (!open->op_truncate)
 		return 0;
 	if (!(open->op_share_access & NFS4_SHARE_ACCESS_WRITE))
 		return nfserr_inval;
-	return nfsd_setattr(rqstp, fh, &iattr, 0, (time64_t)0);
+	return nfsd_setattr(rqstp, fh, &attrs, 0, (time64_t)0);
 }
 
 static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index fcdab8a8a41f..594d6f85c89f 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -51,6 +51,7 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
 	struct nfsd_sattrargs *argp = rqstp->rq_argp;
 	struct nfsd_attrstat *resp = rqstp->rq_resp;
 	struct iattr *iap = &argp->attrs;
+	struct nfsd_attrs attrs = { .iattr = iap };
 	struct svc_fh *fhp;
 
 	dprintk("nfsd: SETATTR  %s, valid=%x, size=%ld\n",
@@ -100,7 +101,7 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
 		}
 	}
 
-	resp->status = nfsd_setattr(rqstp, fhp, iap, 0, (time64_t)0);
+	resp->status = nfsd_setattr(rqstp, fhp, &attrs, 0, (time64_t)0);
 	if (resp->status != nfs_ok)
 		goto out;
 
@@ -260,6 +261,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 	svc_fh		*dirfhp = &argp->fh;
 	svc_fh		*newfhp = &resp->fh;
 	struct iattr	*attr = &argp->attrs;
+	struct nfsd_attrs attrs = { .iattr = attr };
 	struct inode	*inode;
 	struct dentry	*dchild;
 	int		type, mode;
@@ -385,7 +387,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 	if (!inode) {
 		/* File doesn't exist. Create it and set attrs */
 		resp->status = nfsd_create_locked(rqstp, dirfhp, argp->name,
-						  argp->len, attr, type, rdev,
+						  argp->len, &attrs, type, rdev,
 						  newfhp);
 	} else if (type == S_IFREG) {
 		dprintk("nfsd:   existing %s, valid=%x, size=%ld\n",
@@ -396,7 +398,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 		 */
 		attr->ia_valid &= ATTR_SIZE;
 		if (attr->ia_valid)
-			resp->status = nfsd_setattr(rqstp, newfhp, attr, 0,
+			resp->status = nfsd_setattr(rqstp, newfhp, &attrs, 0,
 						    (time64_t)0);
 	}
 
@@ -511,6 +513,7 @@ nfsd_proc_mkdir(struct svc_rqst *rqstp)
 {
 	struct nfsd_createargs *argp = rqstp->rq_argp;
 	struct nfsd_diropres *resp = rqstp->rq_resp;
+	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
 
 	dprintk("nfsd: MKDIR    %s %.*s\n", SVCFH_fmt(&argp->fh), argp->len, argp->name);
 
@@ -522,7 +525,7 @@ nfsd_proc_mkdir(struct svc_rqst *rqstp)
 	argp->attrs.ia_valid &= ~ATTR_SIZE;
 	fh_init(&resp->fh, NFS_FHSIZE);
 	resp->status = nfsd_create(rqstp, &argp->fh, argp->name, argp->len,
-				   &argp->attrs, S_IFDIR, 0, &resp->fh);
+				   &attrs, S_IFDIR, 0, &resp->fh);
 	fh_put(&argp->fh);
 	if (resp->status != nfs_ok)
 		goto out;
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index d79db56475d4..a85dc4dd4f3a 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -349,11 +349,13 @@ nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
  * Set various file attributes.  After this call fhp needs an fh_put.
  */
 __be32
-nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
+nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
+	     struct nfsd_attrs *attr,
 	     int check_guard, time64_t guardtime)
 {
 	struct dentry	*dentry;
 	struct inode	*inode;
+	struct iattr	*iap = attr->iattr;
 	int		accmode = NFSD_MAY_SATTR;
 	umode_t		ftype = 0;
 	__be32		err;
@@ -1208,8 +1210,9 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, u64 offset,
  */
 __be32
 nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		    struct svc_fh *resfhp, struct iattr *iap)
+		    struct svc_fh *resfhp, struct nfsd_attrs *attrs)
 {
+	struct iattr *iap = attrs->iattr;
 	__be32 status;
 
 	/*
@@ -1230,7 +1233,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	 * if the attributes have not changed.
 	 */
 	if (iap->ia_valid)
-		status = nfsd_setattr(rqstp, resfhp, iap, 0, (time64_t)0);
+		status = nfsd_setattr(rqstp, resfhp, attrs, 0, (time64_t)0);
 	else
 		status = nfserrno(commit_metadata(resfhp));
 
@@ -1269,11 +1272,12 @@ nfsd_check_ignore_resizing(struct iattr *iap)
 /* The parent directory should already be locked: */
 __be32
 nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		char *fname, int flen, struct iattr *iap,
-		int type, dev_t rdev, struct svc_fh *resfhp)
+		   char *fname, int flen, struct nfsd_attrs *attrs,
+		   int type, dev_t rdev, struct svc_fh *resfhp)
 {
 	struct dentry	*dentry, *dchild;
 	struct inode	*dirp;
+	struct iattr	*iap = attrs->iattr;
 	__be32		err;
 	int		host_err;
 
@@ -1347,7 +1351,7 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (host_err < 0)
 		goto out_nfserr;
 
-	err = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
+	err = nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
 
 out:
 	dput(dchild);
@@ -1366,8 +1370,8 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
  */
 __be32
 nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		char *fname, int flen, struct iattr *iap,
-		int type, dev_t rdev, struct svc_fh *resfhp)
+	    char *fname, int flen, struct nfsd_attrs *attrs,
+	    int type, dev_t rdev, struct svc_fh *resfhp)
 {
 	struct dentry	*dentry, *dchild = NULL;
 	__be32		err;
@@ -1399,7 +1403,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	dput(dchild);
 	if (err)
 		return err;
-	return nfsd_create_locked(rqstp, fhp, fname, flen, iap, type,
+	return nfsd_create_locked(rqstp, fhp, fname, flen, attrs, type,
 					rdev, resfhp);
 }
 
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 26347d76f44a..9bb0e3957982 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -42,6 +42,10 @@ struct nfsd_file;
 typedef int (*nfsd_filldir_t)(void *, const char *, int, loff_t, u64, unsigned);
 
 /* nfsd/vfs.c */
+struct nfsd_attrs {
+	struct iattr *iattr;
+};
+
 int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 		                struct svc_export **expp);
 __be32		nfsd_lookup(struct svc_rqst *, struct svc_fh *,
@@ -50,7 +54,7 @@ __be32		 nfsd_lookup_dentry(struct svc_rqst *, struct svc_fh *,
 				const char *, unsigned int,
 				struct svc_export **, struct dentry **);
 __be32		nfsd_setattr(struct svc_rqst *, struct svc_fh *,
-				struct iattr *, int, time64_t);
+				struct nfsd_attrs *, int, time64_t);
 int nfsd_mountpoint(struct dentry *, struct svc_export *);
 #ifdef CONFIG_NFSD_V4
 __be32          nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
@@ -63,14 +67,14 @@ __be32		nfsd4_clone_file_range(struct svc_rqst *rqstp,
 				       u64 count, bool sync);
 #endif /* CONFIG_NFSD_V4 */
 __be32		nfsd_create_locked(struct svc_rqst *, struct svc_fh *,
-				char *name, int len, struct iattr *attrs,
+				char *name, int len, struct nfsd_attrs *attrs,
 				int type, dev_t rdev, struct svc_fh *res);
 __be32		nfsd_create(struct svc_rqst *, struct svc_fh *,
-				char *name, int len, struct iattr *attrs,
+				char *name, int len, struct nfsd_attrs *attrs,
 				int type, dev_t rdev, struct svc_fh *res);
 __be32		nfsd_access(struct svc_rqst *, struct svc_fh *, u32 *, u32 *);
 __be32		nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
-				struct svc_fh *resfhp, struct iattr *iap);
+				struct svc_fh *resfhp, struct nfsd_attrs *iap);
 __be32		nfsd_commit(struct svc_rqst *rqst, struct svc_fh *fhp,
 				u64 offset, u32 count, __be32 *verf);
 #ifdef CONFIG_NFSD_V4

Re: [PATCH 03/13] NFSD: introduce struct nfsd_attrs

[Chuck Lever III]

> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> 
> The attributes that nfsd might want to set on a file include 'struct
> iattr' as well as an ACL and security label.
> The latter two are passed around quite separately from the first, in
> part because they are only needed for NFSv4.  This leads to some
> clumsiness in the code, such as the attributes NOT being set in
> nfsd_create_setattr().
> 
> We need to keep the directory locked until all attributes are set to
> ensure the file is never visibile without all its attributes.  This need
> combined with the inconsistent handling of attributes leads to more
> clumsiness.
> 
> As a first step towards tidying this up, introduce 'struct nfsd_attrs'.
> This is passed (by reference) to vfs.c functions that work with
> attributes, and is assembled by the various nfs*proc functions which
> call them.  As yet only iattr is included, but future patches will
> expand this.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs3proc.c  |   12 ++++++++----
> fs/nfsd/nfs4proc.c  |   17 ++++++++++-------
> fs/nfsd/nfs4state.c |    5 ++++-
> fs/nfsd/nfsproc.c   |   11 +++++++----
> fs/nfsd/vfs.c       |   22 +++++++++++++---------
> fs/nfsd/vfs.h       |   12 ++++++++----
> 6 files changed, 50 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 981a3a7a6e16..289eb844d086 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -67,12 +67,13 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp)
> {
> 	struct nfsd3_sattrargs *argp = rqstp->rq_argp;
> 	struct nfsd3_attrstat *resp = rqstp->rq_resp;
> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> 
> 	dprintk("nfsd: SETATTR(3)  %s\n",
> 				SVCFH_fmt(&argp->fh));
> 
> 	fh_copy(&resp->fh, &argp->fh);
> -	resp->status = nfsd_setattr(rqstp, &resp->fh, &argp->attrs,
> +	resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs,
> 				    argp->check_guard, argp->guardtime);
> 	return rpc_success;
> }
> @@ -233,6 +234,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> {
> 	struct iattr *iap = &argp->attrs;
> 	struct dentry *parent, *child;
> +	struct nfsd_attrs attrs = { .iattr = iap };
> 	__u32 v_mtime, v_atime;
> 	struct inode *inode;
> 	__be32 status;
> @@ -331,7 +333,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	}
> 
> set_attr:
> -	status = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
> +	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
> 
> out:
> 	fh_unlock(fhp);
> @@ -368,6 +370,7 @@ nfsd3_proc_mkdir(struct svc_rqst *rqstp)
> {
> 	struct nfsd3_createargs *argp = rqstp->rq_argp;
> 	struct nfsd3_diropres *resp = rqstp->rq_resp;
> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> 
> 	dprintk("nfsd: MKDIR(3)    %s %.*s\n",
> 				SVCFH_fmt(&argp->fh),
> @@ -378,7 +381,7 @@ nfsd3_proc_mkdir(struct svc_rqst *rqstp)
> 	fh_copy(&resp->dirfh, &argp->fh);
> 	fh_init(&resp->fh, NFS3_FHSIZE);
> 	resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
> -				   &argp->attrs, S_IFDIR, 0, &resp->fh);
> +				   &attrs, S_IFDIR, 0, &resp->fh);
> 	fh_unlock(&resp->dirfh);
> 	return rpc_success;
> }
> @@ -428,6 +431,7 @@ nfsd3_proc_mknod(struct svc_rqst *rqstp)
> {
> 	struct nfsd3_mknodargs *argp = rqstp->rq_argp;
> 	struct nfsd3_diropres  *resp = rqstp->rq_resp;
> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> 	int type;
> 	dev_t	rdev = 0;
> 
> @@ -453,7 +457,7 @@ nfsd3_proc_mknod(struct svc_rqst *rqstp)
> 
> 	type = nfs3_ftypes[argp->ftype];
> 	resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
> -				   &argp->attrs, type, rdev, &resp->fh);
> +				   &attrs, type, rdev, &resp->fh);
> 	fh_unlock(&resp->dirfh);
> out:
> 	return rpc_success;
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index d57f91fa9f70..ba750d76f515 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -286,6 +286,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 		  struct svc_fh *resfhp, struct nfsd4_open *open)
> {
> 	struct iattr *iap = &open->op_iattr;
> +	struct nfsd_attrs attrs = { .iattr = iap };
> 	struct dentry *parent, *child;
> 	__u32 v_mtime, v_atime;
> 	struct inode *inode;
> @@ -404,7 +405,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	}
> 
> set_attr:
> -	status = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
> +	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
> 
> out:
> 	fh_unlock(fhp);
> @@ -787,6 +788,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	     union nfsd4_op_u *u)
> {
> 	struct nfsd4_create *create = &u->create;
> +	struct nfsd_attrs attrs = { .iattr = &create->cr_iattr };
> 	struct svc_fh resfh;
> 	__be32 status;
> 	dev_t rdev;
> @@ -818,7 +820,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 			goto out_umask;
> 		status = nfsd_create(rqstp, &cstate->current_fh,
> 				     create->cr_name, create->cr_namelen,
> -				     &create->cr_iattr, S_IFBLK, rdev, &resfh);
> +				     &attrs, S_IFBLK, rdev, &resfh);
> 		break;
> 
> 	case NF4CHR:
> @@ -829,26 +831,26 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 			goto out_umask;
> 		status = nfsd_create(rqstp, &cstate->current_fh,
> 				     create->cr_name, create->cr_namelen,
> -				     &create->cr_iattr, S_IFCHR, rdev, &resfh);
> +				     &attrs, S_IFCHR, rdev, &resfh);
> 		break;
> 
> 	case NF4SOCK:
> 		status = nfsd_create(rqstp, &cstate->current_fh,
> 				     create->cr_name, create->cr_namelen,
> -				     &create->cr_iattr, S_IFSOCK, 0, &resfh);
> +				     &attrs, S_IFSOCK, 0, &resfh);
> 		break;
> 
> 	case NF4FIFO:
> 		status = nfsd_create(rqstp, &cstate->current_fh,
> 				     create->cr_name, create->cr_namelen,
> -				     &create->cr_iattr, S_IFIFO, 0, &resfh);
> +				     &attrs, S_IFIFO, 0, &resfh);
> 		break;
> 
> 	case NF4DIR:
> 		create->cr_iattr.ia_valid &= ~ATTR_SIZE;
> 		status = nfsd_create(rqstp, &cstate->current_fh,
> 				     create->cr_name, create->cr_namelen,
> -				     &create->cr_iattr, S_IFDIR, 0, &resfh);
> +				     &attrs, S_IFDIR, 0, &resfh);
> 		break;
> 
> 	default:
> @@ -1142,6 +1144,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	      union nfsd4_op_u *u)
> {
> 	struct nfsd4_setattr *setattr = &u->setattr;
> +	struct nfsd_attrs attrs = { .iattr = &setattr->sa_iattr };
> 	__be32 status = nfs_ok;
> 	int err;
> 
> @@ -1174,7 +1177,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 				&setattr->sa_label);
> 	if (status)
> 		goto out;
> -	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
> +	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> 				0, (time64_t)0);
> out:
> 	fh_drop_write(&cstate->current_fh);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 8f64af3e38d8..c2ca37d0a616 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5060,11 +5060,14 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
> 		.ia_valid = ATTR_SIZE,
> 		.ia_size = 0,
> 	};
> +	struct nfsd_attrs attrs = {
> +		.iattr = &iattr,
> +	};
> 	if (!open->op_truncate)
> 		return 0;
> 	if (!(open->op_share_access & NFS4_SHARE_ACCESS_WRITE))
> 		return nfserr_inval;
> -	return nfsd_setattr(rqstp, fh, &iattr, 0, (time64_t)0);
> +	return nfsd_setattr(rqstp, fh, &attrs, 0, (time64_t)0);
> }
> 
> static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index fcdab8a8a41f..594d6f85c89f 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -51,6 +51,7 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
> 	struct nfsd_sattrargs *argp = rqstp->rq_argp;
> 	struct nfsd_attrstat *resp = rqstp->rq_resp;
> 	struct iattr *iap = &argp->attrs;
> +	struct nfsd_attrs attrs = { .iattr = iap };
> 	struct svc_fh *fhp;
> 
> 	dprintk("nfsd: SETATTR  %s, valid=%x, size=%ld\n",
> @@ -100,7 +101,7 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
> 		}
> 	}
> 
> -	resp->status = nfsd_setattr(rqstp, fhp, iap, 0, (time64_t)0);
> +	resp->status = nfsd_setattr(rqstp, fhp, &attrs, 0, (time64_t)0);
> 	if (resp->status != nfs_ok)
> 		goto out;
> 
> @@ -260,6 +261,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> 	svc_fh		*dirfhp = &argp->fh;
> 	svc_fh		*newfhp = &resp->fh;
> 	struct iattr	*attr = &argp->attrs;
> +	struct nfsd_attrs attrs = { .iattr = attr };
> 	struct inode	*inode;
> 	struct dentry	*dchild;
> 	int		type, mode;
> @@ -385,7 +387,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> 	if (!inode) {
> 		/* File doesn't exist. Create it and set attrs */
> 		resp->status = nfsd_create_locked(rqstp, dirfhp, argp->name,
> -						  argp->len, attr, type, rdev,
> +						  argp->len, &attrs, type, rdev,
> 						  newfhp);
> 	} else if (type == S_IFREG) {
> 		dprintk("nfsd:   existing %s, valid=%x, size=%ld\n",
> @@ -396,7 +398,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
> 		 */
> 		attr->ia_valid &= ATTR_SIZE;
> 		if (attr->ia_valid)
> -			resp->status = nfsd_setattr(rqstp, newfhp, attr, 0,
> +			resp->status = nfsd_setattr(rqstp, newfhp, &attrs, 0,
> 						    (time64_t)0);
> 	}
> 
> @@ -511,6 +513,7 @@ nfsd_proc_mkdir(struct svc_rqst *rqstp)
> {
> 	struct nfsd_createargs *argp = rqstp->rq_argp;
> 	struct nfsd_diropres *resp = rqstp->rq_resp;
> +	struct nfsd_attrs attrs = { .iattr = &argp->attrs };
> 
> 	dprintk("nfsd: MKDIR    %s %.*s\n", SVCFH_fmt(&argp->fh), argp->len, argp->name);
> 
> @@ -522,7 +525,7 @@ nfsd_proc_mkdir(struct svc_rqst *rqstp)
> 	argp->attrs.ia_valid &= ~ATTR_SIZE;
> 	fh_init(&resp->fh, NFS_FHSIZE);
> 	resp->status = nfsd_create(rqstp, &argp->fh, argp->name, argp->len,
> -				   &argp->attrs, S_IFDIR, 0, &resp->fh);
> +				   &attrs, S_IFDIR, 0, &resp->fh);
> 	fh_put(&argp->fh);
> 	if (resp->status != nfs_ok)
> 		goto out;
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index d79db56475d4..a85dc4dd4f3a 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -349,11 +349,13 @@ nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  * Set various file attributes.  After this call fhp needs an fh_put.
>  */
> __be32
> -nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> +nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> +	     struct nfsd_attrs *attr,
> 	     int check_guard, time64_t guardtime)
> {
> 	struct dentry	*dentry;
> 	struct inode	*inode;
> +	struct iattr	*iap = attr->iattr;
> 	int		accmode = NFSD_MAY_SATTR;
> 	umode_t		ftype = 0;
> 	__be32		err;
> @@ -1208,8 +1210,9 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, u64 offset,
>  */
> __be32
> nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		    struct svc_fh *resfhp, struct iattr *iap)
> +		    struct svc_fh *resfhp, struct nfsd_attrs *attrs)
> {
> +	struct iattr *iap = attrs->iattr;
> 	__be32 status;
> 
> 	/*

/home/cel/src/linux/klimt/fs/nfsd/vfs.c:1214: warning: Function parameter or member 'attrs' not described in 'nfsd_create_setattr'
/home/cel/src/linux/klimt/fs/nfsd/vfs.c:1214: warning: Excess function parameter 'iap' description in 'nfsd_create_setattr'


> @@ -1230,7 +1233,7 @@ nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	 * if the attributes have not changed.
> 	 */
> 	if (iap->ia_valid)
> -		status = nfsd_setattr(rqstp, resfhp, iap, 0, (time64_t)0);
> +		status = nfsd_setattr(rqstp, resfhp, attrs, 0, (time64_t)0);
> 	else
> 		status = nfserrno(commit_metadata(resfhp));
> 
> @@ -1269,11 +1272,12 @@ nfsd_check_ignore_resizing(struct iattr *iap)
> /* The parent directory should already be locked: */
> __be32
> nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		char *fname, int flen, struct iattr *iap,
> -		int type, dev_t rdev, struct svc_fh *resfhp)
> +		   char *fname, int flen, struct nfsd_attrs *attrs,
> +		   int type, dev_t rdev, struct svc_fh *resfhp)
> {
> 	struct dentry	*dentry, *dchild;
> 	struct inode	*dirp;
> +	struct iattr	*iap = attrs->iattr;
> 	__be32		err;
> 	int		host_err;
> 
> @@ -1347,7 +1351,7 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	if (host_err < 0)
> 		goto out_nfserr;
> 
> -	err = nfsd_create_setattr(rqstp, fhp, resfhp, iap);
> +	err = nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> 
> out:
> 	dput(dchild);
> @@ -1366,8 +1370,8 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  */
> __be32
> nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		char *fname, int flen, struct iattr *iap,
> -		int type, dev_t rdev, struct svc_fh *resfhp)
> +	    char *fname, int flen, struct nfsd_attrs *attrs,
> +	    int type, dev_t rdev, struct svc_fh *resfhp)
> {
> 	struct dentry	*dentry, *dchild = NULL;
> 	__be32		err;
> @@ -1399,7 +1403,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	dput(dchild);
> 	if (err)
> 		return err;
> -	return nfsd_create_locked(rqstp, fhp, fname, flen, iap, type,
> +	return nfsd_create_locked(rqstp, fhp, fname, flen, attrs, type,
> 					rdev, resfhp);
> }
> 
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 26347d76f44a..9bb0e3957982 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -42,6 +42,10 @@ struct nfsd_file;
> typedef int (*nfsd_filldir_t)(void *, const char *, int, loff_t, u64, unsigned);
> 
> /* nfsd/vfs.c */
> +struct nfsd_attrs {
> +	struct iattr *iattr;
> +};

Please separate the type and the field name with a tab,
and use field names that are easier to grep for, like:

na_iattr
na_seclabel
na_pacl
na_dpacl
na_labelerr
na_aclerr


> +
> int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
> 		                struct svc_export **expp);
> __be32		nfsd_lookup(struct svc_rqst *, struct svc_fh *,
> @@ -50,7 +54,7 @@ __be32		 nfsd_lookup_dentry(struct svc_rqst *, struct svc_fh *,
> 				const char *, unsigned int,
> 				struct svc_export **, struct dentry **);
> __be32		nfsd_setattr(struct svc_rqst *, struct svc_fh *,
> -				struct iattr *, int, time64_t);
> +				struct nfsd_attrs *, int, time64_t);
> int nfsd_mountpoint(struct dentry *, struct svc_export *);
> #ifdef CONFIG_NFSD_V4
> __be32          nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
> @@ -63,14 +67,14 @@ __be32		nfsd4_clone_file_range(struct svc_rqst *rqstp,
> 				       u64 count, bool sync);
> #endif /* CONFIG_NFSD_V4 */
> __be32		nfsd_create_locked(struct svc_rqst *, struct svc_fh *,
> -				char *name, int len, struct iattr *attrs,
> +				char *name, int len, struct nfsd_attrs *attrs,
> 				int type, dev_t rdev, struct svc_fh *res);
> __be32		nfsd_create(struct svc_rqst *, struct svc_fh *,
> -				char *name, int len, struct iattr *attrs,
> +				char *name, int len, struct nfsd_attrs *attrs,
> 				int type, dev_t rdev, struct svc_fh *res);
> __be32		nfsd_access(struct svc_rqst *, struct svc_fh *, u32 *, u32 *);
> __be32		nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -				struct svc_fh *resfhp, struct iattr *iap);
> +				struct svc_fh *resfhp, struct nfsd_attrs *iap);
> __be32		nfsd_commit(struct svc_rqst *rqst, struct svc_fh *fhp,
> 				u64 offset, u32 count, __be32 *verf);
> #ifdef CONFIG_NFSD_V4
> 
> 

--
Chuck Lever

[PATCH 05/13] NFSD: add security label to struct nfsd_attrs

[NeilBrown]

nfsd_setattr() now sets a security label if provided, and nfsv4 provides
it in the 'open' and 'create' paths and the 'setattr' path.
If setting the label failed (including because the kernel doesn't
support labels), an error field in 'struct nfsd_attrs' is set, and the
caller can respond.  The open/create callers clear
FATTR4_WORD2_SECURITY_LABEL in the returned attr set in this case.
The setattr caller returns the error.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4proc.c |   61 +++++++++++++++-------------------------------------
 fs/nfsd/vfs.c      |   29 +++----------------------
 fs/nfsd/vfs.h      |    4 ++-
 3 files changed, 23 insertions(+), 71 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ee72c94732f0..83d2b645b0d6 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -64,36 +64,6 @@ MODULE_PARM_DESC(nfsd4_ssc_umount_timeout,
 		"idle msecs before unmount export from source server");
 #endif
 
-#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
-#include <linux/security.h>
-
-static inline void
-nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct xdr_netobj *label, u32 *bmval)
-{
-	struct inode *inode = d_inode(resfh->fh_dentry);
-	int status;
-
-	inode_lock(inode);
-	status = security_inode_setsecctx(resfh->fh_dentry,
-		label->data, label->len);
-	inode_unlock(inode);
-
-	if (status)
-		/*
-		 * XXX: We should really fail the whole open, but we may
-		 * already have created a new file, so it may be too
-		 * late.  For now this seems the least of evils:
-		 */
-		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
-
-	return;
-}
-#else
-static inline void
-nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct xdr_netobj *label, u32 *bmval)
-{ }
-#endif
-
 #define NFSDDBG_FACILITY		NFSDDBG_PROC
 
 static u32 nfsd_attrmask[] = {
@@ -286,7 +256,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		  struct svc_fh *resfhp, struct nfsd4_open *open)
 {
 	struct iattr *iap = &open->op_iattr;
-	struct nfsd_attrs attrs = { .iattr = iap };
+	struct nfsd_attrs attrs = {
+		.iattr = iap,
+		.label = &open->op_label,
+	};
 	struct dentry *parent, *child;
 	__u32 v_mtime, v_atime;
 	struct inode *inode;
@@ -407,6 +380,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 set_attr:
 	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
 
+	if (attrs.label_failed)
+		open->op_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
 out:
 	fh_unlock(fhp);
 	if (child && !IS_ERR(child))
@@ -448,9 +423,6 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 		status = nfsd4_create_file(rqstp, current_fh, *resfh, open);
 		current->fs->umask = 0;
 
-		if (!status && open->op_label.len)
-			nfsd4_security_inode_setsecctx(*resfh, &open->op_label, open->op_bmval);
-
 		/*
 		 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
 		 * use the returned bitmask to indicate which attributes
@@ -788,7 +760,10 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	     union nfsd4_op_u *u)
 {
 	struct nfsd4_create *create = &u->create;
-	struct nfsd_attrs attrs = { .iattr = &create->cr_iattr };
+	struct nfsd_attrs attrs = {
+		.iattr = &create->cr_iattr,
+		.label = &create->cr_label,
+	};
 	struct svc_fh resfh;
 	__be32 status;
 	dev_t rdev;
@@ -860,8 +835,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		goto out;
 
-	if (create->cr_label.len)
-		nfsd4_security_inode_setsecctx(&resfh, &create->cr_label, create->cr_bmval);
+	if (attrs.label_failed)
+		create->cr_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
 
 	if (create->cr_acl != NULL)
 		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
@@ -1144,7 +1119,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	      union nfsd4_op_u *u)
 {
 	struct nfsd4_setattr *setattr = &u->setattr;
-	struct nfsd_attrs attrs = { .iattr = &setattr->sa_iattr };
+	struct nfsd_attrs attrs = {
+		.iattr = &setattr->sa_iattr,
+		.label = &setattr->sa_label,
+	};
 	__be32 status = nfs_ok;
 	int err;
 
@@ -1172,13 +1150,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 					    setattr->sa_acl);
 	if (status)
 		goto out;
-	if (setattr->sa_label.len)
-		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
-				&setattr->sa_label);
-	if (status)
-		goto out;
 	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
 				0, (time64_t)0);
+	if (!status)
+		status = attrs.label_failed;
 out:
 	fh_drop_write(&cstate->current_fh);
 	return status;
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 91c9ea09f921..e7a18bedf499 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -458,6 +458,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	host_err = notify_change(&init_user_ns, dentry, iap, NULL);
 
 out_unlock:
+	if (attr->label && attr->label->len)
+		attr->label_failed = security_inode_setsecctx(
+			dentry, attr->label->data, attr->label->len);
 	fh_unlock(fhp);
 	if (size_change)
 		put_write_access(inode);
@@ -496,32 +499,6 @@ int nfsd4_is_junction(struct dentry *dentry)
 		return 0;
 	return 1;
 }
-#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
-__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		struct xdr_netobj *label)
-{
-	__be32 error;
-	int host_error;
-	struct dentry *dentry;
-
-	error = fh_verify(rqstp, fhp, 0 /* S_IFREG */, NFSD_MAY_SATTR);
-	if (error)
-		return error;
-
-	dentry = fhp->fh_dentry;
-
-	inode_lock(d_inode(dentry));
-	host_error = security_inode_setsecctx(dentry, label->data, label->len);
-	inode_unlock(d_inode(dentry));
-	return nfserrno(host_error);
-}
-#else
-__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		struct xdr_netobj *label)
-{
-	return nfserr_notsupp;
-}
-#endif
 
 static struct nfsd4_compound_state *nfsd4_get_cstate(struct svc_rqst *rqstp)
 {
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index f3f43ca3ac6b..8464e04af1ea 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -44,6 +44,8 @@ typedef int (*nfsd_filldir_t)(void *, const char *, int, loff_t, u64, unsigned);
 /* nfsd/vfs.c */
 struct nfsd_attrs {
 	struct iattr *iattr;
+	struct xdr_netobj *label;
+	int label_failed;
 };
 
 int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
@@ -57,8 +59,6 @@ __be32		nfsd_setattr(struct svc_rqst *, struct svc_fh *,
 				struct nfsd_attrs *, int, time64_t);
 int nfsd_mountpoint(struct dentry *, struct svc_export *);
 #ifdef CONFIG_NFSD_V4
-__be32          nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
-		    struct xdr_netobj *);
 __be32		nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *,
 				    struct file *, loff_t, loff_t, int);
 __be32		nfsd4_clone_file_range(struct svc_rqst *rqstp,

Re: [PATCH 05/13] NFSD: add security label to struct nfsd_attrs

[Chuck Lever III]

> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> 
> nfsd_setattr() now sets a security label if provided, and nfsv4 provides
> it in the 'open' and 'create' paths and the 'setattr' path.
> If setting the label failed (including because the kernel doesn't
> support labels), an error field in 'struct nfsd_attrs' is set, and the
> caller can respond.  The open/create callers clear
> FATTR4_WORD2_SECURITY_LABEL in the returned attr set in this case.
> The setattr caller returns the error.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs4proc.c |   61 +++++++++++++++-------------------------------------
> fs/nfsd/vfs.c      |   29 +++----------------------
> fs/nfsd/vfs.h      |    4 ++-
> 3 files changed, 23 insertions(+), 71 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index ee72c94732f0..83d2b645b0d6 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -64,36 +64,6 @@ MODULE_PARM_DESC(nfsd4_ssc_umount_timeout,
> 		"idle msecs before unmount export from source server");
> #endif
> 
> -#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> -#include <linux/security.h>
> -
> -static inline void
> -nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct xdr_netobj *label, u32 *bmval)
> -{
> -	struct inode *inode = d_inode(resfh->fh_dentry);
> -	int status;
> -
> -	inode_lock(inode);
> -	status = security_inode_setsecctx(resfh->fh_dentry,
> -		label->data, label->len);
> -	inode_unlock(inode);
> -
> -	if (status)
> -		/*
> -		 * XXX: We should really fail the whole open, but we may
> -		 * already have created a new file, so it may be too
> -		 * late.  For now this seems the least of evils:
> -		 */
> -		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> -
> -	return;
> -}
> -#else
> -static inline void
> -nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct xdr_netobj *label, u32 *bmval)
> -{ }
> -#endif
> -
> #define NFSDDBG_FACILITY		NFSDDBG_PROC
> 
> static u32 nfsd_attrmask[] = {
> @@ -286,7 +256,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 		  struct svc_fh *resfhp, struct nfsd4_open *open)
> {
> 	struct iattr *iap = &open->op_iattr;
> -	struct nfsd_attrs attrs = { .iattr = iap };
> +	struct nfsd_attrs attrs = {
> +		.iattr = iap,
> +		.label = &open->op_label,
> +	};
> 	struct dentry *parent, *child;
> 	__u32 v_mtime, v_atime;
> 	struct inode *inode;
> @@ -407,6 +380,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> set_attr:
> 	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
> 
> +	if (attrs.label_failed)
> +		open->op_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> out:
> 	fh_unlock(fhp);
> 	if (child && !IS_ERR(child))
> @@ -448,9 +423,6 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> 		status = nfsd4_create_file(rqstp, current_fh, *resfh, open);
> 		current->fs->umask = 0;
> 
> -		if (!status && open->op_label.len)
> -			nfsd4_security_inode_setsecctx(*resfh, &open->op_label, open->op_bmval);
> -
> 		/*
> 		 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
> 		 * use the returned bitmask to indicate which attributes
> @@ -788,7 +760,10 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	     union nfsd4_op_u *u)
> {
> 	struct nfsd4_create *create = &u->create;
> -	struct nfsd_attrs attrs = { .iattr = &create->cr_iattr };
> +	struct nfsd_attrs attrs = {
> +		.iattr = &create->cr_iattr,
> +		.label = &create->cr_label,
> +	};
> 	struct svc_fh resfh;
> 	__be32 status;
> 	dev_t rdev;
> @@ -860,8 +835,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	if (status)
> 		goto out;
> 
> -	if (create->cr_label.len)
> -		nfsd4_security_inode_setsecctx(&resfh, &create->cr_label, create->cr_bmval);
> +	if (attrs.label_failed)
> +		create->cr_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> 
> 	if (create->cr_acl != NULL)
> 		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
> @@ -1144,7 +1119,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	      union nfsd4_op_u *u)
> {
> 	struct nfsd4_setattr *setattr = &u->setattr;
> -	struct nfsd_attrs attrs = { .iattr = &setattr->sa_iattr };
> +	struct nfsd_attrs attrs = {
> +		.iattr = &setattr->sa_iattr,
> +		.label = &setattr->sa_label,
> +	};
> 	__be32 status = nfs_ok;
> 	int err;
> 
> @@ -1172,13 +1150,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 					    setattr->sa_acl);
> 	if (status)
> 		goto out;
> -	if (setattr->sa_label.len)
> -		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
> -				&setattr->sa_label);
> -	if (status)
> -		goto out;
> 	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> 				0, (time64_t)0);
> +	if (!status)
> +		status = attrs.label_failed;

/home/cel/src/linux/klimt/fs/nfsd/nfs4proc.c:1156:24: warning: incorrect type in assignment (different base types)
/home/cel/src/linux/klimt/fs/nfsd/nfs4proc.c:1156:24:    expected restricted __be32 [assigned] [usertype] status
/home/cel/src/linux/klimt/fs/nfsd/nfs4proc.c:1156:24:    got int [addressable] label_failed

Let's make these a __be32 nfserr status code instead.


> out:
> 	fh_drop_write(&cstate->current_fh);
> 	return status;
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 91c9ea09f921..e7a18bedf499 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -458,6 +458,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	host_err = notify_change(&init_user_ns, dentry, iap, NULL);
> 
> out_unlock:
> +	if (attr->label && attr->label->len)
> +		attr->label_failed = security_inode_setsecctx(
> +			dentry, attr->label->data, attr->label->len);
> 	fh_unlock(fhp);
> 	if (size_change)
> 		put_write_access(inode);
> @@ -496,32 +499,6 @@ int nfsd4_is_junction(struct dentry *dentry)
> 		return 0;
> 	return 1;
> }
> -#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> -__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct xdr_netobj *label)
> -{
> -	__be32 error;
> -	int host_error;
> -	struct dentry *dentry;
> -
> -	error = fh_verify(rqstp, fhp, 0 /* S_IFREG */, NFSD_MAY_SATTR);
> -	if (error)
> -		return error;
> -
> -	dentry = fhp->fh_dentry;
> -
> -	inode_lock(d_inode(dentry));
> -	host_error = security_inode_setsecctx(dentry, label->data, label->len);
> -	inode_unlock(d_inode(dentry));
> -	return nfserrno(host_error);
> -}
> -#else
> -__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct xdr_netobj *label)
> -{
> -	return nfserr_notsupp;
> -}
> -#endif
> 
> static struct nfsd4_compound_state *nfsd4_get_cstate(struct svc_rqst *rqstp)
> {
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index f3f43ca3ac6b..8464e04af1ea 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -44,6 +44,8 @@ typedef int (*nfsd_filldir_t)(void *, const char *, int, loff_t, u64, unsigned);
> /* nfsd/vfs.c */
> struct nfsd_attrs {
> 	struct iattr *iattr;
> +	struct xdr_netobj *label;
> +	int label_failed;
> };
> 
> int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
> @@ -57,8 +59,6 @@ __be32		nfsd_setattr(struct svc_rqst *, struct svc_fh *,
> 				struct nfsd_attrs *, int, time64_t);
> int nfsd_mountpoint(struct dentry *, struct svc_export *);
> #ifdef CONFIG_NFSD_V4
> -__be32          nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
> -		    struct xdr_netobj *);
> __be32		nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *,
> 				    struct file *, loff_t, loff_t, int);
> __be32		nfsd4_clone_file_range(struct svc_rqst *rqstp,
> 
> 

--
Chuck Lever

Re: [PATCH 05/13] NFSD: add security label to struct nfsd_attrs

[NeilBrown]

On Fri, 29 Jul 2022, Chuck Lever III wrote:
> 
> > On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> > 
> > nfsd_setattr() now sets a security label if provided, and nfsv4 provides
> > it in the 'open' and 'create' paths and the 'setattr' path.
> > If setting the label failed (including because the kernel doesn't
> > support labels), an error field in 'struct nfsd_attrs' is set, and the
> > caller can respond.  The open/create callers clear
> > FATTR4_WORD2_SECURITY_LABEL in the returned attr set in this case.
> > The setattr caller returns the error.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > fs/nfsd/nfs4proc.c |   61 +++++++++++++++-------------------------------------
> > fs/nfsd/vfs.c      |   29 +++----------------------
> > fs/nfsd/vfs.h      |    4 ++-
> > 3 files changed, 23 insertions(+), 71 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index ee72c94732f0..83d2b645b0d6 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -64,36 +64,6 @@ MODULE_PARM_DESC(nfsd4_ssc_umount_timeout,
> > 		"idle msecs before unmount export from source server");
> > #endif
> > 
> > -#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> > -#include <linux/security.h>
> > -
> > -static inline void
> > -nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct xdr_netobj *label, u32 *bmval)
> > -{
> > -	struct inode *inode = d_inode(resfh->fh_dentry);
> > -	int status;
> > -
> > -	inode_lock(inode);
> > -	status = security_inode_setsecctx(resfh->fh_dentry,
> > -		label->data, label->len);
> > -	inode_unlock(inode);
> > -
> > -	if (status)
> > -		/*
> > -		 * XXX: We should really fail the whole open, but we may
> > -		 * already have created a new file, so it may be too
> > -		 * late.  For now this seems the least of evils:
> > -		 */
> > -		bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> > -
> > -	return;
> > -}
> > -#else
> > -static inline void
> > -nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct xdr_netobj *label, u32 *bmval)
> > -{ }
> > -#endif
> > -
> > #define NFSDDBG_FACILITY		NFSDDBG_PROC
> > 
> > static u32 nfsd_attrmask[] = {
> > @@ -286,7 +256,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 		  struct svc_fh *resfhp, struct nfsd4_open *open)
> > {
> > 	struct iattr *iap = &open->op_iattr;
> > -	struct nfsd_attrs attrs = { .iattr = iap };
> > +	struct nfsd_attrs attrs = {
> > +		.iattr = iap,
> > +		.label = &open->op_label,
> > +	};
> > 	struct dentry *parent, *child;
> > 	__u32 v_mtime, v_atime;
> > 	struct inode *inode;
> > @@ -407,6 +380,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > set_attr:
> > 	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
> > 
> > +	if (attrs.label_failed)
> > +		open->op_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> > out:
> > 	fh_unlock(fhp);
> > 	if (child && !IS_ERR(child))
> > @@ -448,9 +423,6 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> > 		status = nfsd4_create_file(rqstp, current_fh, *resfh, open);
> > 		current->fs->umask = 0;
> > 
> > -		if (!status && open->op_label.len)
> > -			nfsd4_security_inode_setsecctx(*resfh, &open->op_label, open->op_bmval);
> > -
> > 		/*
> > 		 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
> > 		 * use the returned bitmask to indicate which attributes
> > @@ -788,7 +760,10 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > 	     union nfsd4_op_u *u)
> > {
> > 	struct nfsd4_create *create = &u->create;
> > -	struct nfsd_attrs attrs = { .iattr = &create->cr_iattr };
> > +	struct nfsd_attrs attrs = {
> > +		.iattr = &create->cr_iattr,
> > +		.label = &create->cr_label,
> > +	};
> > 	struct svc_fh resfh;
> > 	__be32 status;
> > 	dev_t rdev;
> > @@ -860,8 +835,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > 	if (status)
> > 		goto out;
> > 
> > -	if (create->cr_label.len)
> > -		nfsd4_security_inode_setsecctx(&resfh, &create->cr_label, create->cr_bmval);
> > +	if (attrs.label_failed)
> > +		create->cr_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> > 
> > 	if (create->cr_acl != NULL)
> > 		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
> > @@ -1144,7 +1119,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > 	      union nfsd4_op_u *u)
> > {
> > 	struct nfsd4_setattr *setattr = &u->setattr;
> > -	struct nfsd_attrs attrs = { .iattr = &setattr->sa_iattr };
> > +	struct nfsd_attrs attrs = {
> > +		.iattr = &setattr->sa_iattr,
> > +		.label = &setattr->sa_label,
> > +	};
> > 	__be32 status = nfs_ok;
> > 	int err;
> > 
> > @@ -1172,13 +1150,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > 					    setattr->sa_acl);
> > 	if (status)
> > 		goto out;
> > -	if (setattr->sa_label.len)
> > -		status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh,
> > -				&setattr->sa_label);
> > -	if (status)
> > -		goto out;
> > 	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> > 				0, (time64_t)0);
> > +	if (!status)
> > +		status = attrs.label_failed;
> 
> /home/cel/src/linux/klimt/fs/nfsd/nfs4proc.c:1156:24: warning: incorrect type in assignment (different base types)
> /home/cel/src/linux/klimt/fs/nfsd/nfs4proc.c:1156:24:    expected restricted __be32 [assigned] [usertype] status
> /home/cel/src/linux/klimt/fs/nfsd/nfs4proc.c:1156:24:    got int [addressable] label_failed
> 
> Let's make these a __be32 nfserr status code instead.

The value assigned to the *_failed fields are errnos, so __be32 would
not be correct.
This assignment needs to change to
        if (!status)
               status = nfserrno(attrs.label_failed);

Thanks,
NeilBrown


> 
> 
> > out:
> > 	fh_drop_write(&cstate->current_fh);
> > 	return status;
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 91c9ea09f921..e7a18bedf499 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -458,6 +458,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 	host_err = notify_change(&init_user_ns, dentry, iap, NULL);
> > 
> > out_unlock:
> > +	if (attr->label && attr->label->len)
> > +		attr->label_failed = security_inode_setsecctx(
> > +			dentry, attr->label->data, attr->label->len);
> > 	fh_unlock(fhp);
> > 	if (size_change)
> > 		put_write_access(inode);
> > @@ -496,32 +499,6 @@ int nfsd4_is_junction(struct dentry *dentry)
> > 		return 0;
> > 	return 1;
> > }
> > -#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> > -__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > -		struct xdr_netobj *label)
> > -{
> > -	__be32 error;
> > -	int host_error;
> > -	struct dentry *dentry;
> > -
> > -	error = fh_verify(rqstp, fhp, 0 /* S_IFREG */, NFSD_MAY_SATTR);
> > -	if (error)
> > -		return error;
> > -
> > -	dentry = fhp->fh_dentry;
> > -
> > -	inode_lock(d_inode(dentry));
> > -	host_error = security_inode_setsecctx(dentry, label->data, label->len);
> > -	inode_unlock(d_inode(dentry));
> > -	return nfserrno(host_error);
> > -}
> > -#else
> > -__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > -		struct xdr_netobj *label)
> > -{
> > -	return nfserr_notsupp;
> > -}
> > -#endif
> > 
> > static struct nfsd4_compound_state *nfsd4_get_cstate(struct svc_rqst *rqstp)
> > {
> > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> > index f3f43ca3ac6b..8464e04af1ea 100644
> > --- a/fs/nfsd/vfs.h
> > +++ b/fs/nfsd/vfs.h
> > @@ -44,6 +44,8 @@ typedef int (*nfsd_filldir_t)(void *, const char *, int, loff_t, u64, unsigned);
> > /* nfsd/vfs.c */
> > struct nfsd_attrs {
> > 	struct iattr *iattr;
> > +	struct xdr_netobj *label;
> > +	int label_failed;
> > };
> > 
> > int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
> > @@ -57,8 +59,6 @@ __be32		nfsd_setattr(struct svc_rqst *, struct svc_fh *,
> > 				struct nfsd_attrs *, int, time64_t);
> > int nfsd_mountpoint(struct dentry *, struct svc_export *);
> > #ifdef CONFIG_NFSD_V4
> > -__be32          nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
> > -		    struct xdr_netobj *);
> > __be32		nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *,
> > 				    struct file *, loff_t, loff_t, int);
> > __be32		nfsd4_clone_file_range(struct svc_rqst *rqstp,
> > 
> > 
> 
> --
> Chuck Lever
> 
> 
> 
> 

[PATCH 07/13] NFSD: change nfsd_create()/nfsd_symlink() to unlock directory before returning.

[NeilBrown]

nfsd_create() usually returns with the directory still locked.
nfsd_symlink() usually returns with it unlocked.  This is clumsy.

Until recently nfsd_create() needed to keep the directory locked until
ACLs and security label had been set.  These are now set inside
nfsd_create() (in nfsd_setattr()) so this need is gone.

So change nfsd_create() and nfsd_symlink() to always unlock, and remove
any fh_unlock() calls that follow calls to these functions.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs3proc.c |    2 --
 fs/nfsd/nfs4proc.c |    2 --
 fs/nfsd/vfs.c      |   38 +++++++++++++++++++++-----------------
 3 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 5e369096e42f..0060a89997d4 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -382,7 +382,6 @@ nfsd3_proc_mkdir(struct svc_rqst *rqstp)
 	fh_init(&resp->fh, NFS3_FHSIZE);
 	resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
 				   &attrs, S_IFDIR, 0, &resp->fh);
-	fh_unlock(&resp->dirfh);
 	return rpc_success;
 }
 
@@ -459,7 +458,6 @@ nfsd3_proc_mknod(struct svc_rqst *rqstp)
 	type = nfs3_ftypes[argp->ftype];
 	resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
 				   &attrs, type, rdev, &resp->fh);
-	fh_unlock(&resp->dirfh);
 out:
 	return rpc_success;
 }
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index c49cd04cb567..915c9457a571 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -823,8 +823,6 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		create->cr_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
 	if (attrs.label_failed)
 		create->cr_bmval[0] &= ~FATTR4_WORD0_ACL;
-
-	fh_unlock(&cstate->current_fh);
 	set_change_info(&create->cr_cinfo, &cstate->current_fh);
 	fh_dup2(&cstate->current_fh, &resfh);
 out:
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 4bb05586a258..877331fabae0 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1378,8 +1378,10 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	fh_lock_nested(fhp, I_MUTEX_PARENT);
 	dchild = lookup_one_len(fname, dentry, flen);
 	host_err = PTR_ERR(dchild);
-	if (IS_ERR(dchild))
-		return nfserrno(host_err);
+	if (IS_ERR(dchild)) {
+		err = nfserrno(host_err);
+		goto out_unlock;
+	}
 	err = fh_compose(resfhp, fhp->fh_export, dchild, fhp);
 	/*
 	 * We unconditionally drop our ref to dchild as fh_compose will have
@@ -1387,9 +1389,12 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	 */
 	dput(dchild);
 	if (err)
-		return err;
-	return nfsd_create_locked(rqstp, fhp, fname, flen, attrs, type,
-					rdev, resfhp);
+		goto out_unlock;
+	err = nfsd_create_locked(rqstp, fhp, fname, flen, attrs, type,
+				 rdev, resfhp);
+out_unlock:
+	fh_unlock(fhp);
+	return err;
 }
 
 /*
@@ -1456,16 +1461,19 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		goto out;
 
 	host_err = fh_want_write(fhp);
-	if (host_err)
-		goto out_nfserr;
+	if (host_err) {
+		err = nfserrno(host_err);
+		goto out;
+	}
 
 	fh_lock(fhp);
 	dentry = fhp->fh_dentry;
 	dnew = lookup_one_len(fname, dentry, flen);
-	host_err = PTR_ERR(dnew);
-	if (IS_ERR(dnew))
-		goto out_nfserr;
-
+	if (IS_ERR(dnew)) {
+		err = nfserrno(PTR_ERR(dnew));
+		fh_unlock(fhp);
+		goto out_drop_write;
+	}
 	host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
 	err = nfserrno(host_err);
 	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
@@ -1474,16 +1482,12 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	fh_unlock(fhp);
 	if (!err)
 		err = nfserrno(commit_metadata(fhp));
-	fh_drop_write(fhp);
-
 	dput(dnew);
 	if (err==0) err = cerr;
+out_drop_write:
+	fh_drop_write(fhp);
 out:
 	return err;
-
-out_nfserr:
-	err = nfserrno(host_err);
-	goto out;
 }
 
 /*

[PATCH 08/13] NFSD: always drop directory lock in nfsd_unlink()

[NeilBrown]

Some error paths in nfsd_unlink() allow it to exit without unlocking the
directory.  This is not a problem in practice as the directory will be
locked with an fh_put(), but it is untidy and potentially confusing.

This allows us to remove all the fh_unlock() calls that are immediately
after nfsd_unlink() calls.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs3proc.c |    2 --
 fs/nfsd/nfs4proc.c |    4 +---
 fs/nfsd/vfs.c      |    7 +++++--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 0060a89997d4..774e4a2ab9b1 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -480,7 +480,6 @@ nfsd3_proc_remove(struct svc_rqst *rqstp)
 	fh_copy(&resp->fh, &argp->fh);
 	resp->status = nfsd_unlink(rqstp, &resp->fh, -S_IFDIR,
 				   argp->name, argp->len);
-	fh_unlock(&resp->fh);
 	return rpc_success;
 }
 
@@ -501,7 +500,6 @@ nfsd3_proc_rmdir(struct svc_rqst *rqstp)
 	fh_copy(&resp->fh, &argp->fh);
 	resp->status = nfsd_unlink(rqstp, &resp->fh, S_IFDIR,
 				   argp->name, argp->len);
-	fh_unlock(&resp->fh);
 	return rpc_success;
 }
 
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 915c9457a571..1aa6ae4ec2f5 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1002,10 +1002,8 @@ nfsd4_remove(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		return nfserr_grace;
 	status = nfsd_unlink(rqstp, &cstate->current_fh, 0,
 			     remove->rm_name, remove->rm_namelen);
-	if (!status) {
-		fh_unlock(&cstate->current_fh);
+	if (!status)
 		set_change_info(&remove->rm_cinfo, &cstate->current_fh);
-	}
 	return status;
 }
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 877331fabae0..f15ceed6d184 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1742,12 +1742,12 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 	rdentry = lookup_one_len(fname, dentry, flen);
 	host_err = PTR_ERR(rdentry);
 	if (IS_ERR(rdentry))
-		goto out_drop_write;
+		goto out_unlock;
 
 	if (d_really_is_negative(rdentry)) {
 		dput(rdentry);
 		host_err = -ENOENT;
-		goto out_drop_write;
+		goto out_unlock;
 	}
 	rinode = d_inode(rdentry);
 	ihold(rinode);
@@ -1785,6 +1785,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 	}
 out:
 	return err;
+out_unlock:
+	fh_unlock(fhp);
+	goto out_drop_write;
 }
 
 /*

Re: [PATCH 00/13] NFSD: clean up locking

[Jeff Layton]

On Tue, 2022-07-26 at 16:45 +1000, NeilBrown wrote:
> This is the latest version of my series to clean up locking -
> particularly of directories - in preparation for proposed patches which
> change how directory locking works across the VFS.
> 
> I've included Jeff's patches to validate the dentry after getting a
> delegation.  The second patch has been changed quite a bit to use
> nfsd_lookup_dentry(). I've left Jeff's From: line in place - let me know
> if you'd rather I change it.
> 

That's fine.

> Setting of ACLs and security labels has been moved from nfs4 code to
> nfsd_setattr() which allows quite a lot of code cleanup.
> 
> I think I've addressed all the concerns that have been raised, though
> maybe not in the way that was suggested.
> 
> I've tested this with cthon tests over v2, v3, v4.0, v4.1, and xfstests
> on v3 and v4.1, and pynfs 4.0, 4.1.  No problems appeared.
> 
> Thanks,
> NeilBrown
> 
> 
> ---
> 
> Jeff Layton (2):
>       NFSD: drop fh argument from alloc_init_deleg
>       NFSD: verify the opened dentry after setting a delegation
> 
> NeilBrown (11):
>       NFSD: introduce struct nfsd_attrs
>       NFSD: set attributes when creating symlinks
>       NFSD: add security label to struct nfsd_attrs
>       NFSD: add posix ACLs to struct nfsd_attrs
>       NFSD: change nfsd_create()/nfsd_symlink() to unlock directory before returning.
>       NFSD: always drop directory lock in nfsd_unlink()
>       NFSD: only call fh_unlock() once in nfsd_link()
>       NFSD: reduce locking in nfsd_lookup()
>       NFSD: use explicit lock/unlock for directory ops
>       NFSD: use (un)lock_inode instead of fh_(un)lock for file operations
>       NFSD: discard fh_locked flag and fh_lock/fh_unlock
> 

It looks like the last 5 patches are missing from the posting
(everything after patch #8).

> 
>  fs/nfsd/acl.h       |   6 +-
>  fs/nfsd/nfs2acl.c   |   6 +-
>  fs/nfsd/nfs3acl.c   |   4 +-
>  fs/nfsd/nfs3proc.c  |  25 ++---
>  fs/nfsd/nfs4acl.c   |  46 ++-------
>  fs/nfsd/nfs4proc.c  | 153 ++++++++++++-----------------
>  fs/nfsd/nfs4state.c |  71 +++++++++++---
>  fs/nfsd/nfsfh.c     |  22 ++++-
>  fs/nfsd/nfsfh.h     |  58 +----------
>  fs/nfsd/nfsproc.c   |  19 ++--
>  fs/nfsd/vfs.c       | 230 +++++++++++++++++++++-----------------------
>  fs/nfsd/vfs.h       |  31 ++++--
>  fs/nfsd/xdr4.h      |   1 +
>  13 files changed, 314 insertions(+), 358 deletions(-)
> 
> --
> Signature
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 00/13] NFSD: clean up locking

[Chuck Lever III]

> On Jul 27, 2022, at 11:50 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Tue, 2022-07-26 at 16:45 +1000, NeilBrown wrote:
>> This is the latest version of my series to clean up locking -
>> particularly of directories - in preparation for proposed patches which
>> change how directory locking works across the VFS.
>> 
>> I've included Jeff's patches to validate the dentry after getting a
>> delegation.  The second patch has been changed quite a bit to use
>> nfsd_lookup_dentry(). I've left Jeff's From: line in place - let me know
>> if you'd rather I change it.
>> 
> 
> That's fine.
> 
>> Setting of ACLs and security labels has been moved from nfs4 code to
>> nfsd_setattr() which allows quite a lot of code cleanup.
>> 
>> I think I've addressed all the concerns that have been raised, though
>> maybe not in the way that was suggested.
>> 
>> I've tested this with cthon tests over v2, v3, v4.0, v4.1, and xfstests
>> on v3 and v4.1, and pynfs 4.0, 4.1.  No problems appeared.
>> 
>> Thanks,
>> NeilBrown
>> 
>> 
>> ---
>> 
>> Jeff Layton (2):
>>      NFSD: drop fh argument from alloc_init_deleg
>>      NFSD: verify the opened dentry after setting a delegation
>> 
>> NeilBrown (11):
>>      NFSD: introduce struct nfsd_attrs
>>      NFSD: set attributes when creating symlinks
>>      NFSD: add security label to struct nfsd_attrs
>>      NFSD: add posix ACLs to struct nfsd_attrs
>>      NFSD: change nfsd_create()/nfsd_symlink() to unlock directory before returning.
>>      NFSD: always drop directory lock in nfsd_unlink()
>>      NFSD: only call fh_unlock() once in nfsd_link()
>>      NFSD: reduce locking in nfsd_lookup()
>>      NFSD: use explicit lock/unlock for directory ops
>>      NFSD: use (un)lock_inode instead of fh_(un)lock for file operations
>>      NFSD: discard fh_locked flag and fh_lock/fh_unlock
>> 
> 
> It looks like the last 5 patches are missing from the posting
> (everything after patch #8).

Fwiw, I don't see them in my inbox either, and they don't appear on lore.


>> fs/nfsd/acl.h       |   6 +-
>> fs/nfsd/nfs2acl.c   |   6 +-
>> fs/nfsd/nfs3acl.c   |   4 +-
>> fs/nfsd/nfs3proc.c  |  25 ++---
>> fs/nfsd/nfs4acl.c   |  46 ++-------
>> fs/nfsd/nfs4proc.c  | 153 ++++++++++++-----------------
>> fs/nfsd/nfs4state.c |  71 +++++++++++---
>> fs/nfsd/nfsfh.c     |  22 ++++-
>> fs/nfsd/nfsfh.h     |  58 +----------
>> fs/nfsd/nfsproc.c   |  19 ++--
>> fs/nfsd/vfs.c       | 230 +++++++++++++++++++++-----------------------
>> fs/nfsd/vfs.h       |  31 ++++--
>> fs/nfsd/xdr4.h      |   1 +
>> 13 files changed, 314 insertions(+), 358 deletions(-)
>> 
>> --
>> Signature
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever

Re: [PATCH 00/13] NFSD: clean up locking

[NeilBrown]

On Thu, 28 Jul 2022, Jeff Layton wrote:
> On Tue, 2022-07-26 at 16:45 +1000, NeilBrown wrote:
> > This is the latest version of my series to clean up locking -
> > particularly of directories - in preparation for proposed patches which
> > change how directory locking works across the VFS.
> > 
> > I've included Jeff's patches to validate the dentry after getting a
> > delegation.  The second patch has been changed quite a bit to use
> > nfsd_lookup_dentry(). I've left Jeff's From: line in place - let me know
> > if you'd rather I change it.
> > 
> 
> That's fine.
> 
> > Setting of ACLs and security labels has been moved from nfs4 code to
> > nfsd_setattr() which allows quite a lot of code cleanup.
> > 
> > I think I've addressed all the concerns that have been raised, though
> > maybe not in the way that was suggested.
> > 
> > I've tested this with cthon tests over v2, v3, v4.0, v4.1, and xfstests
> > on v3 and v4.1, and pynfs 4.0, 4.1.  No problems appeared.
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> > ---
> > 
> > Jeff Layton (2):
> >       NFSD: drop fh argument from alloc_init_deleg
> >       NFSD: verify the opened dentry after setting a delegation
> > 
> > NeilBrown (11):
> >       NFSD: introduce struct nfsd_attrs
> >       NFSD: set attributes when creating symlinks
> >       NFSD: add security label to struct nfsd_attrs
> >       NFSD: add posix ACLs to struct nfsd_attrs
> >       NFSD: change nfsd_create()/nfsd_symlink() to unlock directory before returning.
> >       NFSD: always drop directory lock in nfsd_unlink()
> >       NFSD: only call fh_unlock() once in nfsd_link()
> >       NFSD: reduce locking in nfsd_lookup()
> >       NFSD: use explicit lock/unlock for directory ops
> >       NFSD: use (un)lock_inode instead of fh_(un)lock for file operations
> >       NFSD: discard fh_locked flag and fh_lock/fh_unlock
> > 
> 
> It looks like the last 5 patches are missing from the posting
> (everything after patch #8).
> 

Sorry.  You should have them now.

NeilBrown

Re: [PATCH 00/13] NFSD: clean up locking

[Chuck Lever III]

> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> 
> This is the latest version of my series to clean up locking -
> particularly of directories - in preparation for proposed patches which
> change how directory locking works across the VFS.
> 
> I've included Jeff's patches to validate the dentry after getting a
> delegation.  The second patch has been changed quite a bit to use
> nfsd_lookup_dentry(). I've left Jeff's From: line in place - let me know
> if you'd rather I change it.
> 
> Setting of ACLs and security labels has been moved from nfs4 code to
> nfsd_setattr() which allows quite a lot of code cleanup.
> 
> I think I've addressed all the concerns that have been raised, though
> maybe not in the way that was suggested.
> 
> I've tested this with cthon tests over v2, v3, v4.0, v4.1, and xfstests
> on v3 and v4.1, and pynfs 4.0, 4.1.  No problems appeared.
> 
> Thanks,
> NeilBrown

Hi Neil-

No objections to this round, looks like a very good set of clean-ups.

I've also resurrected NFSv2 on my test systems, and captured a baseline
without these patches applied.

There are a number of cosmetic issues I found, posting those separately.
If you trust me, I can take care of those here, or you can submit a
v3 (v4?).


> ---
> 
> Jeff Layton (2):
>      NFSD: drop fh argument from alloc_init_deleg
>      NFSD: verify the opened dentry after setting a delegation
> 
> NeilBrown (11):
>      NFSD: introduce struct nfsd_attrs
>      NFSD: set attributes when creating symlinks
>      NFSD: add security label to struct nfsd_attrs
>      NFSD: add posix ACLs to struct nfsd_attrs
>      NFSD: change nfsd_create()/nfsd_symlink() to unlock directory before returning.
>      NFSD: always drop directory lock in nfsd_unlink()
>      NFSD: only call fh_unlock() once in nfsd_link()
>      NFSD: reduce locking in nfsd_lookup()
>      NFSD: use explicit lock/unlock for directory ops
>      NFSD: use (un)lock_inode instead of fh_(un)lock for file operations
>      NFSD: discard fh_locked flag and fh_lock/fh_unlock
> 
> 
> fs/nfsd/acl.h       |   6 +-
> fs/nfsd/nfs2acl.c   |   6 +-
> fs/nfsd/nfs3acl.c   |   4 +-
> fs/nfsd/nfs3proc.c  |  25 ++---
> fs/nfsd/nfs4acl.c   |  46 ++-------
> fs/nfsd/nfs4proc.c  | 153 ++++++++++++-----------------
> fs/nfsd/nfs4state.c |  71 +++++++++++---
> fs/nfsd/nfsfh.c     |  22 ++++-
> fs/nfsd/nfsfh.h     |  58 +----------
> fs/nfsd/nfsproc.c   |  19 ++--
> fs/nfsd/vfs.c       | 230 +++++++++++++++++++++-----------------------
> fs/nfsd/vfs.h       |  31 ++++--
> fs/nfsd/xdr4.h      |   1 +
> 13 files changed, 314 insertions(+), 358 deletions(-)
> 
> --
> Signature
> 

--
Chuck Lever

Re: [PATCH 00/13] NFSD: clean up locking

[NeilBrown]

On Fri, 29 Jul 2022, Chuck Lever III wrote:
> 
> > On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
> > 
> > This is the latest version of my series to clean up locking -
> > particularly of directories - in preparation for proposed patches which
> > change how directory locking works across the VFS.
> > 
> > I've included Jeff's patches to validate the dentry after getting a
> > delegation.  The second patch has been changed quite a bit to use
> > nfsd_lookup_dentry(). I've left Jeff's From: line in place - let me know
> > if you'd rather I change it.
> > 
> > Setting of ACLs and security labels has been moved from nfs4 code to
> > nfsd_setattr() which allows quite a lot of code cleanup.
> > 
> > I think I've addressed all the concerns that have been raised, though
> > maybe not in the way that was suggested.
> > 
> > I've tested this with cthon tests over v2, v3, v4.0, v4.1, and xfstests
> > on v3 and v4.1, and pynfs 4.0, 4.1.  No problems appeared.
> > 
> > Thanks,
> > NeilBrown
> 
> Hi Neil-
> 
> No objections to this round, looks like a very good set of clean-ups.
> 
> I've also resurrected NFSv2 on my test systems, and captured a baseline
> without these patches applied.
> 
> There are a number of cosmetic issues I found, posting those separately.
> If you trust me, I can take care of those here, or you can submit a
> v3 (v4?).

I would love for you to make those changes yourself!
As I noted separately there is a bug : nfserrno() needed where you
suggested __be32.
All others are cosmetic and I trust you to fix those up however seems
best.

Thanks,
NeilBrown