The inode that nfs4_open_delegation() passes to this function is
wrong, which throws off the result. The inode will end up getting a
directory-style change attr instead of a regular-file-style one.

Fix up nfs4_delegation_stat() to fetch STATX_MODE, and then drop the
inode parameter from nfsd4_change_attribute(), since it's no longer
needed.

Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
This version should apply cleanly to v6.12-rc7. Some later patches in
nfsd-next might need to be twiddled as a result, but it should be simple
to fix. Also, I fixed up the Fixes: to point to the right commit. This
dates back a bit further than I had originally thought.
---
 fs/nfsd/nfs4state.c |  5 ++---
 fs/nfsd/nfs4xdr.c   |  2 +-
 fs/nfsd/nfsfh.c     | 20 ++++++++++++--------
 fs/nfsd/nfsfh.h     |  3 +--
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 551d2958ec2905be51b4a96414a15a5e4f87f9ea..d3cfc6471539932fadc01a95a1fe0948f2935666 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5957,7 +5957,7 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
 	path.dentry = file_dentry(nf->nf_file);
 
 	rc = vfs_getattr(&path, stat,
-			 (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
+			 (STATX_MODE | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
 			 AT_STATX_SYNC_AS_STAT);
 
 	nfsd_file_put(nf);
@@ -6041,8 +6041,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 		}
 		open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE;
 		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
-		dp->dl_cb_fattr.ncf_initial_cinfo =
-			nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry));
+		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
 		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
 	} else {
 		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index f118921250c3163ea45b77a53dc57ef364eec32b..8d25aef51ad150625540e1b8baba8baf9d64b788 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3040,7 +3040,7 @@ static __be32 nfsd4_encode_fattr4_change(struct xdr_stream *xdr,
 		return nfs_ok;
 	}
 
-	c = nfsd4_change_attribute(&args->stat, d_inode(args->dentry));
+	c = nfsd4_change_attribute(&args->stat);
 	return nfsd4_encode_changeid4(xdr, c);
 }
 
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 40ad58a6a0361e48a48262a2c61abbcfd908a3bb..96e19c50a5d7ee8cd610bec4ecaec617286deea3 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -667,20 +667,18 @@ fh_update(struct svc_fh *fhp)
 __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
 {
 	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
-	struct inode *inode;
 	struct kstat stat;
 	__be32 err;
 
 	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
 		return nfs_ok;
 
-	inode = d_inode(fhp->fh_dentry);
 	err = fh_getattr(fhp, &stat);
 	if (err)
 		return err;
 
 	if (v4)
-		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
+		fhp->fh_pre_change = nfsd4_change_attribute(&stat);
 
 	fhp->fh_pre_mtime = stat.mtime;
 	fhp->fh_pre_ctime = stat.ctime;
@@ -697,7 +695,6 @@ __be32 __must_check fh_fill_pre_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)
@@ -713,7 +710,7 @@ __be32 fh_fill_post_attrs(struct svc_fh *fhp)
 	fhp->fh_post_saved = true;
 	if (v4)
 		fhp->fh_post_change =
-			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
+			nfsd4_change_attribute(&fhp->fh_post_attr);
 	return nfs_ok;
 }
 
@@ -804,7 +801,14 @@ enum fsid_source fsid_source(const struct svc_fh *fhp)
 	return FSIDSOURCE_DEV;
 }
 
-/*
+/**
+ * nfsd4_change_attribute - Generate an NFSv4 change_attribute value
+ * @stat: inode attributes
+ *
+ * Caller must fill in @stat before calling, typically by invoking
+ * vfs_getattr() with STATX_MODE, STATX_CTIME, and STATX_CHANGE_COOKIE.
+ * Returns an unsigned 64-bit changeid4 value (RFC 8881 Section 3.2).
+ *
  * We could use i_version alone as the change attribute.  However, i_version
  * can go backwards on a regular file after an unclean shutdown.  On its own
  * that doesn't necessarily cause a problem, but if i_version goes backwards
@@ -821,13 +825,13 @@ enum fsid_source fsid_source(const struct svc_fh *fhp)
  * assume that the new change attr is always logged to stable storage in some
  * fashion before the results can be seen.
  */
-u64 nfsd4_change_attribute(const struct kstat *stat, const struct inode *inode)
+u64 nfsd4_change_attribute(const struct kstat *stat)
 {
 	u64 chattr;
 
 	if (stat->result_mask & STATX_CHANGE_COOKIE) {
 		chattr = stat->change_cookie;
-		if (S_ISREG(inode->i_mode) &&
+		if (S_ISREG(stat->mode) &&
 		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
 			chattr += (u64)stat->ctime.tv_sec << 30;
 			chattr += stat->ctime.tv_nsec;
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 5b7394801dc4270dbd5236f3e2f2237130c73dad..876152a91f122f83fb94ffdfb0eedf8fca56a20c 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -297,8 +297,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
 	fhp->fh_pre_saved = false;
 }
 
-u64 nfsd4_change_attribute(const struct kstat *stat,
-			   const struct inode *inode);
+u64 nfsd4_change_attribute(const struct kstat *stat);
 __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp);
 __be32 fh_fill_post_attrs(struct svc_fh *fhp);
 __be32 __must_check fh_fill_both_attrs(struct svc_fh *fhp);

---
base-commit: 2d5404caa8c7bb5c4e0435f94b28834ae5456623
change-id: 20241111-master-cb9afaab4ca0

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

On Mon, Nov 11, 2024 at 11:01:13AM -0500, Jeff Layton wrote:
> The inode that nfs4_open_delegation() passes to this function is
> wrong, which throws off the result. The inode will end up getting a
> directory-style change attr instead of a regular-file-style one.
> 
> Fix up nfs4_delegation_stat() to fetch STATX_MODE, and then drop the
> inode parameter from nfsd4_change_attribute(), since it's no longer
> needed.
> 
> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> This version should apply cleanly to v6.12-rc7. Some later patches in
> nfsd-next might need to be twiddled as a result, but it should be simple
> to fix. Also, I fixed up the Fixes: to point to the right commit. This
> dates back a bit further than I had originally thought.

Ah. The Fixes: change makes this interesting. If we're not
addressing a fix that is limited to in 12-rc, then I lean more
towards getting this in via a normal merge window.

Also, it's pretty late in the -rc window to take fixes that are more
than a few lines. I would like to see changes like this get some
time in linux-next etc etc (which it has already done for v6.13, but
would be difficult to guarantee for v6.12-rc at this point).

So then that changes my concern about this to only reducing the
number of pre-requisite patches that will need to be backported to
cleanly apply this fix to LTS kernels.

So about this:

 - Apply this version of the patch to nfsd-next, but earlier in the
   series

 - Fix up the later patches, as you mentioned above

 - Then let automation grab it for LTS 6.11 and 6.12

Does that sound over-caffeinated, or would you be OK if I reordered
nfsd-next as described here?


> ---
>  fs/nfsd/nfs4state.c |  5 ++---
>  fs/nfsd/nfs4xdr.c   |  2 +-
>  fs/nfsd/nfsfh.c     | 20 ++++++++++++--------
>  fs/nfsd/nfsfh.h     |  3 +--
>  4 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 551d2958ec2905be51b4a96414a15a5e4f87f9ea..d3cfc6471539932fadc01a95a1fe0948f2935666 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5957,7 +5957,7 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>  	path.dentry = file_dentry(nf->nf_file);
>  
>  	rc = vfs_getattr(&path, stat,
> -			 (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
> +			 (STATX_MODE | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
>  			 AT_STATX_SYNC_AS_STAT);
>  
>  	nfsd_file_put(nf);
> @@ -6041,8 +6041,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  		}
>  		open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE;
>  		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
> -		dp->dl_cb_fattr.ncf_initial_cinfo =
> -			nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry));
> +		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
>  		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
>  	} else {
>  		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index f118921250c3163ea45b77a53dc57ef364eec32b..8d25aef51ad150625540e1b8baba8baf9d64b788 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3040,7 +3040,7 @@ static __be32 nfsd4_encode_fattr4_change(struct xdr_stream *xdr,
>  		return nfs_ok;
>  	}
>  
> -	c = nfsd4_change_attribute(&args->stat, d_inode(args->dentry));
> +	c = nfsd4_change_attribute(&args->stat);
>  	return nfsd4_encode_changeid4(xdr, c);
>  }
>  
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 40ad58a6a0361e48a48262a2c61abbcfd908a3bb..96e19c50a5d7ee8cd610bec4ecaec617286deea3 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -667,20 +667,18 @@ fh_update(struct svc_fh *fhp)
>  __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
>  {
>  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> -	struct inode *inode;
>  	struct kstat stat;
>  	__be32 err;
>  
>  	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
>  		return nfs_ok;
>  
> -	inode = d_inode(fhp->fh_dentry);
>  	err = fh_getattr(fhp, &stat);
>  	if (err)
>  		return err;
>  
>  	if (v4)
> -		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
> +		fhp->fh_pre_change = nfsd4_change_attribute(&stat);
>  
>  	fhp->fh_pre_mtime = stat.mtime;
>  	fhp->fh_pre_ctime = stat.ctime;
> @@ -697,7 +695,6 @@ __be32 __must_check fh_fill_pre_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)
> @@ -713,7 +710,7 @@ __be32 fh_fill_post_attrs(struct svc_fh *fhp)
>  	fhp->fh_post_saved = true;
>  	if (v4)
>  		fhp->fh_post_change =
> -			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> +			nfsd4_change_attribute(&fhp->fh_post_attr);
>  	return nfs_ok;
>  }
>  
> @@ -804,7 +801,14 @@ enum fsid_source fsid_source(const struct svc_fh *fhp)
>  	return FSIDSOURCE_DEV;
>  }
>  
> -/*
> +/**
> + * nfsd4_change_attribute - Generate an NFSv4 change_attribute value
> + * @stat: inode attributes
> + *
> + * Caller must fill in @stat before calling, typically by invoking
> + * vfs_getattr() with STATX_MODE, STATX_CTIME, and STATX_CHANGE_COOKIE.
> + * Returns an unsigned 64-bit changeid4 value (RFC 8881 Section 3.2).
> + *
>   * We could use i_version alone as the change attribute.  However, i_version
>   * can go backwards on a regular file after an unclean shutdown.  On its own
>   * that doesn't necessarily cause a problem, but if i_version goes backwards
> @@ -821,13 +825,13 @@ enum fsid_source fsid_source(const struct svc_fh *fhp)
>   * assume that the new change attr is always logged to stable storage in some
>   * fashion before the results can be seen.
>   */
> -u64 nfsd4_change_attribute(const struct kstat *stat, const struct inode *inode)
> +u64 nfsd4_change_attribute(const struct kstat *stat)
>  {
>  	u64 chattr;
>  
>  	if (stat->result_mask & STATX_CHANGE_COOKIE) {
>  		chattr = stat->change_cookie;
> -		if (S_ISREG(inode->i_mode) &&
> +		if (S_ISREG(stat->mode) &&
>  		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
>  			chattr += (u64)stat->ctime.tv_sec << 30;
>  			chattr += stat->ctime.tv_nsec;
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 5b7394801dc4270dbd5236f3e2f2237130c73dad..876152a91f122f83fb94ffdfb0eedf8fca56a20c 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -297,8 +297,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
>  	fhp->fh_pre_saved = false;
>  }
>  
> -u64 nfsd4_change_attribute(const struct kstat *stat,
> -			   const struct inode *inode);
> +u64 nfsd4_change_attribute(const struct kstat *stat);
>  __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp);
>  __be32 fh_fill_post_attrs(struct svc_fh *fhp);
>  __be32 __must_check fh_fill_both_attrs(struct svc_fh *fhp);
> 
> ---
> base-commit: 2d5404caa8c7bb5c4e0435f94b28834ae5456623
> change-id: 20241111-master-cb9afaab4ca0
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

-- 
Chuck Lever

On Mon, 2024-11-11 at 11:26 -0500, Chuck Lever wrote:
> On Mon, Nov 11, 2024 at 11:01:13AM -0500, Jeff Layton wrote:
> > The inode that nfs4_open_delegation() passes to this function is
> > wrong, which throws off the result. The inode will end up getting a
> > directory-style change attr instead of a regular-file-style one.
> > 
> > Fix up nfs4_delegation_stat() to fetch STATX_MODE, and then drop the
> > inode parameter from nfsd4_change_attribute(), since it's no longer
> > needed.
> > 
> > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > This version should apply cleanly to v6.12-rc7. Some later patches in
> > nfsd-next might need to be twiddled as a result, but it should be simple
> > to fix. Also, I fixed up the Fixes: to point to the right commit. This
> > dates back a bit further than I had originally thought.
> 
> Ah. The Fixes: change makes this interesting. If we're not
> addressing a fix that is limited to in 12-rc, then I lean more
> towards getting this in via a normal merge window.
> 

Ok.

> Also, it's pretty late in the -rc window to take fixes that are more
> than a few lines. I would like to see changes like this get some
> time in linux-next etc etc (which it has already done for v6.13, but
> would be difficult to guarantee for v6.12-rc at this point).
> 
> So then that changes my concern about this to only reducing the
> number of pre-requisite patches that will need to be backported to
> cleanly apply this fix to LTS kernels.
> 
> So about this:
> 
>  - Apply this version of the patch to nfsd-next, but earlier in the
>    series
> 
>  - Fix up the later patches, as you mentioned above
> 
>  - Then let automation grab it for LTS 6.11 and 6.12
> 
> Does that sound over-caffeinated, or would you be OK if I reordered
> nfsd-next as described here?
> 

That sounds fine to me.

-- 
Jeff Layton <jlayton@kernel.org>