[PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post attributes

[PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post attributes

[Jeff Layton]

Boyang reported tripping the BUG_ON in set_change_info. While we
couldn't confirm it, one way this could happen would be for nfsd_lookup
to succeed and then for fh_fill_both_attrs to fail.

This patchset attempts to (sanely) fix this, usually by aborting the
operation if fetching the pre attributes fails. Post-op attribute fetch
handling is more difficult to deal with however since we've already done
the operation, so this has it just fudge the change_info4 if that
occurs.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- make fh_fill_*_attrs return an error and have the callers handle it
- rework of set_change_info, to better handle missing pre/post attrs

---
Jeff Layton (2):
      nfsd: handle failure to collect pre/post-op attrs more sanely
      nfsd: remove unsafe BUG_ON from set_change_info

 fs/nfsd/nfs3proc.c |  4 +++-
 fs/nfsd/nfs4proc.c | 45 +++++++++++++++++++++++++++++++++------
 fs/nfsd/nfsfh.c    | 26 ++++++++++++++---------
 fs/nfsd/nfsfh.h    |  6 +++---
 fs/nfsd/vfs.c      | 62 ++++++++++++++++++++++++++++++++++--------------------
 fs/nfsd/xdr4.h     | 11 ----------
 6 files changed, 100 insertions(+), 54 deletions(-)
---
base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e
change-id: 20230720-bz2223560-9c4690a8217b

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v2 1/2] nfsd: handle failure to collect pre/post-op attrs more sanely

[Jeff Layton]

Collecting pre_op_attrs can fail, in which case it's probably best to
fail the whole operation.

Change fh_fill_{pre,post,both}_attrs to return __be32. For the pre and
both functions, have the callers check the return code and abort the
operation if it failed.

If fh_fill_post_attrs fails, then it's too late to do anything about it,
so most of those callers ignore the return value. If this happens, then
fh_post_saved will be false, which should cue the later stages to deal
with it.

Suggested-by: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs3proc.c |  4 +++-
 fs/nfsd/nfs4proc.c | 14 ++++++------
 fs/nfsd/nfsfh.c    | 26 ++++++++++++++---------
 fs/nfsd/nfsfh.h    |  6 +++---
 fs/nfsd/vfs.c      | 62 ++++++++++++++++++++++++++++++++++--------------------
 5 files changed, 69 insertions(+), 43 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index fc8d5b7db9f8..268ef57751c4 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -307,7 +307,9 @@ 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);
+	status = fh_fill_pre_attrs(fhp);
+	if (status != nfs_ok)
+		goto out;
 	host_err = vfs_create(&nop_mnt_idmap, inode, child, iap->ia_mode, true);
 	if (host_err < 0) {
 		status = nfserrno(host_err);
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index d8e7a533f9d2..9285e1eab4d5 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -297,12 +297,12 @@ 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);
+		status = fh_fill_both_attrs(fhp);
+		if (status != nfs_ok)
+			goto out;
 
 		switch (open->op_createmode) {
 		case NFS4_CREATE_UNCHECKED:
@@ -345,7 +345,9 @@ 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 = fh_fill_pre_attrs(fhp);
+	if (status != nfs_ok)
+		goto out;
 	status = nfsd4_vfs_create(fhp, child, open);
 	if (status != nfs_ok)
 		goto out;
@@ -424,11 +426,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	} else {
 		status = nfsd_lookup(rqstp, current_fh,
 				     open->op_fname, open->op_fnamelen, *resfh);
-		if (!status)
+		if (status == nfs_ok)
 			/* NFSv4 protocol requires change attributes even though
 			 * no change happened.
 			 */
-			fh_fill_both_attrs(current_fh);
+			status = fh_fill_both_attrs(current_fh);
 	}
 	if (status)
 		goto out;
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index c291389a1d71..f7e68a91e826 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -614,7 +614,7 @@ fh_update(struct svc_fh *fhp)
  * @fhp: file handle to be updated
  *
  */
-void fh_fill_pre_attrs(struct svc_fh *fhp)
+__be32 fh_fill_pre_attrs(struct svc_fh *fhp)
 {
 	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
 	struct inode *inode;
@@ -622,12 +622,12 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
 	__be32 err;
 
 	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
-		return;
+		return nfs_ok;
 
 	inode = d_inode(fhp->fh_dentry);
 	err = fh_getattr(fhp, &stat);
 	if (err)
-		return;
+		return err;
 
 	if (v4)
 		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
@@ -636,6 +636,7 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
 	fhp->fh_pre_ctime = stat.ctime;
 	fhp->fh_pre_size  = stat.size;
 	fhp->fh_pre_saved = true;
+	return nfs_ok;
 }
 
 /**
@@ -643,26 +644,27 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
  * @fhp: file handle to be updated
  *
  */
-void fh_fill_post_attrs(struct svc_fh *fhp)
+__be32 fh_fill_post_attrs(struct svc_fh *fhp)
 {
 	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
 	struct inode *inode = d_inode(fhp->fh_dentry);
 	__be32 err;
 
 	if (fhp->fh_no_wcc)
-		return;
+		return nfs_ok;
 
 	if (fhp->fh_post_saved)
 		printk("nfsd: inode locked twice during operation.\n");
 
 	err = fh_getattr(fhp, &fhp->fh_post_attr);
 	if (err)
-		return;
+		return err;
 
 	fhp->fh_post_saved = true;
 	if (v4)
 		fhp->fh_post_change =
 			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
+	return nfs_ok;
 }
 
 /**
@@ -672,16 +674,20 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
  * This is used when the directory wasn't changed, but wcc attributes
  * are needed anyway.
  */
-void fh_fill_both_attrs(struct svc_fh *fhp)
+__be32 fh_fill_both_attrs(struct svc_fh *fhp)
 {
-	fh_fill_post_attrs(fhp);
-	if (!fhp->fh_post_saved)
-		return;
+	__be32 err;
+
+	err = fh_fill_post_attrs(fhp);
+	if (err)
+		return err;
+
 	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;
+	return nfs_ok;
 }
 
 /*
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 4e0ecf0ae2cf..486803694acc 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -294,7 +294,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
 }
 
 u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode);
-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);
+__be32 fh_fill_pre_attrs(struct svc_fh *fhp);
+__be32 fh_fill_post_attrs(struct svc_fh *fhp);
+__be32 fh_fill_both_attrs(struct svc_fh *fhp);
 #endif /* _LINUX_NFSD_NFSFH_H */
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 8a2321d19194..f200afd33630 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1537,9 +1537,11 @@ 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, attrs, type, rdev, resfhp);
-	fh_fill_post_attrs(fhp);
+	err = fh_fill_pre_attrs(fhp);
+	if (err == nfs_ok) {
+		err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp);
+		fh_fill_post_attrs(fhp);
+	}
 out_unlock:
 	inode_unlock(dentry->d_inode);
 	return err;
@@ -1632,13 +1634,16 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		inode_unlock(dentry->d_inode);
 		goto out_drop_write;
 	}
-	fh_fill_pre_attrs(fhp);
+	err = fh_fill_pre_attrs(fhp);
+	if (err)
+		goto out_unlock;
 	host_err = vfs_symlink(&nop_mnt_idmap, 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_fill_post_attrs(fhp);
+out_unlock:
 	inode_unlock(dentry->d_inode);
 	if (!err)
 		err = nfserrno(commit_metadata(fhp));
@@ -1700,7 +1705,9 @@ 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);
+	err = fh_fill_pre_attrs(ffhp);
+	if (err != nfs_ok)
+		goto out_dput;
 	host_err = vfs_link(dold, &nop_mnt_idmap, dirp, dnew, NULL);
 	fh_fill_post_attrs(ffhp);
 	inode_unlock(dirp);
@@ -1786,8 +1793,12 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 	}
 
 	trap = lock_rename(tdentry, fdentry);
-	fh_fill_pre_attrs(ffhp);
-	fh_fill_pre_attrs(tfhp);
+	err = fh_fill_pre_attrs(ffhp);
+	if (err != nfs_ok)
+		goto out_unlock;
+	err = fh_fill_pre_attrs(tfhp);
+	if (err != nfs_ok)
+		goto out_unlock;
 
 	odentry = lookup_one_len(fname, fdentry, flen);
 	host_err = PTR_ERR(odentry);
@@ -1854,6 +1865,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 		fh_fill_post_attrs(ffhp);
 		fh_fill_post_attrs(tfhp);
 	}
+out_unlock:
 	unlock_rename(tdentry, fdentry);
 	fh_drop_write(ffhp);
 
@@ -1913,12 +1925,14 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 		goto out_unlock;
 	}
 	rinode = d_inode(rdentry);
-	ihold(rinode);
+	err = fh_fill_pre_attrs(fhp);
+	if (err != nfs_ok)
+		goto out_unlock;
 
+	ihold(rinode);
 	if (!type)
 		type = d_inode(rdentry)->i_mode & S_IFMT;
 
-	fh_fill_pre_attrs(fhp);
 	if (type != S_IFDIR) {
 		int retries;
 
@@ -2338,16 +2352,17 @@ nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name)
 		return nfserrno(ret);
 
 	inode_lock(fhp->fh_dentry->d_inode);
-	fh_fill_pre_attrs(fhp);
-
-	ret = __vfs_removexattr_locked(&nop_mnt_idmap, fhp->fh_dentry,
-				       name, NULL);
-
-	fh_fill_post_attrs(fhp);
+	err = fh_fill_pre_attrs(fhp);
+	if (err == nfs_ok) {
+		ret = __vfs_removexattr_locked(&nop_mnt_idmap, fhp->fh_dentry,
+					       name, NULL);
+		err = nfsd_xattr_errno(ret);
+		fh_fill_post_attrs(fhp);
+	}
 	inode_unlock(fhp->fh_dentry->d_inode);
 	fh_drop_write(fhp);
 
-	return nfsd_xattr_errno(ret);
+	return err;
 }
 
 __be32
@@ -2365,15 +2380,16 @@ nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name,
 	if (ret)
 		return nfserrno(ret);
 	inode_lock(fhp->fh_dentry->d_inode);
-	fh_fill_pre_attrs(fhp);
-
-	ret = __vfs_setxattr_locked(&nop_mnt_idmap, fhp->fh_dentry, name, buf,
-				    len, flags, NULL);
-	fh_fill_post_attrs(fhp);
+	err = fh_fill_pre_attrs(fhp);
+	if (err != nfs_ok) {
+		ret = __vfs_setxattr_locked(&nop_mnt_idmap, fhp->fh_dentry,
+					    name, buf, len, flags, NULL);
+		fh_fill_post_attrs(fhp);
+		err = nfsd_xattr_errno(ret);
+	}
 	inode_unlock(fhp->fh_dentry->d_inode);
 	fh_drop_write(fhp);
-
-	return nfsd_xattr_errno(ret);
+	return err;
 }
 #endif
 

-- 
2.41.0

[PATCH v2 2/2] nfsd: remove unsafe BUG_ON from set_change_info

[Jeff Layton]

At one time, nfsd would scrape inode information directly out of struct
inode in order to populate the change_info4. At that time, the BUG_ON in
set_change_info made some sense, since having it unset meant a coding
error.

More recently, it calls vfs_getattr to get this information, which can
fail. If that fails, fh_pre_saved can end up not being set. While this
situation is unfortunate, we don't need to crash the box.

Move set_change_info to nfs4proc.c since all of the callers are there.
Revise the condition for setting "atomic" to also check for
fh_pre_saved, and rework the rest to try and handle either flag being
missing when this occurs.

Reported-by: Boyang Xue <bxue@redhat.com>
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2223560
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4proc.c | 31 +++++++++++++++++++++++++++++++
 fs/nfsd/xdr4.h     | 11 -----------
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 9285e1eab4d5..4467be7d9c2a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -382,6 +382,37 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return status;
 }
 
+/**
+ * set_change_info - set up the change_info4 for a reply
+ * @cinfo: pointer to nfsd4_change_info to be populated
+ * @fhp: pointer to svc_fh to use as source
+ *
+ * Many operations in NFSv4 require change_info4 in the reply. This function
+ * populates that from the info that we (should!) have already collected. In
+ * the event that we didn't get any pre-attrs, just zero out both.
+ */
+static void
+set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp)
+{
+	cinfo->atomic = (u32)(fhp->fh_pre_saved && fhp->fh_post_saved && !fhp->fh_no_atomic_attr);
+	cinfo->before_change = fhp->fh_pre_change;
+	cinfo->after_change = fhp->fh_post_change;
+
+	/*
+	 * If fetching the pre-change attributes failed, then we should
+	 * have already failed the whole operation. We could have still
+	 * failed to fetch post-change attributes however.
+	 *
+	 * The pre field should be set at this point. WARN if it's
+	 * that's ever not the case. If either value is unset, then just
+	 * zero out the field since we don't have any other recourse.
+	 */
+	if (WARN_ON_ONCE(!fhp->fh_pre_saved))
+		cinfo->before_change = 0;
+	if (!fhp->fh_post_saved)
+		cinfo->after_change = 0;
+}
+
 static __be32
 do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh)
 {
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index b2931fdf53be..9e67f63c5f4d 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -775,17 +775,6 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op);
 
 #define NFS4_SVC_XDRSIZE		sizeof(struct nfsd4_compoundargs)
 
-static inline void
-set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp)
-{
-	BUG_ON(!fhp->fh_pre_saved);
-	cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr);
-
-	cinfo->before_change = fhp->fh_pre_change;
-	cinfo->after_change = fhp->fh_post_change;
-}
-
-
 bool nfsd4_mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp);
 bool nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, struct xdr_stream *xdr);
 bool nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr);

-- 
2.41.0

Re: [PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post attributes

[NeilBrown]

On Fri, 21 Jul 2023, Jeff Layton wrote:
> Boyang reported tripping the BUG_ON in set_change_info. While we
> couldn't confirm it, one way this could happen would be for nfsd_lookup
> to succeed and then for fh_fill_both_attrs to fail.
> 
> This patchset attempts to (sanely) fix this, usually by aborting the
> operation if fetching the pre attributes fails. Post-op attribute fetch
> handling is more difficult to deal with however since we've already done
> the operation, so this has it just fudge the change_info4 if that
> occurs.

I think both v3 and v4 allow a reply that says "the operation was a
success but there are no post-op attrs".  With v4 you can say "there is
no change-attr, but here are some other attrs".  I think.

Our xdr-encoding doesn't make that easy, but it is just a "simple matter
of coding".  If you think it is worth it.

NeilBrown


> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> Changes in v2:
> - make fh_fill_*_attrs return an error and have the callers handle it
> - rework of set_change_info, to better handle missing pre/post attrs
> 
> ---
> Jeff Layton (2):
>       nfsd: handle failure to collect pre/post-op attrs more sanely
>       nfsd: remove unsafe BUG_ON from set_change_info
> 
>  fs/nfsd/nfs3proc.c |  4 +++-
>  fs/nfsd/nfs4proc.c | 45 +++++++++++++++++++++++++++++++++------
>  fs/nfsd/nfsfh.c    | 26 ++++++++++++++---------
>  fs/nfsd/nfsfh.h    |  6 +++---
>  fs/nfsd/vfs.c      | 62 ++++++++++++++++++++++++++++++++++--------------------
>  fs/nfsd/xdr4.h     | 11 ----------
>  6 files changed, 100 insertions(+), 54 deletions(-)
> ---
> base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e
> change-id: 20230720-bz2223560-9c4690a8217b
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 
> 

Re: [PATCH v2 1/2] nfsd: handle failure to collect pre/post-op attrs more sanely

[NeilBrown]

On Fri, 21 Jul 2023, Jeff Layton wrote:
> Collecting pre_op_attrs can fail, in which case it's probably best to
> fail the whole operation.
> 
> Change fh_fill_{pre,post,both}_attrs to return __be32. For the pre and
> both functions, have the callers check the return code and abort the
> operation if it failed.
> 
> If fh_fill_post_attrs fails, then it's too late to do anything about it,
> so most of those callers ignore the return value. If this happens, then
> fh_post_saved will be false, which should cue the later stages to deal
> with it.
> 
> Suggested-by: Chuck Lever <chuck.lever@oracle.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs3proc.c |  4 +++-
>  fs/nfsd/nfs4proc.c | 14 ++++++------
>  fs/nfsd/nfsfh.c    | 26 ++++++++++++++---------
>  fs/nfsd/nfsfh.h    |  6 +++---
>  fs/nfsd/vfs.c      | 62 ++++++++++++++++++++++++++++++++++--------------------
>  5 files changed, 69 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index fc8d5b7db9f8..268ef57751c4 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -307,7 +307,9 @@ 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);
> +	status = fh_fill_pre_attrs(fhp);
> +	if (status != nfs_ok)
> +		goto out;
>  	host_err = vfs_create(&nop_mnt_idmap, inode, child, iap->ia_mode, true);
>  	if (host_err < 0) {
>  		status = nfserrno(host_err);
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index d8e7a533f9d2..9285e1eab4d5 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -297,12 +297,12 @@ 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);
> +		status = fh_fill_both_attrs(fhp);
> +		if (status != nfs_ok)
> +			goto out;
>  
>  		switch (open->op_createmode) {
>  		case NFS4_CREATE_UNCHECKED:
> @@ -345,7 +345,9 @@ 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 = fh_fill_pre_attrs(fhp);
> +	if (status != nfs_ok)
> +		goto out;
>  	status = nfsd4_vfs_create(fhp, child, open);
>  	if (status != nfs_ok)
>  		goto out;
> @@ -424,11 +426,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
>  	} else {
>  		status = nfsd_lookup(rqstp, current_fh,
>  				     open->op_fname, open->op_fnamelen, *resfh);
> -		if (!status)
> +		if (status == nfs_ok)
>  			/* NFSv4 protocol requires change attributes even though
>  			 * no change happened.
>  			 */
> -			fh_fill_both_attrs(current_fh);
> +			status = fh_fill_both_attrs(current_fh);
>  	}
>  	if (status)
>  		goto out;
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index c291389a1d71..f7e68a91e826 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -614,7 +614,7 @@ fh_update(struct svc_fh *fhp)
>   * @fhp: file handle to be updated
>   *
>   */
> -void fh_fill_pre_attrs(struct svc_fh *fhp)
> +__be32 fh_fill_pre_attrs(struct svc_fh *fhp)
>  {
>  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
>  	struct inode *inode;
> @@ -622,12 +622,12 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
>  	__be32 err;
>  
>  	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
> -		return;
> +		return nfs_ok;
>  
>  	inode = d_inode(fhp->fh_dentry);
>  	err = fh_getattr(fhp, &stat);
>  	if (err)
> -		return;
> +		return err;
>  
>  	if (v4)
>  		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
> @@ -636,6 +636,7 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
>  	fhp->fh_pre_ctime = stat.ctime;
>  	fhp->fh_pre_size  = stat.size;
>  	fhp->fh_pre_saved = true;
> +	return nfs_ok;
>  }
>  
>  /**
> @@ -643,26 +644,27 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
>   * @fhp: file handle to be updated
>   *
>   */
> -void fh_fill_post_attrs(struct svc_fh *fhp)
> +__be32 fh_fill_post_attrs(struct svc_fh *fhp)
>  {
>  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
>  	struct inode *inode = d_inode(fhp->fh_dentry);
>  	__be32 err;
>  
>  	if (fhp->fh_no_wcc)
> -		return;
> +		return nfs_ok;
>  
>  	if (fhp->fh_post_saved)
>  		printk("nfsd: inode locked twice during operation.\n");
>  
>  	err = fh_getattr(fhp, &fhp->fh_post_attr);
>  	if (err)
> -		return;
> +		return err;
>  
>  	fhp->fh_post_saved = true;
>  	if (v4)
>  		fhp->fh_post_change =
>  			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> +	return nfs_ok;
>  }
>  
>  /**
> @@ -672,16 +674,20 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
>   * This is used when the directory wasn't changed, but wcc attributes
>   * are needed anyway.
>   */
> -void fh_fill_both_attrs(struct svc_fh *fhp)
> +__be32 fh_fill_both_attrs(struct svc_fh *fhp)
>  {
> -	fh_fill_post_attrs(fhp);
> -	if (!fhp->fh_post_saved)
> -		return;
> +	__be32 err;
> +
> +	err = fh_fill_post_attrs(fhp);
> +	if (err)
> +		return err;
> +
>  	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;
> +	return nfs_ok;
>  }
>  
>  /*
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 4e0ecf0ae2cf..486803694acc 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -294,7 +294,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
>  }
>  
>  u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode);
> -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);
> +__be32 fh_fill_pre_attrs(struct svc_fh *fhp);
> +__be32 fh_fill_post_attrs(struct svc_fh *fhp);
> +__be32 fh_fill_both_attrs(struct svc_fh *fhp);
>  #endif /* _LINUX_NFSD_NFSFH_H */
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 8a2321d19194..f200afd33630 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1537,9 +1537,11 @@ 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, attrs, type, rdev, resfhp);
> -	fh_fill_post_attrs(fhp);
> +	err = fh_fill_pre_attrs(fhp);
> +	if (err == nfs_ok) {
> +		err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp);
> +		fh_fill_post_attrs(fhp);

Most error handling in nfsd is
 
   if (err)
       goto ....

Here (and one other place I think) you have
   if (not err)
       do stuff;

Do we want to be more consistent?  I'm in two minds about this and I
don't dislike your patch.  But I noticed the inconsistency and thought I
should mention it.

Also, should we put a __must_check annotation on fh_fill_pre_attrs() and
..post..?  Then I wouldn't have to manually check that you found and
fixed all callers (which I haven't).

Thanks,
NeilBrown

Re: [PATCH v2 1/2] nfsd: handle failure to collect pre/post-op attrs more sanely

[Chuck Lever]

On Fri, Jul 21, 2023 at 07:46:20AM +1000, NeilBrown wrote:
> On Fri, 21 Jul 2023, Jeff Layton wrote:
> > Collecting pre_op_attrs can fail, in which case it's probably best to
> > fail the whole operation.
> > 
> > Change fh_fill_{pre,post,both}_attrs to return __be32. For the pre and
> > both functions, have the callers check the return code and abort the
> > operation if it failed.
> > 
> > If fh_fill_post_attrs fails, then it's too late to do anything about it,
> > so most of those callers ignore the return value. If this happens, then
> > fh_post_saved will be false, which should cue the later stages to deal
> > with it.
> > 
> > Suggested-by: Chuck Lever <chuck.lever@oracle.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs3proc.c |  4 +++-
> >  fs/nfsd/nfs4proc.c | 14 ++++++------
> >  fs/nfsd/nfsfh.c    | 26 ++++++++++++++---------
> >  fs/nfsd/nfsfh.h    |  6 +++---
> >  fs/nfsd/vfs.c      | 62 ++++++++++++++++++++++++++++++++++--------------------
> >  5 files changed, 69 insertions(+), 43 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > index fc8d5b7db9f8..268ef57751c4 100644
> > --- a/fs/nfsd/nfs3proc.c
> > +++ b/fs/nfsd/nfs3proc.c
> > @@ -307,7 +307,9 @@ 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);
> > +	status = fh_fill_pre_attrs(fhp);
> > +	if (status != nfs_ok)
> > +		goto out;
> >  	host_err = vfs_create(&nop_mnt_idmap, inode, child, iap->ia_mode, true);
> >  	if (host_err < 0) {
> >  		status = nfserrno(host_err);
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index d8e7a533f9d2..9285e1eab4d5 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -297,12 +297,12 @@ 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);
> > +		status = fh_fill_both_attrs(fhp);
> > +		if (status != nfs_ok)
> > +			goto out;
> >  
> >  		switch (open->op_createmode) {
> >  		case NFS4_CREATE_UNCHECKED:
> > @@ -345,7 +345,9 @@ 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 = fh_fill_pre_attrs(fhp);
> > +	if (status != nfs_ok)
> > +		goto out;
> >  	status = nfsd4_vfs_create(fhp, child, open);
> >  	if (status != nfs_ok)
> >  		goto out;
> > @@ -424,11 +426,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> >  	} else {
> >  		status = nfsd_lookup(rqstp, current_fh,
> >  				     open->op_fname, open->op_fnamelen, *resfh);
> > -		if (!status)
> > +		if (status == nfs_ok)
> >  			/* NFSv4 protocol requires change attributes even though
> >  			 * no change happened.
> >  			 */
> > -			fh_fill_both_attrs(current_fh);
> > +			status = fh_fill_both_attrs(current_fh);
> >  	}
> >  	if (status)
> >  		goto out;
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index c291389a1d71..f7e68a91e826 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -614,7 +614,7 @@ fh_update(struct svc_fh *fhp)
> >   * @fhp: file handle to be updated
> >   *
> >   */
> > -void fh_fill_pre_attrs(struct svc_fh *fhp)
> > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp)
> >  {
> >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> >  	struct inode *inode;
> > @@ -622,12 +622,12 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> >  	__be32 err;
> >  
> >  	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
> > -		return;
> > +		return nfs_ok;
> >  
> >  	inode = d_inode(fhp->fh_dentry);
> >  	err = fh_getattr(fhp, &stat);
> >  	if (err)
> > -		return;
> > +		return err;
> >  
> >  	if (v4)
> >  		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
> > @@ -636,6 +636,7 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> >  	fhp->fh_pre_ctime = stat.ctime;
> >  	fhp->fh_pre_size  = stat.size;
> >  	fhp->fh_pre_saved = true;
> > +	return nfs_ok;
> >  }
> >  
> >  /**
> > @@ -643,26 +644,27 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> >   * @fhp: file handle to be updated
> >   *
> >   */
> > -void fh_fill_post_attrs(struct svc_fh *fhp)
> > +__be32 fh_fill_post_attrs(struct svc_fh *fhp)
> >  {
> >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> >  	struct inode *inode = d_inode(fhp->fh_dentry);
> >  	__be32 err;
> >  
> >  	if (fhp->fh_no_wcc)
> > -		return;
> > +		return nfs_ok;
> >  
> >  	if (fhp->fh_post_saved)
> >  		printk("nfsd: inode locked twice during operation.\n");
> >  
> >  	err = fh_getattr(fhp, &fhp->fh_post_attr);
> >  	if (err)
> > -		return;
> > +		return err;
> >  
> >  	fhp->fh_post_saved = true;
> >  	if (v4)
> >  		fhp->fh_post_change =
> >  			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> > +	return nfs_ok;
> >  }
> >  
> >  /**
> > @@ -672,16 +674,20 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
> >   * This is used when the directory wasn't changed, but wcc attributes
> >   * are needed anyway.
> >   */
> > -void fh_fill_both_attrs(struct svc_fh *fhp)
> > +__be32 fh_fill_both_attrs(struct svc_fh *fhp)
> >  {
> > -	fh_fill_post_attrs(fhp);
> > -	if (!fhp->fh_post_saved)
> > -		return;
> > +	__be32 err;
> > +
> > +	err = fh_fill_post_attrs(fhp);
> > +	if (err)
> > +		return err;
> > +
> >  	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;
> > +	return nfs_ok;
> >  }
> >  
> >  /*
> > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> > index 4e0ecf0ae2cf..486803694acc 100644
> > --- a/fs/nfsd/nfsfh.h
> > +++ b/fs/nfsd/nfsfh.h
> > @@ -294,7 +294,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
> >  }
> >  
> >  u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode);
> > -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);
> > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp);
> > +__be32 fh_fill_post_attrs(struct svc_fh *fhp);
> > +__be32 fh_fill_both_attrs(struct svc_fh *fhp);
> >  #endif /* _LINUX_NFSD_NFSFH_H */
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 8a2321d19194..f200afd33630 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1537,9 +1537,11 @@ 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, attrs, type, rdev, resfhp);
> > -	fh_fill_post_attrs(fhp);
> > +	err = fh_fill_pre_attrs(fhp);
> > +	if (err == nfs_ok) {
> > +		err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp);
> > +		fh_fill_post_attrs(fhp);
> 
> Most error handling in nfsd is
>  
>    if (err)
>        goto ....
> 
> Here (and one other place I think) you have
>    if (not err)
>        do stuff;
> 
> Do we want to be more consistent?

Yes, unless being consistent makes this code unreadable. There
doesn't seem to be a reason to drop that convention here.


> I'm in two minds about this and I
> don't dislike your patch.  But I noticed the inconsistency and thought I
> should mention it.
> 
> Also, should we put a __must_check annotation on fh_fill_pre_attrs() and
> ..post..?  Then I wouldn't have to manually check that you found and
> fixed all callers (which I haven't).

Re: [PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post attributes

[Chuck Lever]

On Fri, Jul 21, 2023 at 07:42:47AM +1000, NeilBrown wrote:
> On Fri, 21 Jul 2023, Jeff Layton wrote:
> > Boyang reported tripping the BUG_ON in set_change_info. While we
> > couldn't confirm it, one way this could happen would be for nfsd_lookup
> > to succeed and then for fh_fill_both_attrs to fail.
> > 
> > This patchset attempts to (sanely) fix this, usually by aborting the
> > operation if fetching the pre attributes fails. Post-op attribute fetch
> > handling is more difficult to deal with however since we've already done
> > the operation, so this has it just fudge the change_info4 if that
> > occurs.
> 
> I think both v3 and v4 allow a reply that says "the operation was a
> success but there are no post-op attrs".  With v4 you can say "there is
> no change-attr, but here are some other attrs".  I think.

If the protocols enable NFSD to avoid returning made-up values, I'm
all for it.


> Our xdr-encoding doesn't make that easy, but it is just a "simple matter
> of coding".  If you think it is worth it.
> 
> NeilBrown
> 
> 
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > Changes in v2:
> > - make fh_fill_*_attrs return an error and have the callers handle it
> > - rework of set_change_info, to better handle missing pre/post attrs
> > 
> > ---
> > Jeff Layton (2):
> >       nfsd: handle failure to collect pre/post-op attrs more sanely
> >       nfsd: remove unsafe BUG_ON from set_change_info
> > 
> >  fs/nfsd/nfs3proc.c |  4 +++-
> >  fs/nfsd/nfs4proc.c | 45 +++++++++++++++++++++++++++++++++------
> >  fs/nfsd/nfsfh.c    | 26 ++++++++++++++---------
> >  fs/nfsd/nfsfh.h    |  6 +++---
> >  fs/nfsd/vfs.c      | 62 ++++++++++++++++++++++++++++++++++--------------------
> >  fs/nfsd/xdr4.h     | 11 ----------
> >  6 files changed, 100 insertions(+), 54 deletions(-)
> > ---
> > base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e
> > change-id: 20230720-bz2223560-9c4690a8217b
> > 
> > Best regards,
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> > 
> 

Re: [PATCH v2 1/2] nfsd: handle failure to collect pre/post-op attrs more sanely

[Jeff Layton]

On Thu, 2023-07-20 at 19:09 -0400, Chuck Lever wrote:
> On Fri, Jul 21, 2023 at 07:46:20AM +1000, NeilBrown wrote:
> > On Fri, 21 Jul 2023, Jeff Layton wrote:
> > > Collecting pre_op_attrs can fail, in which case it's probably best to
> > > fail the whole operation.
> > > 
> > > Change fh_fill_{pre,post,both}_attrs to return __be32. For the pre and
> > > both functions, have the callers check the return code and abort the
> > > operation if it failed.
> > > 
> > > If fh_fill_post_attrs fails, then it's too late to do anything about it,
> > > so most of those callers ignore the return value. If this happens, then
> > > fh_post_saved will be false, which should cue the later stages to deal
> > > with it.
> > > 
> > > Suggested-by: Chuck Lever <chuck.lever@oracle.com>
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/nfsd/nfs3proc.c |  4 +++-
> > >  fs/nfsd/nfs4proc.c | 14 ++++++------
> > >  fs/nfsd/nfsfh.c    | 26 ++++++++++++++---------
> > >  fs/nfsd/nfsfh.h    |  6 +++---
> > >  fs/nfsd/vfs.c      | 62 ++++++++++++++++++++++++++++++++++--------------------
> > >  5 files changed, 69 insertions(+), 43 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > > index fc8d5b7db9f8..268ef57751c4 100644
> > > --- a/fs/nfsd/nfs3proc.c
> > > +++ b/fs/nfsd/nfs3proc.c
> > > @@ -307,7 +307,9 @@ 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);
> > > +	status = fh_fill_pre_attrs(fhp);
> > > +	if (status != nfs_ok)
> > > +		goto out;
> > >  	host_err = vfs_create(&nop_mnt_idmap, inode, child, iap->ia_mode, true);
> > >  	if (host_err < 0) {
> > >  		status = nfserrno(host_err);
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index d8e7a533f9d2..9285e1eab4d5 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -297,12 +297,12 @@ 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);
> > > +		status = fh_fill_both_attrs(fhp);
> > > +		if (status != nfs_ok)
> > > +			goto out;
> > >  
> > >  		switch (open->op_createmode) {
> > >  		case NFS4_CREATE_UNCHECKED:
> > > @@ -345,7 +345,9 @@ 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 = fh_fill_pre_attrs(fhp);
> > > +	if (status != nfs_ok)
> > > +		goto out;
> > >  	status = nfsd4_vfs_create(fhp, child, open);
> > >  	if (status != nfs_ok)
> > >  		goto out;
> > > @@ -424,11 +426,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> > >  	} else {
> > >  		status = nfsd_lookup(rqstp, current_fh,
> > >  				     open->op_fname, open->op_fnamelen, *resfh);
> > > -		if (!status)
> > > +		if (status == nfs_ok)
> > >  			/* NFSv4 protocol requires change attributes even though
> > >  			 * no change happened.
> > >  			 */
> > > -			fh_fill_both_attrs(current_fh);
> > > +			status = fh_fill_both_attrs(current_fh);
> > >  	}
> > >  	if (status)
> > >  		goto out;
> > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > > index c291389a1d71..f7e68a91e826 100644
> > > --- a/fs/nfsd/nfsfh.c
> > > +++ b/fs/nfsd/nfsfh.c
> > > @@ -614,7 +614,7 @@ fh_update(struct svc_fh *fhp)
> > >   * @fhp: file handle to be updated
> > >   *
> > >   */
> > > -void fh_fill_pre_attrs(struct svc_fh *fhp)
> > > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp)
> > >  {
> > >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> > >  	struct inode *inode;
> > > @@ -622,12 +622,12 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> > >  	__be32 err;
> > >  
> > >  	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
> > > -		return;
> > > +		return nfs_ok;
> > >  
> > >  	inode = d_inode(fhp->fh_dentry);
> > >  	err = fh_getattr(fhp, &stat);
> > >  	if (err)
> > > -		return;
> > > +		return err;
> > >  
> > >  	if (v4)
> > >  		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
> > > @@ -636,6 +636,7 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> > >  	fhp->fh_pre_ctime = stat.ctime;
> > >  	fhp->fh_pre_size  = stat.size;
> > >  	fhp->fh_pre_saved = true;
> > > +	return nfs_ok;
> > >  }
> > >  
> > >  /**
> > > @@ -643,26 +644,27 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> > >   * @fhp: file handle to be updated
> > >   *
> > >   */
> > > -void fh_fill_post_attrs(struct svc_fh *fhp)
> > > +__be32 fh_fill_post_attrs(struct svc_fh *fhp)
> > >  {
> > >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> > >  	struct inode *inode = d_inode(fhp->fh_dentry);
> > >  	__be32 err;
> > >  
> > >  	if (fhp->fh_no_wcc)
> > > -		return;
> > > +		return nfs_ok;
> > >  
> > >  	if (fhp->fh_post_saved)
> > >  		printk("nfsd: inode locked twice during operation.\n");
> > >  
> > >  	err = fh_getattr(fhp, &fhp->fh_post_attr);
> > >  	if (err)
> > > -		return;
> > > +		return err;
> > >  
> > >  	fhp->fh_post_saved = true;
> > >  	if (v4)
> > >  		fhp->fh_post_change =
> > >  			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> > > +	return nfs_ok;
> > >  }
> > >  
> > >  /**
> > > @@ -672,16 +674,20 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
> > >   * This is used when the directory wasn't changed, but wcc attributes
> > >   * are needed anyway.
> > >   */
> > > -void fh_fill_both_attrs(struct svc_fh *fhp)
> > > +__be32 fh_fill_both_attrs(struct svc_fh *fhp)
> > >  {
> > > -	fh_fill_post_attrs(fhp);
> > > -	if (!fhp->fh_post_saved)
> > > -		return;
> > > +	__be32 err;
> > > +
> > > +	err = fh_fill_post_attrs(fhp);
> > > +	if (err)
> > > +		return err;
> > > +
> > >  	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;
> > > +	return nfs_ok;
> > >  }
> > >  
> > >  /*
> > > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> > > index 4e0ecf0ae2cf..486803694acc 100644
> > > --- a/fs/nfsd/nfsfh.h
> > > +++ b/fs/nfsd/nfsfh.h
> > > @@ -294,7 +294,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
> > >  }
> > >  
> > >  u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode);
> > > -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);
> > > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp);
> > > +__be32 fh_fill_post_attrs(struct svc_fh *fhp);
> > > +__be32 fh_fill_both_attrs(struct svc_fh *fhp);
> > >  #endif /* _LINUX_NFSD_NFSFH_H */
> > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > index 8a2321d19194..f200afd33630 100644
> > > --- a/fs/nfsd/vfs.c
> > > +++ b/fs/nfsd/vfs.c
> > > @@ -1537,9 +1537,11 @@ 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, attrs, type, rdev, resfhp);
> > > -	fh_fill_post_attrs(fhp);
> > > +	err = fh_fill_pre_attrs(fhp);
> > > +	if (err == nfs_ok) {
> > > +		err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp);
> > > +		fh_fill_post_attrs(fhp);
> > 
> > Most error handling in nfsd is
> >  
> >    if (err)
> >        goto ....
> > 
> > Here (and one other place I think) you have
> >    if (not err)
> >        do stuff;
> > 
> > Do we want to be more consistent?
> 
> Yes, unless being consistent makes this code unreadable. There
> doesn't seem to be a reason to drop that convention here.
> 

My usual test for this is to use gotos if unwinding errors is complex
enough to warrant it, and to just use the second form if the code is
fairly simple.

But...if you want gotos everywhere, then so be it. I'll respin this.

> 
> > I'm in two minds about this and I
> > don't dislike your patch.  But I noticed the inconsistency and thought I
> > should mention it.
> > 
> > Also, should we put a __must_check annotation on fh_fill_pre_attrs() and
> > ..post..?  Then I wouldn't have to manually check that you found and
> > fixed all callers (which I haven't).

Maybe for the "pre" and "both" ones. We would _not_ want to add
__must_check for the post one, since most of the callers (correctly)
ignore that return value.

I'll plan to roll that in.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post attributes

[Jeff Layton]

On Fri, 2023-07-21 at 07:42 +1000, NeilBrown wrote:
> On Fri, 21 Jul 2023, Jeff Layton wrote:
> > Boyang reported tripping the BUG_ON in set_change_info. While we
> > couldn't confirm it, one way this could happen would be for nfsd_lookup
> > to succeed and then for fh_fill_both_attrs to fail.
> > 
> > This patchset attempts to (sanely) fix this, usually by aborting the
> > operation if fetching the pre attributes fails. Post-op attribute fetch
> > handling is more difficult to deal with however since we've already done
> > the operation, so this has it just fudge the change_info4 if that
> > occurs.
> 
> I think both v3 and v4 allow a reply that says "the operation was a
> success but there are no post-op attrs".  With v4 you can say "there is
> no change-attr, but here are some other attrs".  I think.
> 

v3 has this ability:

      union pre_op_attr switch (bool attributes_follow) {
      case TRUE:
           wcc_attr  attributes;
      case FALSE:
           void;
      };

...we can just set the attributes_follow flag to false there in that
case.

That's not possible with v4, AFAICT. Several of the *4resok structures
contain a change_info4, which just looks like this:

struct change_info4 {
        bool            atomic;
        changeid4       before;
        changeid4       after;
};

We can set "atomic" to false (and this patch does that in this
situation), but I don't believe there is any alternative to the change
attribute. If the underlying fs doesn't support native change attrs, the
server is expected to fake one up somehow (usually from the ctime).

We could (in principle) allow the operation to proceed on v3 even if
fh_fill_pre_attrs fails, but I don't think we can do the same thing with
v4. That said, if getattr is failing then it's somewhat likely that
other operations will fail too, so aborting the operation in this
situation doesn't seem too onerous.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post attributes

[NeilBrown]

On Fri, 21 Jul 2023, Jeff Layton wrote:
> On Fri, 2023-07-21 at 07:42 +1000, NeilBrown wrote:
> > 
> > I think both v3 and v4 allow a reply that says "the operation was a
> > success but there are no post-op attrs".  With v4 you can say "there is
> > no change-attr, but here are some other attrs".  I think.
> > 
> 
> v3 has this ability:
> 
>       union pre_op_attr switch (bool attributes_follow) {
>       case TRUE:
>            wcc_attr  attributes;
>       case FALSE:
>            void;
>       };
> 
> ...we can just set the attributes_follow flag to false there in that
> case.
> 
> That's not possible with v4, AFAICT. Several of the *4resok structures
> contain a change_info4, which just looks like this:
> 
> struct change_info4 {
>         bool            atomic;
>         changeid4       before;
>         changeid4       after;
> };

Yes...  I was thinking of GETATTR which reports a bitmap of all the
attributes that it can return.  Though I'm not sure if the server is
"allowed" to not return something that it has said is "supported".  And
I think changeid has to be "supported".  I'm not sure.

But anyway, that doesn't help change_info4 which comes with
directory-modifying operation.

> 
> We can set "atomic" to false (and this patch does that in this
> situation), but I don't believe there is any alternative to the change
> attribute. If the underlying fs doesn't support native change attrs, the
> server is expected to fake one up somehow (usually from the ctime).

I had a look again at the current code and your patch, and I think that
if the "post' vfs_getattr() fails, then the operation succeeds, the
change_info is marked non-atomic (as you say) and the "after" changeid is
set to an uninitialised value.  Is that right?  Did I miss something?
Maybe we should set it to the pre value plus 1.

It probably doesn't matter at all in practice, but if I'm right and it
is using an uninitialized value, we should at least fix that.

Thanks - your v3 patch looks good in general.  I like the must_check and
the goto structure.

Thanks,
NeilBrown


> 
> We could (in principle) allow the operation to proceed on v3 even if
> fh_fill_pre_attrs fails, but I don't think we can do the same thing with
> v4. That said, if getattr is failing then it's somewhat likely that
> other operations will fail too, so aborting the operation in this
> situation doesn't seem too onerous.
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

Re: [PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post attributes

[Jeff Layton]

On Sat, 2023-07-22 at 10:34 +1000, NeilBrown wrote:
> On Fri, 21 Jul 2023, Jeff Layton wrote:
> > On Fri, 2023-07-21 at 07:42 +1000, NeilBrown wrote:
> > > 
> > > I think both v3 and v4 allow a reply that says "the operation was a
> > > success but there are no post-op attrs".  With v4 you can say "there is
> > > no change-attr, but here are some other attrs".  I think.
> > > 
> > 
> > v3 has this ability:
> > 
> >       union pre_op_attr switch (bool attributes_follow) {
> >       case TRUE:
> >            wcc_attr  attributes;
> >       case FALSE:
> >            void;
> >       };
> > 
> > ...we can just set the attributes_follow flag to false there in that
> > case.
> > 
> > That's not possible with v4, AFAICT. Several of the *4resok structures
> > contain a change_info4, which just looks like this:
> > 
> > struct change_info4 {
> >         bool            atomic;
> >         changeid4       before;
> >         changeid4       after;
> > };
> 
> Yes...  I was thinking of GETATTR which reports a bitmap of all the
> attributes that it can return.  Though I'm not sure if the server is
> "allowed" to not return something that it has said is "supported".  And
> I think changeid has to be "supported".  I'm not sure.
>
> But anyway, that doesn't help change_info4 which comes with
> directory-modifying operation.
> 
> > 
> > We can set "atomic" to false (and this patch does that in this
> > situation), but I don't believe there is any alternative to the change
> > attribute. If the underlying fs doesn't support native change attrs, the
> > server is expected to fake one up somehow (usually from the ctime).
> 
> I had a look again at the current code and your patch, and I think that
> if the "post' vfs_getattr() fails, then the operation succeeds, the
> change_info is marked non-atomic (as you say) and the "after" changeid is
> set to an uninitialised value.  Is that right?  Did I miss something?
> Maybe we should set it to the pre value plus 1.
> 
> It probably doesn't matter at all in practice, but if I'm right and it
> is using an uninitialized value, we should at least fix that.
> 
> Thanks - your v3 patch looks good in general.  I like the must_check and
> the goto structure.
> 
> Thanks,
> NeilBrown
> 
> 


The current patch sets the missing pre/post values to 0. I'm happy to
change that to pre-value+1 though if you think that'd be more correct.
The client already fudges the changeid like that in the CB_GETATTR case,
so I doubt that would break anything.
-- 
Jeff Layton <jlayton@kernel.org>