[PATCH v2 0/2] Fix NFSv3 SETATTR behaviours

[PATCH v2 0/2] Fix NFSv3 SETATTR behaviours

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Fix recent regressions in nfsd_setattr(), and fix the "guarded SETATTR"
behaviour for NFSv3.

Trond Myklebust (2):
  nfsd: Fix a regression in nfsd_setattr()
  nfsd: Fix NFSv3 atomicity bugs in nfsd_setattr()

 fs/nfsd/nfs3proc.c  |  6 ++++--
 fs/nfsd/nfs3xdr.c   |  5 +----
 fs/nfsd/nfs4proc.c  |  7 +++++--
 fs/nfsd/nfs4state.c |  2 +-
 fs/nfsd/nfsproc.c   |  6 +++---
 fs/nfsd/vfs.c       | 29 ++++++++++++++++++++---------
 fs/nfsd/vfs.h       |  2 +-
 fs/nfsd/xdr3.h      |  2 +-
 8 files changed, 36 insertions(+), 23 deletions(-)

-- 
2.43.1

[PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes behaviour
when doing a SETATTR rpc call by stripping out the calls to
fh_fill_pre_attrs() and fh_fill_post_attrs().

Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of fh_(un)lock for file operations")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfsd/nfs4proc.c | 4 ++++
 fs/nfsd/vfs.c      | 9 +++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 14712fa08f76..e6d8624efc83 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	};
 	struct inode *inode;
 	__be32 status = nfs_ok;
+	bool save_no_wcc;
 	int err;
 
 	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
@@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	if (status)
 		goto out;
+	save_no_wcc = cstate->current_fh.fh_no_wcc;
+	cstate->current_fh.fh_no_wcc = true;
 	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
 				0, (time64_t)0);
+	cstate->current_fh.fh_no_wcc = save_no_wcc;
 	if (!status)
 		status = nfserrno(attrs.na_labelerr);
 	if (!status)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 6e7e37192461..58fab461bc00 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -497,7 +497,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	int		accmode = NFSD_MAY_SATTR;
 	umode_t		ftype = 0;
 	__be32		err;
-	int		host_err;
+	int		host_err = 0;
 	bool		get_write_count;
 	bool		size_change = (iap->ia_valid & ATTR_SIZE);
 	int		retries;
@@ -555,6 +555,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	}
 
 	inode_lock(inode);
+	err = fh_fill_pre_attrs(fhp);
+	if (err)
+		goto out_unlock;
 	for (retries = 1;;) {
 		struct iattr attrs;
 
@@ -582,13 +585,15 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
 						dentry, ACL_TYPE_DEFAULT,
 						attr->na_dpacl);
+	fh_fill_post_attrs(fhp);
+out_unlock:
 	inode_unlock(inode);
 	if (size_change)
 		put_write_access(inode);
 out:
 	if (!host_err)
 		host_err = commit_metadata(fhp);
-	return nfserrno(host_err);
+	return err != 0 ? err : nfserrno(host_err);
 }
 
 #if defined(CONFIG_NFSD_V4)
-- 
2.43.1

[PATCH v2 2/2] nfsd: Fix NFSv3 atomicity bugs in nfsd_setattr()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

The main point of the guarded SETATTR is to prevent races with other
WRITE and SETATTR calls. That requires that the check of the guard time
against the inode ctime be done after taking the inode lock.

Furthermore, we need to take into account the 32-bit nature of
timestamps in NFSv3, and the possibility that files may change at a
faster rate than once a second.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfsd/nfs3proc.c  |  6 ++++--
 fs/nfsd/nfs3xdr.c   |  5 +----
 fs/nfsd/nfs4proc.c  |  3 +--
 fs/nfsd/nfs4state.c |  2 +-
 fs/nfsd/nfsproc.c   |  6 +++---
 fs/nfsd/vfs.c       | 20 +++++++++++++-------
 fs/nfsd/vfs.h       |  2 +-
 fs/nfsd/xdr3.h      |  2 +-
 8 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index b78eceebd945..dfcc957e460d 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -71,13 +71,15 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp)
 	struct nfsd_attrs attrs = {
 		.na_iattr	= &argp->attrs,
 	};
+	const struct timespec64 *guardtime = NULL;
 
 	dprintk("nfsd: SETATTR(3)  %s\n",
 				SVCFH_fmt(&argp->fh));
 
 	fh_copy(&resp->fh, &argp->fh);
-	resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs,
-				    argp->check_guard, argp->guardtime);
+	if (argp->check_guard)
+		guardtime = &argp->guardtime;
+	resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs, guardtime);
 	return rpc_success;
 }
 
diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index f32128955ec8..a7a07470c1f8 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -295,17 +295,14 @@ svcxdr_decode_sattr3(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 static bool
 svcxdr_decode_sattrguard3(struct xdr_stream *xdr, struct nfsd3_sattrargs *args)
 {
-	__be32 *p;
 	u32 check;
 
 	if (xdr_stream_decode_bool(xdr, &check) < 0)
 		return false;
 	if (check) {
-		p = xdr_inline_decode(xdr, XDR_UNIT * 2);
-		if (!p)
+		if (!svcxdr_decode_nfstime3(xdr, &args->guardtime))
 			return false;
 		args->check_guard = 1;
-		args->guardtime = be32_to_cpup(p);
 	} else
 		args->check_guard = 0;
 
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index e6d8624efc83..ae48690f4c7c 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1171,8 +1171,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	save_no_wcc = cstate->current_fh.fh_no_wcc;
 	cstate->current_fh.fh_no_wcc = true;
-	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
-				0, (time64_t)0);
+	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs, NULL);
 	cstate->current_fh.fh_no_wcc = save_no_wcc;
 	if (!status)
 		status = nfserrno(attrs.na_labelerr);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2fa54cfd4882..538edd85b51e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5191,7 +5191,7 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
 		return 0;
 	if (!(open->op_share_access & NFS4_SHARE_ACCESS_WRITE))
 		return nfserr_inval;
-	return nfsd_setattr(rqstp, fh, &attrs, 0, (time64_t)0);
+	return nfsd_setattr(rqstp, fh, &attrs, NULL);
 }
 
 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 a7315928a760..36370b957b63 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -103,7 +103,7 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
 		}
 	}
 
-	resp->status = nfsd_setattr(rqstp, fhp, &attrs, 0, (time64_t)0);
+	resp->status = nfsd_setattr(rqstp, fhp, &attrs, NULL);
 	if (resp->status != nfs_ok)
 		goto out;
 
@@ -390,8 +390,8 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 		 */
 		attr->ia_valid &= ATTR_SIZE;
 		if (attr->ia_valid)
-			resp->status = nfsd_setattr(rqstp, newfhp, &attrs, 0,
-						    (time64_t)0);
+			resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
+						    NULL);
 	}
 
 out_unlock:
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 58fab461bc00..3602e35e83d2 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -476,7 +476,6 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
  * @rqstp: controlling RPC transaction
  * @fhp: filehandle of target
  * @attr: attributes to set
- * @check_guard: set to 1 if guardtime is a valid timestamp
  * @guardtime: do not act if ctime.tv_sec does not match this timestamp
  *
  * This call may adjust the contents of @attr (in particular, this
@@ -488,8 +487,7 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
  */
 __be32
 nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
-	     struct nfsd_attrs *attr,
-	     int check_guard, time64_t guardtime)
+	     struct nfsd_attrs *attr, const struct timespec64 *guardtime)
 {
 	struct dentry	*dentry;
 	struct inode	*inode;
@@ -538,9 +536,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	nfsd_sanitize_attrs(inode, iap);
 
-	if (check_guard && guardtime != inode_get_ctime_sec(inode))
-		return nfserr_notsync;
-
 	/*
 	 * The size case is special, it changes the file in addition to the
 	 * attributes, and file systems don't expect it to be mixed with
@@ -558,6 +553,16 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	err = fh_fill_pre_attrs(fhp);
 	if (err)
 		goto out_unlock;
+
+	if (guardtime) {
+		struct timespec64 ctime = inode_get_ctime(inode);
+		if ((u32)guardtime->tv_sec != (u32)ctime.tv_sec ||
+		    guardtime->tv_nsec != ctime.tv_nsec) {
+			err = nfserr_notsync;
+			goto out_fill_attrs;
+		}
+	}
+
 	for (retries = 1;;) {
 		struct iattr attrs;
 
@@ -585,6 +590,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
 						dentry, ACL_TYPE_DEFAULT,
 						attr->na_dpacl);
+out_fill_attrs:
 	fh_fill_post_attrs(fhp);
 out_unlock:
 	inode_unlock(inode);
@@ -1409,7 +1415,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, attrs, 0, (time64_t)0);
+		status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
 	else
 		status = nfserrno(commit_metadata(resfhp));
 
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 702fbc4483bf..7d77303ef5f7 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -69,7 +69,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 nfsd_attrs *, int, time64_t);
+			     struct nfsd_attrs *, const struct timespec64 *);
 int nfsd_mountpoint(struct dentry *, struct svc_export *);
 #ifdef CONFIG_NFSD_V4
 __be32		nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *,
diff --git a/fs/nfsd/xdr3.h b/fs/nfsd/xdr3.h
index 03fe4e21306c..522067b7fd75 100644
--- a/fs/nfsd/xdr3.h
+++ b/fs/nfsd/xdr3.h
@@ -14,7 +14,7 @@ struct nfsd3_sattrargs {
 	struct svc_fh		fh;
 	struct iattr		attrs;
 	int			check_guard;
-	time64_t		guardtime;
+	struct timespec64	guardtime;
 };
 
 struct nfsd3_diropargs {
-- 
2.43.1

Re: [PATCH v2 2/2] nfsd: Fix NFSv3 atomicity bugs in nfsd_setattr()

[Jeff Layton]

On Thu, 2024-02-15 at 20:24 -0500, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> The main point of the guarded SETATTR is to prevent races with other
> WRITE and SETATTR calls. That requires that the check of the guard time
> against the inode ctime be done after taking the inode lock.
> 
> Furthermore, we need to take into account the 32-bit nature of
> timestamps in NFSv3, and the possibility that files may change at a
> faster rate than once a second.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfsd/nfs3proc.c  |  6 ++++--
>  fs/nfsd/nfs3xdr.c   |  5 +----
>  fs/nfsd/nfs4proc.c  |  3 +--
>  fs/nfsd/nfs4state.c |  2 +-
>  fs/nfsd/nfsproc.c   |  6 +++---
>  fs/nfsd/vfs.c       | 20 +++++++++++++-------
>  fs/nfsd/vfs.h       |  2 +-
>  fs/nfsd/xdr3.h      |  2 +-
>  8 files changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index b78eceebd945..dfcc957e460d 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -71,13 +71,15 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp)
>  	struct nfsd_attrs attrs = {
>  		.na_iattr	= &argp->attrs,
>  	};
> +	const struct timespec64 *guardtime = NULL;
>  
>  	dprintk("nfsd: SETATTR(3)  %s\n",
>  				SVCFH_fmt(&argp->fh));
>  
>  	fh_copy(&resp->fh, &argp->fh);
> -	resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs,
> -				    argp->check_guard, argp->guardtime);
> +	if (argp->check_guard)
> +		guardtime = &argp->guardtime;
> +	resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs, guardtime);
>  	return rpc_success;
>  }
>  
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index f32128955ec8..a7a07470c1f8 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -295,17 +295,14 @@ svcxdr_decode_sattr3(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  static bool
>  svcxdr_decode_sattrguard3(struct xdr_stream *xdr, struct nfsd3_sattrargs *args)
>  {
> -	__be32 *p;
>  	u32 check;
>  
>  	if (xdr_stream_decode_bool(xdr, &check) < 0)
>  		return false;
>  	if (check) {
> -		p = xdr_inline_decode(xdr, XDR_UNIT * 2);
> -		if (!p)
> +		if (!svcxdr_decode_nfstime3(xdr, &args->guardtime))
>  			return false;
>  		args->check_guard = 1;
> -		args->guardtime = be32_to_cpup(p);
>  	} else
>  		args->check_guard = 0;
>  
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e6d8624efc83..ae48690f4c7c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1171,8 +1171,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		goto out;
>  	save_no_wcc = cstate->current_fh.fh_no_wcc;
>  	cstate->current_fh.fh_no_wcc = true;
> -	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> -				0, (time64_t)0);
> +	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs, NULL);
>  	cstate->current_fh.fh_no_wcc = save_no_wcc;
>  	if (!status)
>  		status = nfserrno(attrs.na_labelerr);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2fa54cfd4882..538edd85b51e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5191,7 +5191,7 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
>  		return 0;
>  	if (!(open->op_share_access & NFS4_SHARE_ACCESS_WRITE))
>  		return nfserr_inval;
> -	return nfsd_setattr(rqstp, fh, &attrs, 0, (time64_t)0);
> +	return nfsd_setattr(rqstp, fh, &attrs, NULL);
>  }
>  
>  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 a7315928a760..36370b957b63 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -103,7 +103,7 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
>  		}
>  	}
>  
> -	resp->status = nfsd_setattr(rqstp, fhp, &attrs, 0, (time64_t)0);
> +	resp->status = nfsd_setattr(rqstp, fhp, &attrs, NULL);
>  	if (resp->status != nfs_ok)
>  		goto out;
>  
> @@ -390,8 +390,8 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  		 */
>  		attr->ia_valid &= ATTR_SIZE;
>  		if (attr->ia_valid)
> -			resp->status = nfsd_setattr(rqstp, newfhp, &attrs, 0,
> -						    (time64_t)0);
> +			resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
> +						    NULL);
>  	}
>  
>  out_unlock:
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 58fab461bc00..3602e35e83d2 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -476,7 +476,6 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
>   * @rqstp: controlling RPC transaction
>   * @fhp: filehandle of target
>   * @attr: attributes to set
> - * @check_guard: set to 1 if guardtime is a valid timestamp
>   * @guardtime: do not act if ctime.tv_sec does not match this timestamp
>   *
>   * This call may adjust the contents of @attr (in particular, this
> @@ -488,8 +487,7 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
>   */
>  __be32
>  nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -	     struct nfsd_attrs *attr,
> -	     int check_guard, time64_t guardtime)
> +	     struct nfsd_attrs *attr, const struct timespec64 *guardtime)
>  {
>  	struct dentry	*dentry;
>  	struct inode	*inode;
> @@ -538,9 +536,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	nfsd_sanitize_attrs(inode, iap);
>  
> -	if (check_guard && guardtime != inode_get_ctime_sec(inode))
> -		return nfserr_notsync;
> -
>  	/*
>  	 * The size case is special, it changes the file in addition to the
>  	 * attributes, and file systems don't expect it to be mixed with
> @@ -558,6 +553,16 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	err = fh_fill_pre_attrs(fhp);
>  	if (err)
>  		goto out_unlock;
> +
> +	if (guardtime) {
> +		struct timespec64 ctime = inode_get_ctime(inode);
> +		if ((u32)guardtime->tv_sec != (u32)ctime.tv_sec ||
> +		    guardtime->tv_nsec != ctime.tv_nsec) {
> +			err = nfserr_notsync;
> +			goto out_fill_attrs;
> +		}
> +	}
> +
>  	for (retries = 1;;) {
>  		struct iattr attrs;
>  
> @@ -585,6 +590,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
>  						dentry, ACL_TYPE_DEFAULT,
>  						attr->na_dpacl);
> +out_fill_attrs:
>  	fh_fill_post_attrs(fhp);
>  out_unlock:
>  	inode_unlock(inode);
> @@ -1409,7 +1415,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, attrs, 0, (time64_t)0);
> +		status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
>  	else
>  		status = nfserrno(commit_metadata(resfhp));
>  
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 702fbc4483bf..7d77303ef5f7 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -69,7 +69,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 nfsd_attrs *, int, time64_t);
> +			     struct nfsd_attrs *, const struct timespec64 *);
>  int nfsd_mountpoint(struct dentry *, struct svc_export *);
>  #ifdef CONFIG_NFSD_V4
>  __be32		nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *,
> diff --git a/fs/nfsd/xdr3.h b/fs/nfsd/xdr3.h
> index 03fe4e21306c..522067b7fd75 100644
> --- a/fs/nfsd/xdr3.h
> +++ b/fs/nfsd/xdr3.h
> @@ -14,7 +14,7 @@ struct nfsd3_sattrargs {
>  	struct svc_fh		fh;
>  	struct iattr		attrs;
>  	int			check_guard;
> -	time64_t		guardtime;
> +	struct timespec64	guardtime;
>  };
>  
>  struct nfsd3_diropargs {

I thought I sent this the other day, but...

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 2/2] nfsd: Fix NFSv3 atomicity bugs in nfsd_setattr()

[NeilBrown]

On Fri, 16 Feb 2024, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> The main point of the guarded SETATTR is to prevent races with other
> WRITE and SETATTR calls. That requires that the check of the guard time
> against the inode ctime be done after taking the inode lock.
> 
> Furthermore, we need to take into account the 32-bit nature of
> timestamps in NFSv3, and the possibility that files may change at a
> faster rate than once a second.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfsd/nfs3proc.c  |  6 ++++--
>  fs/nfsd/nfs3xdr.c   |  5 +----
>  fs/nfsd/nfs4proc.c  |  3 +--
>  fs/nfsd/nfs4state.c |  2 +-
>  fs/nfsd/nfsproc.c   |  6 +++---
>  fs/nfsd/vfs.c       | 20 +++++++++++++-------
>  fs/nfsd/vfs.h       |  2 +-
>  fs/nfsd/xdr3.h      |  2 +-
>  8 files changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index b78eceebd945..dfcc957e460d 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -71,13 +71,15 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp)
>  	struct nfsd_attrs attrs = {
>  		.na_iattr	= &argp->attrs,
>  	};
> +	const struct timespec64 *guardtime = NULL;
>  
>  	dprintk("nfsd: SETATTR(3)  %s\n",
>  				SVCFH_fmt(&argp->fh));
>  
>  	fh_copy(&resp->fh, &argp->fh);
> -	resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs,
> -				    argp->check_guard, argp->guardtime);
> +	if (argp->check_guard)
> +		guardtime = &argp->guardtime;
> +	resp->status = nfsd_setattr(rqstp, &resp->fh, &attrs, guardtime);
>  	return rpc_success;
>  }
>  
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index f32128955ec8..a7a07470c1f8 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -295,17 +295,14 @@ svcxdr_decode_sattr3(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  static bool
>  svcxdr_decode_sattrguard3(struct xdr_stream *xdr, struct nfsd3_sattrargs *args)
>  {
> -	__be32 *p;
>  	u32 check;
>  
>  	if (xdr_stream_decode_bool(xdr, &check) < 0)
>  		return false;
>  	if (check) {
> -		p = xdr_inline_decode(xdr, XDR_UNIT * 2);
> -		if (!p)
> +		if (!svcxdr_decode_nfstime3(xdr, &args->guardtime))
>  			return false;
>  		args->check_guard = 1;
> -		args->guardtime = be32_to_cpup(p);
>  	} else
>  		args->check_guard = 0;
>  
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e6d8624efc83..ae48690f4c7c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1171,8 +1171,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		goto out;
>  	save_no_wcc = cstate->current_fh.fh_no_wcc;
>  	cstate->current_fh.fh_no_wcc = true;
> -	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> -				0, (time64_t)0);
> +	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs, NULL);
>  	cstate->current_fh.fh_no_wcc = save_no_wcc;
>  	if (!status)
>  		status = nfserrno(attrs.na_labelerr);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2fa54cfd4882..538edd85b51e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5191,7 +5191,7 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
>  		return 0;
>  	if (!(open->op_share_access & NFS4_SHARE_ACCESS_WRITE))
>  		return nfserr_inval;
> -	return nfsd_setattr(rqstp, fh, &attrs, 0, (time64_t)0);
> +	return nfsd_setattr(rqstp, fh, &attrs, NULL);
>  }
>  
>  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 a7315928a760..36370b957b63 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -103,7 +103,7 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
>  		}
>  	}
>  
> -	resp->status = nfsd_setattr(rqstp, fhp, &attrs, 0, (time64_t)0);
> +	resp->status = nfsd_setattr(rqstp, fhp, &attrs, NULL);
>  	if (resp->status != nfs_ok)
>  		goto out;
>  
> @@ -390,8 +390,8 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  		 */
>  		attr->ia_valid &= ATTR_SIZE;
>  		if (attr->ia_valid)
> -			resp->status = nfsd_setattr(rqstp, newfhp, &attrs, 0,
> -						    (time64_t)0);
> +			resp->status = nfsd_setattr(rqstp, newfhp, &attrs,
> +						    NULL);
>  	}
>  
>  out_unlock:
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 58fab461bc00..3602e35e83d2 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -476,7 +476,6 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
>   * @rqstp: controlling RPC transaction
>   * @fhp: filehandle of target
>   * @attr: attributes to set
> - * @check_guard: set to 1 if guardtime is a valid timestamp
>   * @guardtime: do not act if ctime.tv_sec does not match this timestamp
>   *
>   * This call may adjust the contents of @attr (in particular, this
> @@ -488,8 +487,7 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
>   */
>  __be32
>  nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -	     struct nfsd_attrs *attr,
> -	     int check_guard, time64_t guardtime)
> +	     struct nfsd_attrs *attr, const struct timespec64 *guardtime)
>  {
>  	struct dentry	*dentry;
>  	struct inode	*inode;
> @@ -538,9 +536,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	nfsd_sanitize_attrs(inode, iap);
>  
> -	if (check_guard && guardtime != inode_get_ctime_sec(inode))
> -		return nfserr_notsync;
> -
>  	/*
>  	 * The size case is special, it changes the file in addition to the
>  	 * attributes, and file systems don't expect it to be mixed with
> @@ -558,6 +553,16 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	err = fh_fill_pre_attrs(fhp);
>  	if (err)
>  		goto out_unlock;
> +
> +	if (guardtime) {
> +		struct timespec64 ctime = inode_get_ctime(inode);
> +		if ((u32)guardtime->tv_sec != (u32)ctime.tv_sec ||
> +		    guardtime->tv_nsec != ctime.tv_nsec) {
> +			err = nfserr_notsync;
> +			goto out_fill_attrs;
> +		}
> +	}
> +
>  	for (retries = 1;;) {
>  		struct iattr attrs;
>  
> @@ -585,6 +590,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
>  						dentry, ACL_TYPE_DEFAULT,
>  						attr->na_dpacl);
> +out_fill_attrs:
>  	fh_fill_post_attrs(fhp);
>  out_unlock:
>  	inode_unlock(inode);
> @@ -1409,7 +1415,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, attrs, 0, (time64_t)0);
> +		status = nfsd_setattr(rqstp, resfhp, attrs, NULL);
>  	else
>  		status = nfserrno(commit_metadata(resfhp));
>  
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 702fbc4483bf..7d77303ef5f7 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -69,7 +69,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 nfsd_attrs *, int, time64_t);
> +			     struct nfsd_attrs *, const struct timespec64 *);
>  int nfsd_mountpoint(struct dentry *, struct svc_export *);
>  #ifdef CONFIG_NFSD_V4
>  __be32		nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *,
> diff --git a/fs/nfsd/xdr3.h b/fs/nfsd/xdr3.h
> index 03fe4e21306c..522067b7fd75 100644
> --- a/fs/nfsd/xdr3.h
> +++ b/fs/nfsd/xdr3.h
> @@ -14,7 +14,7 @@ struct nfsd3_sattrargs {
>  	struct svc_fh		fh;
>  	struct iattr		attrs;
>  	int			check_guard;
> -	time64_t		guardtime;
> +	struct timespec64	guardtime;
>  };
>  
>  struct nfsd3_diropargs {
> -- 
> 2.43.1
> 
> 
> 

Nice cleanup was well as the bug fix - thanks,

Reviewed-by: NeilBrown <neilb@suse.de>

NeilBrown

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Chuck Lever]

On Thu, Feb 15, 2024 at 08:24:50PM -0500, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes behaviour
> when doing a SETATTR rpc call by stripping out the calls to
> fh_fill_pre_attrs() and fh_fill_post_attrs().

Can you give more detail about what broke?


> Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of fh_(un)lock for file operations")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfsd/nfs4proc.c | 4 ++++
>  fs/nfsd/vfs.c      | 9 +++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 14712fa08f76..e6d8624efc83 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	};
>  	struct inode *inode;
>  	__be32 status = nfs_ok;
> +	bool save_no_wcc;
>  	int err;
>  
>  	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> @@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	if (status)
>  		goto out;
> +	save_no_wcc = cstate->current_fh.fh_no_wcc;
> +	cstate->current_fh.fh_no_wcc = true;
>  	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
>  				0, (time64_t)0);
> +	cstate->current_fh.fh_no_wcc = save_no_wcc;
>  	if (!status)
>  		status = nfserrno(attrs.na_labelerr);
>  	if (!status)
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 6e7e37192461..58fab461bc00 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -497,7 +497,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	int		accmode = NFSD_MAY_SATTR;
>  	umode_t		ftype = 0;
>  	__be32		err;
> -	int		host_err;
> +	int		host_err = 0;
>  	bool		get_write_count;
>  	bool		size_change = (iap->ia_valid & ATTR_SIZE);
>  	int		retries;
> @@ -555,6 +555,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	}
>  
>  	inode_lock(inode);
> +	err = fh_fill_pre_attrs(fhp);
> +	if (err)
> +		goto out_unlock;
>  	for (retries = 1;;) {
>  		struct iattr attrs;
>  
> @@ -582,13 +585,15 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
>  						dentry, ACL_TYPE_DEFAULT,
>  						attr->na_dpacl);
> +	fh_fill_post_attrs(fhp);
> +out_unlock:
>  	inode_unlock(inode);
>  	if (size_change)
>  		put_write_access(inode);
>  out:
>  	if (!host_err)
>  		host_err = commit_metadata(fhp);
> -	return nfserrno(host_err);
> +	return err != 0 ? err : nfserrno(host_err);
>  }
>  
>  #if defined(CONFIG_NFSD_V4)
> -- 
> 2.43.1
> 
> 

-- 
Chuck Lever

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Trond Myklebust]

On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
> On Thu, Feb 15, 2024 at 08:24:50PM -0500, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes
> > behaviour
> > when doing a SETATTR rpc call by stripping out the calls to
> > fh_fill_pre_attrs() and fh_fill_post_attrs().
> 
> Can you give more detail about what broke?

Without the calls to fh_fill_pre_attrs() and fh_fill_post_attrs(), we
don't store any pre/post op attributes and we can't return any such
attributes to the NFSv3 client.

> 
> 
> > Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of
> > fh_(un)lock for file operations")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 4 ++++
> >  fs/nfsd/vfs.c      | 9 +++++++--
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 14712fa08f76..e6d8624efc83 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >  	};
> >  	struct inode *inode;
> >  	__be32 status = nfs_ok;
> > +	bool save_no_wcc;
> >  	int err;
> >  
> >  	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> > @@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >  
> >  	if (status)
> >  		goto out;
> > +	save_no_wcc = cstate->current_fh.fh_no_wcc;
> > +	cstate->current_fh.fh_no_wcc = true;
> >  	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> >  				0, (time64_t)0);
> > +	cstate->current_fh.fh_no_wcc = save_no_wcc;
> >  	if (!status)
> >  		status = nfserrno(attrs.na_labelerr);
> >  	if (!status)
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 6e7e37192461..58fab461bc00 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -497,7 +497,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
> > svc_fh *fhp,
> >  	int		accmode = NFSD_MAY_SATTR;
> >  	umode_t		ftype = 0;
> >  	__be32		err;
> > -	int		host_err;
> > +	int		host_err = 0;
> >  	bool		get_write_count;
> >  	bool		size_change = (iap->ia_valid & ATTR_SIZE);
> >  	int		retries;
> > @@ -555,6 +555,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
> > svc_fh *fhp,
> >  	}
> >  
> >  	inode_lock(inode);
> > +	err = fh_fill_pre_attrs(fhp);
> > +	if (err)
> > +		goto out_unlock;
> >  	for (retries = 1;;) {
> >  		struct iattr attrs;
> >  
> > @@ -582,13 +585,15 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
> > svc_fh *fhp,
> >  		attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
> >  						dentry,
> > ACL_TYPE_DEFAULT,
> >  						attr->na_dpacl);
> > +	fh_fill_post_attrs(fhp);
> > +out_unlock:
> >  	inode_unlock(inode);
> >  	if (size_change)
> >  		put_write_access(inode);
> >  out:
> >  	if (!host_err)
> >  		host_err = commit_metadata(fhp);
> > -	return nfserrno(host_err);
> > +	return err != 0 ? err : nfserrno(host_err);
> >  }
> >  
> >  #if defined(CONFIG_NFSD_V4)
> > -- 
> > 2.43.1
> > 
> > 
> 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Chuck Lever III]

> On Feb 16, 2024, at 1:18 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
>> On Thu, Feb 15, 2024 at 08:24:50PM -0500, trondmy@kernel.org wrote:
>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> 
>>> Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes
>>> behaviour
>>> when doing a SETATTR rpc call by stripping out the calls to
>>> fh_fill_pre_attrs() and fh_fill_post_attrs().
>> 
>> Can you give more detail about what broke?
> 
> Without the calls to fh_fill_pre_attrs() and fh_fill_post_attrs(), we
> don't store any pre/post op attributes and we can't return any such
> attributes to the NFSv3 client.

I get that. Why does that matter?


>>> Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of
>>> fh_(un)lock for file operations")
>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> ---
>>>  fs/nfsd/nfs4proc.c | 4 ++++
>>>  fs/nfsd/vfs.c      | 9 +++++++--
>>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index 14712fa08f76..e6d8624efc83 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
>>> nfsd4_compound_state *cstate,
>>>   };
>>>   struct inode *inode;
>>>   __be32 status = nfs_ok;
>>> + bool save_no_wcc;
>>>   int err;
>>>  
>>>   if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
>>> @@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
>>> nfsd4_compound_state *cstate,
>>>  
>>>   if (status)
>>>   goto out;
>>> + save_no_wcc = cstate->current_fh.fh_no_wcc;
>>> + cstate->current_fh.fh_no_wcc = true;
>>>   status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
>>>   0, (time64_t)0);
>>> + cstate->current_fh.fh_no_wcc = save_no_wcc;
>>>   if (!status)
>>>   status = nfserrno(attrs.na_labelerr);
>>>   if (!status)
>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index 6e7e37192461..58fab461bc00 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -497,7 +497,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
>>> svc_fh *fhp,
>>>   int accmode = NFSD_MAY_SATTR;
>>>   umode_t ftype = 0;
>>>   __be32 err;
>>> - int host_err;
>>> + int host_err = 0;
>>>   bool get_write_count;
>>>   bool size_change = (iap->ia_valid & ATTR_SIZE);
>>>   int retries;
>>> @@ -555,6 +555,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
>>> svc_fh *fhp,
>>>   }
>>>  
>>>   inode_lock(inode);
>>> + err = fh_fill_pre_attrs(fhp);
>>> + if (err)
>>> + goto out_unlock;
>>>   for (retries = 1;;) {
>>>   struct iattr attrs;
>>>  
>>> @@ -582,13 +585,15 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
>>> svc_fh *fhp,
>>>   attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
>>>   dentry,
>>> ACL_TYPE_DEFAULT,
>>>   attr->na_dpacl);
>>> + fh_fill_post_attrs(fhp);
>>> +out_unlock:
>>>   inode_unlock(inode);
>>>   if (size_change)
>>>   put_write_access(inode);
>>>  out:
>>>   if (!host_err)
>>>   host_err = commit_metadata(fhp);
>>> - return nfserrno(host_err);
>>> + return err != 0 ? err : nfserrno(host_err);
>>>  }
>>>  
>>>  #if defined(CONFIG_NFSD_V4)
>>> -- 
>>> 2.43.1
>>> 
>>> 
>> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com


--
Chuck Lever

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Chuck Lever III]

> On Feb 16, 2024, at 1:19 PM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
> 
> 
>> On Feb 16, 2024, at 1:18 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
>> 
>> On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
>>> On Thu, Feb 15, 2024 at 08:24:50PM -0500, trondmy@kernel.org wrote:
>>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>>> 
>>>> Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes
>>>> behaviour
>>>> when doing a SETATTR rpc call by stripping out the calls to
>>>> fh_fill_pre_attrs() and fh_fill_post_attrs().
>>> 
>>> Can you give more detail about what broke?
>> 
>> Without the calls to fh_fill_pre_attrs() and fh_fill_post_attrs(), we
>> don't store any pre/post op attributes and we can't return any such
>> attributes to the NFSv3 client.
> 
> I get that. Why does that matter?

Or, to be a little less terse... clients rely on the pre/post
op attributes around a SETATTR, I guess, but I don't see why.
I'm missing some context.


>>>> Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of
>>>> fh_(un)lock for file operations")
>>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>>> ---
>>>> fs/nfsd/nfs4proc.c | 4 ++++
>>>> fs/nfsd/vfs.c      | 9 +++++++--
>>>> 2 files changed, 11 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>> index 14712fa08f76..e6d8624efc83 100644
>>>> --- a/fs/nfsd/nfs4proc.c
>>>> +++ b/fs/nfsd/nfs4proc.c
>>>> @@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
>>>> nfsd4_compound_state *cstate,
>>>>  };
>>>>  struct inode *inode;
>>>>  __be32 status = nfs_ok;
>>>> + bool save_no_wcc;
>>>>  int err;
>>>> 
>>>>  if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
>>>> @@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
>>>> nfsd4_compound_state *cstate,
>>>> 
>>>>  if (status)
>>>>  goto out;
>>>> + save_no_wcc = cstate->current_fh.fh_no_wcc;
>>>> + cstate->current_fh.fh_no_wcc = true;
>>>>  status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
>>>>  0, (time64_t)0);
>>>> + cstate->current_fh.fh_no_wcc = save_no_wcc;
>>>>  if (!status)
>>>>  status = nfserrno(attrs.na_labelerr);
>>>>  if (!status)
>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>> index 6e7e37192461..58fab461bc00 100644
>>>> --- a/fs/nfsd/vfs.c
>>>> +++ b/fs/nfsd/vfs.c
>>>> @@ -497,7 +497,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
>>>> svc_fh *fhp,
>>>>  int accmode = NFSD_MAY_SATTR;
>>>>  umode_t ftype = 0;
>>>>  __be32 err;
>>>> - int host_err;
>>>> + int host_err = 0;
>>>>  bool get_write_count;
>>>>  bool size_change = (iap->ia_valid & ATTR_SIZE);
>>>>  int retries;
>>>> @@ -555,6 +555,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
>>>> svc_fh *fhp,
>>>>  }
>>>> 
>>>>  inode_lock(inode);
>>>> + err = fh_fill_pre_attrs(fhp);
>>>> + if (err)
>>>> + goto out_unlock;
>>>>  for (retries = 1;;) {
>>>>  struct iattr attrs;
>>>> 
>>>> @@ -582,13 +585,15 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
>>>> svc_fh *fhp,
>>>>  attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
>>>>  dentry,
>>>> ACL_TYPE_DEFAULT,
>>>>  attr->na_dpacl);
>>>> + fh_fill_post_attrs(fhp);
>>>> +out_unlock:
>>>>  inode_unlock(inode);
>>>>  if (size_change)
>>>>  put_write_access(inode);
>>>> out:
>>>>  if (!host_err)
>>>>  host_err = commit_metadata(fhp);
>>>> - return nfserrno(host_err);
>>>> + return err != 0 ? err : nfserrno(host_err);
>>>> }
>>>> 
>>>> #if defined(CONFIG_NFSD_V4)
>>>> -- 
>>>> 2.43.1
>>>> 
>>>> 
>>> 
>> 
>> -- 
>> Trond Myklebust
>> Linux NFS client maintainer, Hammerspace
>> trond.myklebust@hammerspace.com
> 
> 
> --
> Chuck Lever


--
Chuck Lever

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Trond Myklebust]

On Fri, 2024-02-16 at 18:25 +0000, Chuck Lever III wrote:
> 
> 
> > On Feb 16, 2024, at 1:19 PM, Chuck Lever III
> > <chuck.lever@oracle.com> wrote:
> > 
> > 
> > 
> > > On Feb 16, 2024, at 1:18 PM, Trond Myklebust
> > > <trondmy@hammerspace.com> wrote:
> > > 
> > > On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
> > > > On Thu, Feb 15, 2024 at 08:24:50PM -0500,
> > > > trondmy@kernel.org wrote:
> > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > > 
> > > > > Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes
> > > > > behaviour
> > > > > when doing a SETATTR rpc call by stripping out the calls to
> > > > > fh_fill_pre_attrs() and fh_fill_post_attrs().
> > > > 
> > > > Can you give more detail about what broke?
> > > 
> > > Without the calls to fh_fill_pre_attrs() and
> > > fh_fill_post_attrs(), we
> > > don't store any pre/post op attributes and we can't return any
> > > such
> > > attributes to the NFSv3 client.
> > 
> > I get that. Why does that matter?
> 
> Or, to be a little less terse... clients rely on the pre/post
> op attributes around a SETATTR, I guess, but I don't see why.
> I'm missing some context.

   1. SETATTR is not atomic, and is not implemented as being atomic in
      Linux. It is perfectly possible for, say, the file to get
      truncated, but for the other attribute changes to get dropped on
      the floor. NFSv4 communicates that information via the bitmap.
      NFSv3 does it using the pre/post attributes.
   2. When doing a guarded SETATTR, if the server returns
      NFS3ERR_NOT_SYNC, the client may want to update its cached ctime
      and resend.


> 
> 
> > > > > Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of
> > > > > fh_(un)lock for file operations")
> > > > > Signed-off-by: Trond Myklebust
> > > > > <trond.myklebust@hammerspace.com>
> > > > > ---
> > > > > fs/nfsd/nfs4proc.c | 4 ++++
> > > > > fs/nfsd/vfs.c      | 9 +++++++--
> > > > > 2 files changed, 11 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > index 14712fa08f76..e6d8624efc83 100644
> > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > @@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp,
> > > > > struct
> > > > > nfsd4_compound_state *cstate,
> > > > >  };
> > > > >  struct inode *inode;
> > > > >  __be32 status = nfs_ok;
> > > > > + bool save_no_wcc;
> > > > >  int err;
> > > > > 
> > > > >  if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> > > > > @@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp,
> > > > > struct
> > > > > nfsd4_compound_state *cstate,
> > > > > 
> > > > >  if (status)
> > > > >  goto out;
> > > > > + save_no_wcc = cstate->current_fh.fh_no_wcc;
> > > > > + cstate->current_fh.fh_no_wcc = true;
> > > > >  status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> > > > >  0, (time64_t)0);
> > > > > + cstate->current_fh.fh_no_wcc = save_no_wcc;
> > > > >  if (!status)
> > > > >  status = nfserrno(attrs.na_labelerr);
> > > > >  if (!status)
> > > > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > > > index 6e7e37192461..58fab461bc00 100644
> > > > > --- a/fs/nfsd/vfs.c
> > > > > +++ b/fs/nfsd/vfs.c
> > > > > @@ -497,7 +497,7 @@ nfsd_setattr(struct svc_rqst *rqstp,
> > > > > struct
> > > > > svc_fh *fhp,
> > > > >  int accmode = NFSD_MAY_SATTR;
> > > > >  umode_t ftype = 0;
> > > > >  __be32 err;
> > > > > - int host_err;
> > > > > + int host_err = 0;
> > > > >  bool get_write_count;
> > > > >  bool size_change = (iap->ia_valid & ATTR_SIZE);
> > > > >  int retries;
> > > > > @@ -555,6 +555,9 @@ nfsd_setattr(struct svc_rqst *rqstp,
> > > > > struct
> > > > > svc_fh *fhp,
> > > > >  }
> > > > > 
> > > > >  inode_lock(inode);
> > > > > + err = fh_fill_pre_attrs(fhp);
> > > > > + if (err)
> > > > > + goto out_unlock;
> > > > >  for (retries = 1;;) {
> > > > >  struct iattr attrs;
> > > > > 
> > > > > @@ -582,13 +585,15 @@ nfsd_setattr(struct svc_rqst *rqstp,
> > > > > struct
> > > > > svc_fh *fhp,
> > > > >  attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
> > > > >  dentry,
> > > > > ACL_TYPE_DEFAULT,
> > > > >  attr->na_dpacl);
> > > > > + fh_fill_post_attrs(fhp);
> > > > > +out_unlock:
> > > > >  inode_unlock(inode);
> > > > >  if (size_change)
> > > > >  put_write_access(inode);
> > > > > out:
> > > > >  if (!host_err)
> > > > >  host_err = commit_metadata(fhp);
> > > > > - return nfserrno(host_err);
> > > > > + return err != 0 ? err : nfserrno(host_err);
> > > > > }
> > > > > 
> > > > > #if defined(CONFIG_NFSD_V4)
> > > > > -- 
> > > > > 2.43.1
> > > > > 
> > > > > 
> > > > 

-- 
Trond Myklebust Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Chuck Lever]

On Fri, Feb 16, 2024 at 06:57:16PM +0000, Trond Myklebust wrote:
> On Fri, 2024-02-16 at 18:25 +0000, Chuck Lever III wrote:
> > 
> > 
> > > On Feb 16, 2024, at 1:19 PM, Chuck Lever III
> > > <chuck.lever@oracle.com> wrote:
> > > 
> > > 
> > > 
> > > > On Feb 16, 2024, at 1:18 PM, Trond Myklebust
> > > > <trondmy@hammerspace.com> wrote:
> > > > 
> > > > On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
> > > > > On Thu, Feb 15, 2024 at 08:24:50PM -0500,
> > > > > trondmy@kernel.org wrote:
> > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > > > 
> > > > > > Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes
> > > > > > behaviour
> > > > > > when doing a SETATTR rpc call by stripping out the calls to
> > > > > > fh_fill_pre_attrs() and fh_fill_post_attrs().
> > > > > 
> > > > > Can you give more detail about what broke?
> > > > 
> > > > Without the calls to fh_fill_pre_attrs() and
> > > > fh_fill_post_attrs(), we
> > > > don't store any pre/post op attributes and we can't return any
> > > > such
> > > > attributes to the NFSv3 client.
> > > 
> > > I get that. Why does that matter?
> > 
> > Or, to be a little less terse... clients rely on the pre/post
> > op attributes around a SETATTR, I guess, but I don't see why.
> > I'm missing some context.
> 
>    1. SETATTR is not atomic, and is not implemented as being atomic in
>       Linux. It is perfectly possible for, say, the file to get
>       truncated, but for the other attribute changes to get dropped on
>       the floor. NFSv4 communicates that information via the bitmap.
>       NFSv3 does it using the pre/post attributes.
>    2. When doing a guarded SETATTR, if the server returns
>       NFS3ERR_NOT_SYNC, the client may want to update its cached ctime
>       and resend.

All granted, but I'm still not clear. Let me ask this a different
way.

As far as I can tell, it's always been optional for an NFSv3 server
to include pre- and post-op attributes in wcc_data. Both the
pre_op_attr and post_op_attr XDR types start with an
"attribute_follows" discriminator. Therefore clients cannot rely on
receiving those attributes.

The patch description says that "Commit bb4d53d66e4b broke the NFSv3
                                                     ^^^^^
pre/post op attributes ...". And I think you also used the word
"nasty" in an earlier email. So what is broken if the server /never/
returns those attributes? What are the application-visible effects
of the server behavior change in bb4d53d66e4b ?

I don't have a problem reverting that part of bb4d53d66e4b, but
"broke" is doing some heavy lifting here. I want to understand why
we need to revert. Because it seems to me the server's current NFSv3
SETATTR implementation is spec-compliant. As far as I can tell,
bb4d53d66e4b might result in a little more network traffic in some
cases, but it won't impact interoperability or outcome.

Do you mean that you want to restore the previous, more optimized,
server behavior to return pre- and post-op attributes when they are
available? And if so, what is the application-visible benefit?


-- 
Chuck Lever

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Trond Myklebust]

On Sat, 2024-02-17 at 12:05 -0500, Chuck Lever wrote:
> On Fri, Feb 16, 2024 at 06:57:16PM +0000, Trond Myklebust wrote:
> > On Fri, 2024-02-16 at 18:25 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Feb 16, 2024, at 1:19 PM, Chuck Lever III
> > > > <chuck.lever@oracle.com> wrote:
> > > > 
> > > > 
> > > > 
> > > > > On Feb 16, 2024, at 1:18 PM, Trond Myklebust
> > > > > <trondmy@hammerspace.com> wrote:
> > > > > 
> > > > > On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
> > > > > > On Thu, Feb 15, 2024 at 08:24:50PM -0500,
> > > > > > trondmy@kernel.org wrote:
> > > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > > > > 
> > > > > > > Commit bb4d53d66e4b broke the NFSv3 pre/post op
> > > > > > > attributes
> > > > > > > behaviour
> > > > > > > when doing a SETATTR rpc call by stripping out the calls
> > > > > > > to
> > > > > > > fh_fill_pre_attrs() and fh_fill_post_attrs().
> > > > > > 
> > > > > > Can you give more detail about what broke?
> > > > > 
> > > > > Without the calls to fh_fill_pre_attrs() and
> > > > > fh_fill_post_attrs(), we
> > > > > don't store any pre/post op attributes and we can't return
> > > > > any
> > > > > such
> > > > > attributes to the NFSv3 client.
> > > > 
> > > > I get that. Why does that matter?
> > > 
> > > Or, to be a little less terse... clients rely on the pre/post
> > > op attributes around a SETATTR, I guess, but I don't see why.
> > > I'm missing some context.
> > 
> >    1. SETATTR is not atomic, and is not implemented as being atomic
> > in
> >       Linux. It is perfectly possible for, say, the file to get
> >       truncated, but for the other attribute changes to get dropped
> > on
> >       the floor. NFSv4 communicates that information via the
> > bitmap.
> >       NFSv3 does it using the pre/post attributes.
> >    2. When doing a guarded SETATTR, if the server returns
> >       NFS3ERR_NOT_SYNC, the client may want to update its cached
> > ctime
> >       and resend.
> 
> All granted, but I'm still not clear. Let me ask this a different
> way.
> 
> As far as I can tell, it's always been optional for an NFSv3 server
> to include pre- and post-op attributes in wcc_data. Both the
> pre_op_attr and post_op_attr XDR types start with an
> "attribute_follows" discriminator. Therefore clients cannot rely on
> receiving those attributes.
> 
> The patch description says that "Commit bb4d53d66e4b broke the NFSv3
>                                                      ^^^^^
> pre/post op attributes ...". And I think you also used the word
> "nasty" in an earlier email. So what is broken if the server /never/
> returns those attributes? What are the application-visible effects
> of the server behavior change in bb4d53d66e4b ?
> 
> I don't have a problem reverting that part of bb4d53d66e4b, but
> "broke" is doing some heavy lifting here. I want to understand why
> we need to revert. Because it seems to me the server's current NFSv3
> SETATTR implementation is spec-compliant. As far as I can tell,
> bb4d53d66e4b might result in a little more network traffic in some
> cases, but it won't impact interoperability or outcome.
> 

As I said above, you broke the NFSv3 client's ability to determine
whether or not the SETATTR was a failure, success or a partial success.
That's not heavy lifting, it is a fact.

The function nfsd_setattr() uses two different calls to notify_change()
in order to perform its function. Either one of them can return an
error. Either one of them can fail, and the way that the client finds
out whether or not the operation was a partial success is by examining
the pre/post op attributes (NFSv3) or the returned bitmap (NFSv4).

The patch does not try to fix NFSv4, since AFAICS, that code has always
been broken.

However, the NFSv3 code was not broken prior to bb4d53d66e4b. It was
correctly returning pre/post op attributes that reflected the
success/failure of the setattr operation. That is therefore a
regression.
Furthermore, it is a totally unnecessary regression. The whole point of
SETATTR is to change the value of the attributes of the exact same file
for which the pre/post op attributes are being retrieved. There is no
extra disk access required to retrieve those attributes, nor should
there be any other overhead other than copying them into the buffer.

> Do you mean that you want to restore the previous, more optimized,
> server behavior to return pre- and post-op attributes when they are
> available? And if so, what is the application-visible benefit?

Application correctness: the ability to see that your file got
truncated despite the RPC call returning an error.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Jeff Layton]

On Sat, 2024-02-17 at 18:16 +0000, Trond Myklebust wrote:
> On Sat, 2024-02-17 at 12:05 -0500, Chuck Lever wrote:
> > On Fri, Feb 16, 2024 at 06:57:16PM +0000, Trond Myklebust wrote:
> > > On Fri, 2024-02-16 at 18:25 +0000, Chuck Lever III wrote:
> > > > 
> > > > 
> > > > > On Feb 16, 2024, at 1:19 PM, Chuck Lever III
> > > > > <chuck.lever@oracle.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > > On Feb 16, 2024, at 1:18 PM, Trond Myklebust
> > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > 
> > > > > > On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
> > > > > > > On Thu, Feb 15, 2024 at 08:24:50PM -0500,
> > > > > > > trondmy@kernel.org wrote:
> > > > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > > > > > 
> > > > > > > > Commit bb4d53d66e4b broke the NFSv3 pre/post op
> > > > > > > > attributes
> > > > > > > > behaviour
> > > > > > > > when doing a SETATTR rpc call by stripping out the calls
> > > > > > > > to
> > > > > > > > fh_fill_pre_attrs() and fh_fill_post_attrs().
> > > > > > > 
> > > > > > > Can you give more detail about what broke?
> > > > > > 
> > > > > > Without the calls to fh_fill_pre_attrs() and
> > > > > > fh_fill_post_attrs(), we
> > > > > > don't store any pre/post op attributes and we can't return
> > > > > > any
> > > > > > such
> > > > > > attributes to the NFSv3 client.
> > > > > 
> > > > > I get that. Why does that matter?
> > > > 
> > > > Or, to be a little less terse... clients rely on the pre/post
> > > > op attributes around a SETATTR, I guess, but I don't see why.
> > > > I'm missing some context.
> > > 
> > >    1. SETATTR is not atomic, and is not implemented as being atomic
> > > in
> > >       Linux. It is perfectly possible for, say, the file to get
> > >       truncated, but for the other attribute changes to get dropped
> > > on
> > >       the floor. NFSv4 communicates that information via the
> > > bitmap.
> > >       NFSv3 does it using the pre/post attributes.
> > >    2. When doing a guarded SETATTR, if the server returns
> > >       NFS3ERR_NOT_SYNC, the client may want to update its cached
> > > ctime
> > >       and resend.
> > 
> > All granted, but I'm still not clear. Let me ask this a different
> > way.
> > 
> > As far as I can tell, it's always been optional for an NFSv3 server
> > to include pre- and post-op attributes in wcc_data. Both the
> > pre_op_attr and post_op_attr XDR types start with an
> > "attribute_follows" discriminator. Therefore clients cannot rely on
> > receiving those attributes.
> > 
> > The patch description says that "Commit bb4d53d66e4b broke the NFSv3
> >                                                      ^^^^^
> > pre/post op attributes ...". And I think you also used the word
> > "nasty" in an earlier email. So what is broken if the server /never/
> > returns those attributes? What are the application-visible effects
> > of the server behavior change in bb4d53d66e4b ?
> > 
> > I don't have a problem reverting that part of bb4d53d66e4b, but
> > "broke" is doing some heavy lifting here. I want to understand why
> > we need to revert. Because it seems to me the server's current NFSv3
> > SETATTR implementation is spec-compliant. As far as I can tell,
> > bb4d53d66e4b might result in a little more network traffic in some
> > cases, but it won't impact interoperability or outcome.
> > 
> 
> As I said above, you broke the NFSv3 client's ability to determine
> whether or not the SETATTR was a failure, success or a partial success.
> That's not heavy lifting, it is a fact.
> 
> The function nfsd_setattr() uses two different calls to notify_change()
> in order to perform its function. Either one of them can return an
> error. Either one of them can fail, and the way that the client finds
> out whether or not the operation was a partial success is by examining
> the pre/post op attributes (NFSv3) or the returned bitmap (NFSv4).
> 
> The patch does not try to fix NFSv4, since AFAICS, that code has always
> been broken.
> 

Ugh, yeah we can definitely do a better job there.

> However, the NFSv3 code was not broken prior to bb4d53d66e4b. It was
> correctly returning pre/post op attributes that reflected the
> success/failure of the setattr operation. That is therefore a
> regression.
> Furthermore, it is a totally unnecessary regression. The whole point of
> SETATTR is to change the value of the attributes of the exact same file
> for which the pre/post op attributes are being retrieved. There is no
> extra disk access required to retrieve those attributes, nor should
> there be any other overhead other than copying them into the buffer.
> 
> > Do you mean that you want to restore the previous, more optimized,
> > server behavior to return pre- and post-op attributes when they are
> > available? And if so, what is the application-visible benefit?
> 
> Application correctness: the ability to see that your file got
> truncated despite the RPC call returning an error.
> 

So for instance: a v3 client sends a SETATTR with a size and mode change
in the same op. The size change works, but the mode change doesn't for
some reason.

The server returns an error, as expected, but without the pre/post op
attrs, the v3 client could just assume that _nothing_ worked and not
notice the size change. Alternately, I guess the client could invalidate
the attrcache after any SETATTR error, but that's sub-optimal.

The fix looks reasonable to me:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[NeilBrown]

On Fri, 16 Feb 2024, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes behaviour
> when doing a SETATTR rpc call by stripping out the calls to
> fh_fill_pre_attrs() and fh_fill_post_attrs().
> 
> Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of fh_(un)lock for file operations")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfsd/nfs4proc.c | 4 ++++
>  fs/nfsd/vfs.c      | 9 +++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 14712fa08f76..e6d8624efc83 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	};
>  	struct inode *inode;
>  	__be32 status = nfs_ok;
> +	bool save_no_wcc;
>  	int err;
>  
>  	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> @@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	if (status)
>  		goto out;
> +	save_no_wcc = cstate->current_fh.fh_no_wcc;
> +	cstate->current_fh.fh_no_wcc = true;
>  	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
>  				0, (time64_t)0);
> +	cstate->current_fh.fh_no_wcc = save_no_wcc;

This looks clumsy.
I think the background is that NFSv3 needs atomic wcc attributes for
file operations, but NFSv4 doesn't - it only has them for directory ops.
So NFSv4, like NFSv2, doesn't want fh_fill_pre_attrs() to be called by
nfsd_setattr().

NFSv2 avoids it by always setting ->fh_no_wcc.  Here you temporarily set
fh_no_wcc to true for the same effect.  So the code is correct.
But it is not obvious to the casual reader why this is happening.

I would rather a "wcc_wanted" flag or similar, but that can be done in a
separate clean-up patch later.

Thanks for finding and fixing this regression of mine.

Reviewed-by: NeilBrown <neilb@suse.de>

NeilBrown



>  	if (!status)
>  		status = nfserrno(attrs.na_labelerr);
>  	if (!status)
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 6e7e37192461..58fab461bc00 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -497,7 +497,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	int		accmode = NFSD_MAY_SATTR;
>  	umode_t		ftype = 0;
>  	__be32		err;
> -	int		host_err;
> +	int		host_err = 0;
>  	bool		get_write_count;
>  	bool		size_change = (iap->ia_valid & ATTR_SIZE);
>  	int		retries;
> @@ -555,6 +555,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	}
>  
>  	inode_lock(inode);
> +	err = fh_fill_pre_attrs(fhp);
> +	if (err)
> +		goto out_unlock;
>  	for (retries = 1;;) {
>  		struct iattr attrs;
>  
> @@ -582,13 +585,15 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
>  						dentry, ACL_TYPE_DEFAULT,
>  						attr->na_dpacl);
> +	fh_fill_post_attrs(fhp);
> +out_unlock:
>  	inode_unlock(inode);
>  	if (size_change)
>  		put_write_access(inode);
>  out:
>  	if (!host_err)
>  		host_err = commit_metadata(fhp);
> -	return nfserrno(host_err);
> +	return err != 0 ? err : nfserrno(host_err);
>  }
>  
>  #if defined(CONFIG_NFSD_V4)
> -- 
> 2.43.1
> 
> 
> 

Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Trond Myklebust]

On Mon, 2024-02-19 at 08:57 +1100, NeilBrown wrote:
> On Fri, 16 Feb 2024, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes
> > behaviour
> > when doing a SETATTR rpc call by stripping out the calls to
> > fh_fill_pre_attrs() and fh_fill_post_attrs().
> > 
> > Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of
> > fh_(un)lock for file operations")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 4 ++++
> >  fs/nfsd/vfs.c      | 9 +++++++--
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 14712fa08f76..e6d8624efc83 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >  	};
> >  	struct inode *inode;
> >  	__be32 status = nfs_ok;
> > +	bool save_no_wcc;
> >  	int err;
> >  
> >  	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> > @@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >  
> >  	if (status)
> >  		goto out;
> > +	save_no_wcc = cstate->current_fh.fh_no_wcc;
> > +	cstate->current_fh.fh_no_wcc = true;
> >  	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> >  				0, (time64_t)0);
> > +	cstate->current_fh.fh_no_wcc = save_no_wcc;
> 
> This looks clumsy.
> I think the background is that NFSv3 needs atomic wcc attributes for
> file operations, but NFSv4 doesn't - it only has them for directory
> ops.
> So NFSv4, like NFSv2, doesn't want fh_fill_pre_attrs() to be called
> by
> nfsd_setattr().
> 
> NFSv2 avoids it by always setting ->fh_no_wcc.  Here you temporarily
> set
> fh_no_wcc to true for the same effect.  So the code is correct.
> But it is not obvious to the casual reader why this is happening.
> 
> I would rather a "wcc_wanted" flag or similar, but that can be done
> in a
> separate clean-up patch later.

That is in theory what the fh_no_wcc flag is for, however the issue is
that it got overloaded to also mean 'change_info4 wanted' when we added
support for NFSv4 to knfsd.
NFSv4 does not have a concept of weak cache consistency, but it does
try to track updates to the change attribute atomically (ideally) for
most operations that change the directory contents.

IOW: I think a better clean up would be to separate out 'wcc' and
'change_info4' as representing different functionality.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v2 0/2] Fix NFSv3 SETATTR behaviours

[Chuck Lever]

On Thu, Feb 15, 2024 at 08:24:49PM -0500, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> Fix recent regressions in nfsd_setattr(), and fix the "guarded SETATTR"
> behaviour for NFSv3.
> 
> Trond Myklebust (2):
>   nfsd: Fix a regression in nfsd_setattr()
>   nfsd: Fix NFSv3 atomicity bugs in nfsd_setattr()
> 
>  fs/nfsd/nfs3proc.c  |  6 ++++--
>  fs/nfsd/nfs3xdr.c   |  5 +----
>  fs/nfsd/nfs4proc.c  |  7 +++++--
>  fs/nfsd/nfs4state.c |  2 +-
>  fs/nfsd/nfsproc.c   |  6 +++---
>  fs/nfsd/vfs.c       | 29 ++++++++++++++++++++---------
>  fs/nfsd/vfs.h       |  2 +-
>  fs/nfsd/xdr3.h      |  2 +-
>  8 files changed, 36 insertions(+), 23 deletions(-)
> 
> -- 
> 2.43.1
> 

Both applied to nfsd-next. Thanks gents!

-- 
Chuck Lever