[PATCH v5 00/11] nfsd: assorted cleanups involving v4 special stateids.

[PATCH v5 00/11] nfsd: assorted cleanups involving v4 special stateids.

[NeilBrown]

This series started out as an attempt to simplify the management of the "current stateid" which is associated with the current filehandle.
This end up including fixes for foreign filehandle handling as well,
including the first patch which fixes a possibly NULL pointer dereference.

Other than that first patch, this is mostly cleanups with a few minor bug fixes.

Thanks,
NeilBrown

[PATCH v5 01/11] nfsd: rename ALLOWED_WITHOUT_FH to ALLOWED_WITHOUT_LOCAL_FH and revise use

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsdv4 ops which do not have ALLOWED_WITHOUT_FH can assume that a PUTFH
has been called and may assume that current_fh->fh_dentry is non-NULL.
nfsd4_setattr(), for example, assumes fh_dentry != NULL.

However the possibility of foreign filehandles (needed for v4.2 COPY)
means that there maybe a filehandle present while fh_dentry is NULL.

Sending a COMPOUND containing:
   SEQUENCE
   PUTFH - foreign filehandle
   SETATTR - new mode
   SAVEFH
   COPY - with non-empty server list

to an NFS server with inter-server copy enabled will cause a NULL
pointer dereference when nfsd4_setattr() calls fh_want_write().

Most NFSv4 ops actually want a "local" filehandle, not just any
filehandle.  So this patch renames ALLOWED_WITHOUT_FH to
ALLOWED_WITHOUT_LOCAL_FH and sets it on those which don't require a local
filehandle.  That is all that don't require any filehandle together with
SAVEFH, which is the only OP which needs to handle a foreign current_fh.
(COPY must handle a foreign save_fh, but all ops which access save_fh
already do any required validity tests themselves).

nfsd4_savefh() is changed to validate the filehandle itself as the
caller no longer validates it.

nfsd4_proc_compound no longer allows ops without
ALLOWED_WITHOUT_LOCAL_FH to be called with a foreign fh - current_fh
must be local and ->fh_dentry must be non-NULL.  This protects
nfsd4_setattr() and any others that might use ->fh_dentry without
checking.

The
       current_fh->fh_export &&
test is removed from an "else if" because that condition is now only
tested when current_fh->fh_dentry is not NULL, and in that case
current_fh->fh_export is also guaranteed to not be NULL.

Fixes: b9e8638e3d9e ("NFSD: allow inter server COPY to have a STALE source server fh")
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 58 ++++++++++++++++++++++++++--------------------
 fs/nfsd/xdr4.h     |  2 +-
 2 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index dcad50846a97..e5871e861dce 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -729,6 +729,15 @@ static __be32
 nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	     union nfsd4_op_u *u)
 {
+	/*
+	 * SAVEFH is "ALLOWED_WITHOUT_LOCAL_FH" in that current_fh.fh_dentry
+	 * is not required, but fh_handle *is*.  Thus a foreign fh
+	 * can be saved as needed for inter-server COPY.
+	 */
+	if (!current_fh->fh_dentry &&
+	    !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN))
+		return nfserr_nofilehandle;
+
 	fh_dup2(&cstate->save_fh, &cstate->current_fh);
 	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
 		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
@@ -2919,14 +2928,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 				op->status = nfsd4_open_omfg(rqstp, cstate, op);
 			goto encode_op;
 		}
-		if (!current_fh->fh_dentry &&
-				!HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN)) {
-			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_FH)) {
+		if (!current_fh->fh_dentry) {
+			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_LOCAL_FH)) {
 				op->status = nfserr_nofilehandle;
 				goto encode_op;
 			}
-		} else if (current_fh->fh_export &&
-			   current_fh->fh_export->ex_fslocs.migrated &&
+		} else if (current_fh->fh_export->ex_fslocs.migrated &&
 			  !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
 			op->status = nfserr_moved;
 			goto encode_op;
@@ -3507,21 +3514,21 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_PUTFH] = {
 		.op_func = nfsd4_putfh,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
 		.op_name = "OP_PUTFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_PUTPUBFH] = {
 		.op_func = nfsd4_putrootfh,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
 		.op_name = "OP_PUTPUBFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_PUTROOTFH] = {
 		.op_func = nfsd4_putrootfh,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
 		.op_name = "OP_PUTROOTFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3557,7 +3564,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_RENEW] = {
 		.op_func = nfsd4_renew,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_RENEW",
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3565,14 +3572,15 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_RESTOREFH] = {
 		.op_func = nfsd4_restorefh,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_IS_PUTFH_LIKE | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_RESTOREFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_SAVEFH] = {
 		.op_func = nfsd4_savefh,
-		.op_flags = OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH
+				| OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_SAVEFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
@@ -3593,7 +3601,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_SETCLIENTID] = {
 		.op_func = nfsd4_setclientid,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_MODIFIES_SOMETHING | OP_CACHEME
 				| OP_NONTRIVIAL_ERROR_ENCODE,
 		.op_name = "OP_SETCLIENTID",
@@ -3601,7 +3609,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_SETCLIENTID_CONFIRM] = {
 		.op_func = nfsd4_setclientid_confirm,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_MODIFIES_SOMETHING | OP_CACHEME,
 		.op_name = "OP_SETCLIENTID_CONFIRM",
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3620,7 +3628,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_RELEASE_LOCKOWNER] = {
 		.op_func = nfsd4_release_lockowner,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_RELEASE_LOCKOWNER",
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3630,54 +3638,54 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	[OP_EXCHANGE_ID] = {
 		.op_func = nfsd4_exchange_id,
 		.op_release = nfsd4_exchange_id_release,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_EXCHANGE_ID",
 		.op_rsize_bop = nfsd4_exchange_id_rsize,
 	},
 	[OP_BACKCHANNEL_CTL] = {
 		.op_func = nfsd4_backchannel_ctl,
-		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_BACKCHANNEL_CTL",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_BIND_CONN_TO_SESSION] = {
 		.op_func = nfsd4_bind_conn_to_session,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_BIND_CONN_TO_SESSION",
 		.op_rsize_bop = nfsd4_bind_conn_to_session_rsize,
 	},
 	[OP_CREATE_SESSION] = {
 		.op_func = nfsd4_create_session,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_CREATE_SESSION",
 		.op_rsize_bop = nfsd4_create_session_rsize,
 	},
 	[OP_DESTROY_SESSION] = {
 		.op_func = nfsd4_destroy_session,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_DESTROY_SESSION",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_SEQUENCE] = {
 		.op_func = nfsd4_sequence,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP,
 		.op_name = "OP_SEQUENCE",
 		.op_rsize_bop = nfsd4_sequence_rsize,
 	},
 	[OP_DESTROY_CLIENTID] = {
 		.op_func = nfsd4_destroy_clientid,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_DESTROY_CLIENTID",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_RECLAIM_COMPLETE] = {
 		.op_func = nfsd4_reclaim_complete,
-		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_RECLAIM_COMPLETE",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
@@ -3690,13 +3698,13 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_TEST_STATEID] = {
 		.op_func = nfsd4_test_stateid,
-		.op_flags = ALLOWED_WITHOUT_FH,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH,
 		.op_name = "OP_TEST_STATEID",
 		.op_rsize_bop = nfsd4_test_stateid_rsize,
 	},
 	[OP_FREE_STATEID] = {
 		.op_func = nfsd4_free_stateid,
-		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_FREE_STATEID",
 		.op_get_currentstateid = nfsd4_get_freestateid,
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3711,7 +3719,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	[OP_GETDEVICEINFO] = {
 		.op_func = nfsd4_getdeviceinfo,
 		.op_release = nfsd4_getdeviceinfo_release,
-		.op_flags = ALLOWED_WITHOUT_FH,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH,
 		.op_name = "OP_GETDEVICEINFO",
 		.op_rsize_bop = nfsd4_getdeviceinfo_rsize,
 	},
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index ae75846b3cd7..1f0967236cc2 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -1006,7 +1006,7 @@ extern __be32 nfsd4_free_stateid(struct svc_rqst *rqstp,
 extern void nfsd4_bump_seqid(struct nfsd4_compound_state *, __be32 nfserr);
 
 enum nfsd4_op_flags {
-	ALLOWED_WITHOUT_FH = 1 << 0,    /* No current filehandle required */
+	ALLOWED_WITHOUT_LOCAL_FH = 1 << 0,    /* No current filehandle fh_dentry required */
 	ALLOWED_ON_ABSENT_FS = 1 << 1,  /* ops processed on absent fs */
 	ALLOWED_AS_FIRST_OP = 1 << 2,   /* ops reqired first in compound */
 	/* For rfc 5661 section 2.6.3.1.1: */
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v5 01/11] nfsd: rename ALLOWED_WITHOUT_FH to ALLOWED_WITHOUT_LOCAL_FH and revise use

[Chuck Lever]

On 11/18/25 10:28 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsdv4 ops which do not have ALLOWED_WITHOUT_FH can assume that a PUTFH
> has been called and may assume that current_fh->fh_dentry is non-NULL.
> nfsd4_setattr(), for example, assumes fh_dentry != NULL.
> 
> However the possibility of foreign filehandles (needed for v4.2 COPY)
> means that there maybe a filehandle present while fh_dentry is NULL.
> 
> Sending a COMPOUND containing:
>    SEQUENCE
>    PUTFH - foreign filehandle
>    SETATTR - new mode
>    SAVEFH
>    COPY - with non-empty server list
> 
> to an NFS server with inter-server copy enabled will cause a NULL
> pointer dereference when nfsd4_setattr() calls fh_want_write().
> 
> Most NFSv4 ops actually want a "local" filehandle, not just any
> filehandle.  So this patch renames ALLOWED_WITHOUT_FH to
> ALLOWED_WITHOUT_LOCAL_FH and sets it on those which don't require a local
> filehandle.  That is all that don't require any filehandle together with
> SAVEFH, which is the only OP which needs to handle a foreign current_fh.
> (COPY must handle a foreign save_fh, but all ops which access save_fh
> already do any required validity tests themselves).
> 
> nfsd4_savefh() is changed to validate the filehandle itself as the
> caller no longer validates it.
> 
> nfsd4_proc_compound no longer allows ops without
> ALLOWED_WITHOUT_LOCAL_FH to be called with a foreign fh - current_fh
> must be local and ->fh_dentry must be non-NULL.  This protects
> nfsd4_setattr() and any others that might use ->fh_dentry without
> checking.
> 
> The
>        current_fh->fh_export &&
> test is removed from an "else if" because that condition is now only
> tested when current_fh->fh_dentry is not NULL, and in that case
> current_fh->fh_export is also guaranteed to not be NULL.
> 
> Fixes: b9e8638e3d9e ("NFSD: allow inter server COPY to have a STALE source server fh")

Shall we mark this one with Cc: stable?


> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 58 ++++++++++++++++++++++++++--------------------
>  fs/nfsd/xdr4.h     |  2 +-
>  2 files changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index dcad50846a97..e5871e861dce 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -729,6 +729,15 @@ static __be32
>  nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	     union nfsd4_op_u *u)
>  {
> +	/*
> +	 * SAVEFH is "ALLOWED_WITHOUT_LOCAL_FH" in that current_fh.fh_dentry
> +	 * is not required, but fh_handle *is*.  Thus a foreign fh
> +	 * can be saved as needed for inter-server COPY.
> +	 */
> +	if (!current_fh->fh_dentry &&
> +	    !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN))
> +		return nfserr_nofilehandle;
> +
>  	fh_dup2(&cstate->save_fh, &cstate->current_fh);
>  	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
>  		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));

  CC [M]  fs/nfsd/nfs4proc.o
/home/cel/src/linux/for-korg/fs/nfsd/nfs4proc.c: In function ‘nfsd4_savefh’:
/home/cel/src/linux/for-korg/fs/nfsd/nfs4proc.c:737:14: error:
‘current_fh’ undeclared (first use in this function); did you mean
‘current_uid’?
  737 |         if (!current_fh->fh_dentry &&
      |              ^~~~~~~~~~
      |              current_uid

Perhaps we want:

-	if (!current_fh->fh_dentry &&
-	    !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN))
+	if (!cstate->current_fh.fh_dentry &&
+	    !HAS_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN))
 		return nfserr_nofilehandle;


> @@ -2919,14 +2928,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  				op->status = nfsd4_open_omfg(rqstp, cstate, op);
>  			goto encode_op;
>  		}
> -		if (!current_fh->fh_dentry &&
> -				!HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN)) {
> -			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_FH)) {
> +		if (!current_fh->fh_dentry) {
> +			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_LOCAL_FH)) {
>  				op->status = nfserr_nofilehandle;
>  				goto encode_op;
>  			}
> -		} else if (current_fh->fh_export &&
> -			   current_fh->fh_export->ex_fslocs.migrated &&
> +		} else if (current_fh->fh_export->ex_fslocs.migrated &&
>  			  !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
>  			op->status = nfserr_moved;
>  			goto encode_op;
> @@ -3507,21 +3514,21 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_PUTFH] = {
>  		.op_func = nfsd4_putfh,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
>  		.op_name = "OP_PUTFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_PUTPUBFH] = {
>  		.op_func = nfsd4_putrootfh,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
>  		.op_name = "OP_PUTPUBFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_PUTROOTFH] = {
>  		.op_func = nfsd4_putrootfh,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
>  		.op_name = "OP_PUTROOTFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3557,7 +3564,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_RENEW] = {
>  		.op_func = nfsd4_renew,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_RENEW",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3565,14 +3572,15 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_RESTOREFH] = {
>  		.op_func = nfsd4_restorefh,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_IS_PUTFH_LIKE | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_RESTOREFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_SAVEFH] = {
>  		.op_func = nfsd4_savefh,
> -		.op_flags = OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH
> +				| OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_SAVEFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
> @@ -3593,7 +3601,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_SETCLIENTID] = {
>  		.op_func = nfsd4_setclientid,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_MODIFIES_SOMETHING | OP_CACHEME
>  				| OP_NONTRIVIAL_ERROR_ENCODE,
>  		.op_name = "OP_SETCLIENTID",
> @@ -3601,7 +3609,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_SETCLIENTID_CONFIRM] = {
>  		.op_func = nfsd4_setclientid_confirm,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_MODIFIES_SOMETHING | OP_CACHEME,
>  		.op_name = "OP_SETCLIENTID_CONFIRM",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3620,7 +3628,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_RELEASE_LOCKOWNER] = {
>  		.op_func = nfsd4_release_lockowner,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_RELEASE_LOCKOWNER",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3630,54 +3638,54 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	[OP_EXCHANGE_ID] = {
>  		.op_func = nfsd4_exchange_id,
>  		.op_release = nfsd4_exchange_id_release,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_EXCHANGE_ID",
>  		.op_rsize_bop = nfsd4_exchange_id_rsize,
>  	},
>  	[OP_BACKCHANNEL_CTL] = {
>  		.op_func = nfsd4_backchannel_ctl,
> -		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_BACKCHANNEL_CTL",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_BIND_CONN_TO_SESSION] = {
>  		.op_func = nfsd4_bind_conn_to_session,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_BIND_CONN_TO_SESSION",
>  		.op_rsize_bop = nfsd4_bind_conn_to_session_rsize,
>  	},
>  	[OP_CREATE_SESSION] = {
>  		.op_func = nfsd4_create_session,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_CREATE_SESSION",
>  		.op_rsize_bop = nfsd4_create_session_rsize,
>  	},
>  	[OP_DESTROY_SESSION] = {
>  		.op_func = nfsd4_destroy_session,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_DESTROY_SESSION",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_SEQUENCE] = {
>  		.op_func = nfsd4_sequence,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP,
>  		.op_name = "OP_SEQUENCE",
>  		.op_rsize_bop = nfsd4_sequence_rsize,
>  	},
>  	[OP_DESTROY_CLIENTID] = {
>  		.op_func = nfsd4_destroy_clientid,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_DESTROY_CLIENTID",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_RECLAIM_COMPLETE] = {
>  		.op_func = nfsd4_reclaim_complete,
> -		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_RECLAIM_COMPLETE",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
> @@ -3690,13 +3698,13 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_TEST_STATEID] = {
>  		.op_func = nfsd4_test_stateid,
> -		.op_flags = ALLOWED_WITHOUT_FH,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH,
>  		.op_name = "OP_TEST_STATEID",
>  		.op_rsize_bop = nfsd4_test_stateid_rsize,
>  	},
>  	[OP_FREE_STATEID] = {
>  		.op_func = nfsd4_free_stateid,
> -		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_FREE_STATEID",
>  		.op_get_currentstateid = nfsd4_get_freestateid,
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3711,7 +3719,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	[OP_GETDEVICEINFO] = {
>  		.op_func = nfsd4_getdeviceinfo,
>  		.op_release = nfsd4_getdeviceinfo_release,
> -		.op_flags = ALLOWED_WITHOUT_FH,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH,
>  		.op_name = "OP_GETDEVICEINFO",
>  		.op_rsize_bop = nfsd4_getdeviceinfo_rsize,
>  	},
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index ae75846b3cd7..1f0967236cc2 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -1006,7 +1006,7 @@ extern __be32 nfsd4_free_stateid(struct svc_rqst *rqstp,
>  extern void nfsd4_bump_seqid(struct nfsd4_compound_state *, __be32 nfserr);
>  
>  enum nfsd4_op_flags {
> -	ALLOWED_WITHOUT_FH = 1 << 0,    /* No current filehandle required */
> +	ALLOWED_WITHOUT_LOCAL_FH = 1 << 0,    /* No current filehandle fh_dentry required */

The new comment is unclear to me. Is there missing punctuation?


>  	ALLOWED_ON_ABSENT_FS = 1 << 1,  /* ops processed on absent fs */
>  	ALLOWED_AS_FIRST_OP = 1 << 2,   /* ops reqired first in compound */
>  	/* For rfc 5661 section 2.6.3.1.1: */


-- 
Chuck Lever

Re: [PATCH v5 01/11] nfsd: rename ALLOWED_WITHOUT_FH to ALLOWED_WITHOUT_LOCAL_FH and revise use

[NeilBrown]

On Thu, 20 Nov 2025, Chuck Lever wrote:
> On 11/18/25 10:28 PM, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > nfsdv4 ops which do not have ALLOWED_WITHOUT_FH can assume that a PUTFH
> > has been called and may assume that current_fh->fh_dentry is non-NULL.
> > nfsd4_setattr(), for example, assumes fh_dentry != NULL.
> > 
> > However the possibility of foreign filehandles (needed for v4.2 COPY)
> > means that there maybe a filehandle present while fh_dentry is NULL.
> > 
> > Sending a COMPOUND containing:
> >    SEQUENCE
> >    PUTFH - foreign filehandle
> >    SETATTR - new mode
> >    SAVEFH
> >    COPY - with non-empty server list
> > 
> > to an NFS server with inter-server copy enabled will cause a NULL
> > pointer dereference when nfsd4_setattr() calls fh_want_write().
> > 
> > Most NFSv4 ops actually want a "local" filehandle, not just any
> > filehandle.  So this patch renames ALLOWED_WITHOUT_FH to
> > ALLOWED_WITHOUT_LOCAL_FH and sets it on those which don't require a local
> > filehandle.  That is all that don't require any filehandle together with
> > SAVEFH, which is the only OP which needs to handle a foreign current_fh.
> > (COPY must handle a foreign save_fh, but all ops which access save_fh
> > already do any required validity tests themselves).
> > 
> > nfsd4_savefh() is changed to validate the filehandle itself as the
> > caller no longer validates it.
> > 
> > nfsd4_proc_compound no longer allows ops without
> > ALLOWED_WITHOUT_LOCAL_FH to be called with a foreign fh - current_fh
> > must be local and ->fh_dentry must be non-NULL.  This protects
> > nfsd4_setattr() and any others that might use ->fh_dentry without
> > checking.
> > 
> > The
> >        current_fh->fh_export &&
> > test is removed from an "else if" because that condition is now only
> > tested when current_fh->fh_dentry is not NULL, and in that case
> > current_fh->fh_export is also guaranteed to not be NULL.
> > 
> > Fixes: b9e8638e3d9e ("NFSD: allow inter server COPY to have a STALE source server fh")
> 
> Shall we mark this one with Cc: stable?

That would be appropriate I think.


> 
> 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/nfsd/nfs4proc.c | 58 ++++++++++++++++++++++++++--------------------
> >  fs/nfsd/xdr4.h     |  2 +-
> >  2 files changed, 34 insertions(+), 26 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index dcad50846a97..e5871e861dce 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -729,6 +729,15 @@ static __be32
> >  nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	     union nfsd4_op_u *u)
> >  {
> > +	/*
> > +	 * SAVEFH is "ALLOWED_WITHOUT_LOCAL_FH" in that current_fh.fh_dentry
> > +	 * is not required, but fh_handle *is*.  Thus a foreign fh
> > +	 * can be saved as needed for inter-server COPY.
> > +	 */
> > +	if (!current_fh->fh_dentry &&
> > +	    !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN))
> > +		return nfserr_nofilehandle;
> > +
> >  	fh_dup2(&cstate->save_fh, &cstate->current_fh);
> >  	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
> >  		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
> 
>   CC [M]  fs/nfsd/nfs4proc.o
> /home/cel/src/linux/for-korg/fs/nfsd/nfs4proc.c: In function ‘nfsd4_savefh’:
> /home/cel/src/linux/for-korg/fs/nfsd/nfs4proc.c:737:14: error:
> ‘current_fh’ undeclared (first use in this function); did you mean
> ‘current_uid’?
>   737 |         if (!current_fh->fh_dentry &&
>       |              ^~~~~~~~~~
>       |              current_uid

Both.  I did lots of test compiles, but obviously missed this point
in the series.

> 
> Perhaps we want:
> 
> -	if (!current_fh->fh_dentry &&
> -	    !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN))
> +	if (!cstate->current_fh.fh_dentry &&
> +	    !HAS_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN))
>  		return nfserr_nofilehandle;

yep.

> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -1006,7 +1006,7 @@ extern __be32 nfsd4_free_stateid(struct svc_rqst *rqstp,
> >  extern void nfsd4_bump_seqid(struct nfsd4_compound_state *, __be32 nfserr);
> >  
> >  enum nfsd4_op_flags {
> > -	ALLOWED_WITHOUT_FH = 1 << 0,    /* No current filehandle required */
> > +	ALLOWED_WITHOUT_LOCAL_FH = 1 << 0,    /* No current filehandle fh_dentry required */
> 
> The new comment is unclear to me. Is there missing punctuation?
> 

+	ALLOWED_WITHOUT_LOCAL_FH = 1 << 0,    /* No "current filehandle fh_dentry" required */
??

I've made it:
+	ALLOWED_WITHOUT_LOCAL_FH = 1 << 0,    /* No fh_dentry needed in current filehandle */

Thanks,
NeilBrown


> 
> >  	ALLOWED_ON_ABSENT_FS = 1 << 1,  /* ops processed on absent fs */
> >  	ALLOWED_AS_FIRST_OP = 1 << 2,   /* ops reqired first in compound */
> >  	/* For rfc 5661 section 2.6.3.1.1: */
> 
> 
> -- 
> Chuck Lever
> 

Re: [PATCH v5 01/11] nfsd: rename ALLOWED_WITHOUT_FH to ALLOWED_WITHOUT_LOCAL_FH and revise use

[Jeff Layton]

On Wed, 2025-11-19 at 14:28 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsdv4 ops which do not have ALLOWED_WITHOUT_FH can assume that a PUTFH
> has been called and may assume that current_fh->fh_dentry is non-NULL.
> nfsd4_setattr(), for example, assumes fh_dentry != NULL.
> 
> However the possibility of foreign filehandles (needed for v4.2 COPY)
> means that there maybe a filehandle present while fh_dentry is NULL.
> 
> Sending a COMPOUND containing:
>    SEQUENCE
>    PUTFH - foreign filehandle
>    SETATTR - new mode
>    SAVEFH
>    COPY - with non-empty server list
> 
> to an NFS server with inter-server copy enabled will cause a NULL
> pointer dereference when nfsd4_setattr() calls fh_want_write().
> 
> Most NFSv4 ops actually want a "local" filehandle, not just any
> filehandle.  So this patch renames ALLOWED_WITHOUT_FH to
> ALLOWED_WITHOUT_LOCAL_FH and sets it on those which don't require a local
> filehandle.  That is all that don't require any filehandle together with
> SAVEFH, which is the only OP which needs to handle a foreign current_fh.
> (COPY must handle a foreign save_fh, but all ops which access save_fh
> already do any required validity tests themselves).
> 
> nfsd4_savefh() is changed to validate the filehandle itself as the
> caller no longer validates it.
> 
> nfsd4_proc_compound no longer allows ops without
> ALLOWED_WITHOUT_LOCAL_FH to be called with a foreign fh - current_fh
> must be local and ->fh_dentry must be non-NULL.  This protects
> nfsd4_setattr() and any others that might use ->fh_dentry without
> checking.
> 
> The
>        current_fh->fh_export &&
> test is removed from an "else if" because that condition is now only
> tested when current_fh->fh_dentry is not NULL, and in that case
> current_fh->fh_export is also guaranteed to not be NULL.
> 
> Fixes: b9e8638e3d9e ("NFSD: allow inter server COPY to have a STALE source server fh")
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 58 ++++++++++++++++++++++++++--------------------
>  fs/nfsd/xdr4.h     |  2 +-
>  2 files changed, 34 insertions(+), 26 deletions(-)
> 

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

[PATCH v5 02/11] nfsd: discard NFSD4_FH_FOREIGN

[NeilBrown]

From: NeilBrown <neil@brown.name>

NFSD4_FH_FOREIGN is not needed as the same information is elsewhere.

If fh_handle.fh_len is 0 then there is no filehandle
else if fh_dentry is NULL then the filehandle is foreign
else the filehandle is local.

So we can discard NFSD4_FH_FOREIGN and the related struct field,
and code.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 7 ++-----
 fs/nfsd/nfsfh.h    | 4 ----
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index e5871e861dce..3160e899a5da 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -693,10 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	       putfh->pf_fhlen);
 	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
 #ifdef CONFIG_NFSD_V4_2_INTER_SSC
-	if (ret == nfserr_stale && putfh->no_verify) {
-		SET_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN);
+	if (ret == nfserr_stale && putfh->no_verify)
 		ret = 0;
-	}
 #endif
 	return ret;
 }
@@ -734,8 +732,7 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * is not required, but fh_handle *is*.  Thus a foreign fh
 	 * can be saved as needed for inter-server COPY.
 	 */
-	if (!current_fh->fh_dentry &&
-	    !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN))
+	if (cstate->current_fh.fh_handle.fh_size == 0)
 		return nfserr_nofilehandle;
 
 	fh_dup2(&cstate->save_fh, &cstate->current_fh);
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 5ef7191f8ad8..43fcc1dcf69a 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -93,7 +93,6 @@ typedef struct svc_fh {
 						 */
 	bool			fh_use_wgather;	/* NFSv2 wgather option */
 	bool			fh_64bit_cookies;/* readdir cookie size */
-	int			fh_flags;	/* FH flags */
 	bool			fh_post_saved;	/* post-op attrs saved */
 	bool			fh_pre_saved;	/* pre-op attrs saved */
 
@@ -111,9 +110,6 @@ typedef struct svc_fh {
 	struct kstat		fh_post_attr;	/* full attrs after operation */
 	u64			fh_post_change; /* nfsv4 change; see above */
 } svc_fh;
-#define NFSD4_FH_FOREIGN (1<<0)
-#define SET_FH_FLAG(c, f) ((c)->fh_flags |= (f))
-#define HAS_FH_FLAG(c, f) ((c)->fh_flags & (f))
 
 enum nfsd_fsid {
 	FSID_DEV = 0,
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v5 02/11] nfsd: discard NFSD4_FH_FOREIGN

[Chuck Lever]

On 11/18/25 10:28 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> NFSD4_FH_FOREIGN is not needed as the same information is elsewhere.
> 
> If fh_handle.fh_len is 0 then there is no filehandle

s/fh_len/fh_size

I think to make this new mechanism bullet-proof, maybe fh_put() needs to
set fh_handle.fh_size to zero?

An NFS client can do something crazy like { SECINFO, SAVEFH }. The new
"fh_size == 0" check would pass because fh_put doesn't clear fh_size,
and then NFSD would proceed to save this no-longer-valid handle. The
current check catches this by seeing !fh_dentry without FOREIGN flag
set.


> else if fh_dentry is NULL then the filehandle is foreign
> else the filehandle is local.
> 
> So we can discard NFSD4_FH_FOREIGN and the related struct field,
> and code.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 7 ++-----
>  fs/nfsd/nfsfh.h    | 4 ----
>  2 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e5871e861dce..3160e899a5da 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -693,10 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	       putfh->pf_fhlen);
>  	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
>  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> -	if (ret == nfserr_stale && putfh->no_verify) {
> -		SET_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN);
> +	if (ret == nfserr_stale && putfh->no_verify)
>  		ret = 0;
> -	}
>  #endif
>  	return ret;
>  }
> @@ -734,8 +732,7 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	 * is not required, but fh_handle *is*.  Thus a foreign fh
>  	 * can be saved as needed for inter-server COPY.
>  	 */
> -	if (!current_fh->fh_dentry &&
> -	    !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN))
> +	if (cstate->current_fh.fh_handle.fh_size == 0)
>  		return nfserr_nofilehandle;
>  
>  	fh_dup2(&cstate->save_fh, &cstate->current_fh);
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 5ef7191f8ad8..43fcc1dcf69a 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -93,7 +93,6 @@ typedef struct svc_fh {
>  						 */
>  	bool			fh_use_wgather;	/* NFSv2 wgather option */
>  	bool			fh_64bit_cookies;/* readdir cookie size */
> -	int			fh_flags;	/* FH flags */
>  	bool			fh_post_saved;	/* post-op attrs saved */
>  	bool			fh_pre_saved;	/* pre-op attrs saved */
>  
> @@ -111,9 +110,6 @@ typedef struct svc_fh {
>  	struct kstat		fh_post_attr;	/* full attrs after operation */
>  	u64			fh_post_change; /* nfsv4 change; see above */
>  } svc_fh;
> -#define NFSD4_FH_FOREIGN (1<<0)
> -#define SET_FH_FLAG(c, f) ((c)->fh_flags |= (f))
> -#define HAS_FH_FLAG(c, f) ((c)->fh_flags & (f))
>  
>  enum nfsd_fsid {
>  	FSID_DEV = 0,


-- 
Chuck Lever

Re: [PATCH v5 02/11] nfsd: discard NFSD4_FH_FOREIGN

[NeilBrown]

On Thu, 20 Nov 2025, Chuck Lever wrote:
> On 11/18/25 10:28 PM, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > NFSD4_FH_FOREIGN is not needed as the same information is elsewhere.
> > 
> > If fh_handle.fh_len is 0 then there is no filehandle
> 
> s/fh_len/fh_size
> 
> I think to make this new mechanism bullet-proof, maybe fh_put() needs to
> set fh_handle.fh_size to zero?
> 
> An NFS client can do something crazy like { SECINFO, SAVEFH }. The new
> "fh_size == 0" check would pass because fh_put doesn't clear fh_size,
> and then NFSD would proceed to save this no-longer-valid handle. The
> current check catches this by seeing !fh_dentry without FOREIGN flag
> set.

I didn't know that about SECINFO.  I agree that clearing fh_size in
fh_put() is needed.

Thanks,
NeilBrown


> 
> 
> > else if fh_dentry is NULL then the filehandle is foreign
> > else the filehandle is local.
> > 
> > So we can discard NFSD4_FH_FOREIGN and the related struct field,
> > and code.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/nfsd/nfs4proc.c | 7 ++-----
> >  fs/nfsd/nfsfh.h    | 4 ----
> >  2 files changed, 2 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index e5871e861dce..3160e899a5da 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -693,10 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	       putfh->pf_fhlen);
> >  	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
> >  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > -	if (ret == nfserr_stale && putfh->no_verify) {
> > -		SET_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN);
> > +	if (ret == nfserr_stale && putfh->no_verify)
> >  		ret = 0;
> > -	}
> >  #endif
> >  	return ret;
> >  }
> > @@ -734,8 +732,7 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	 * is not required, but fh_handle *is*.  Thus a foreign fh
> >  	 * can be saved as needed for inter-server COPY.
> >  	 */
> > -	if (!current_fh->fh_dentry &&
> > -	    !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN))
> > +	if (cstate->current_fh.fh_handle.fh_size == 0)
> >  		return nfserr_nofilehandle;
> >  
> >  	fh_dup2(&cstate->save_fh, &cstate->current_fh);
> > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> > index 5ef7191f8ad8..43fcc1dcf69a 100644
> > --- a/fs/nfsd/nfsfh.h
> > +++ b/fs/nfsd/nfsfh.h
> > @@ -93,7 +93,6 @@ typedef struct svc_fh {
> >  						 */
> >  	bool			fh_use_wgather;	/* NFSv2 wgather option */
> >  	bool			fh_64bit_cookies;/* readdir cookie size */
> > -	int			fh_flags;	/* FH flags */
> >  	bool			fh_post_saved;	/* post-op attrs saved */
> >  	bool			fh_pre_saved;	/* pre-op attrs saved */
> >  
> > @@ -111,9 +110,6 @@ typedef struct svc_fh {
> >  	struct kstat		fh_post_attr;	/* full attrs after operation */
> >  	u64			fh_post_change; /* nfsv4 change; see above */
> >  } svc_fh;
> > -#define NFSD4_FH_FOREIGN (1<<0)
> > -#define SET_FH_FLAG(c, f) ((c)->fh_flags |= (f))
> > -#define HAS_FH_FLAG(c, f) ((c)->fh_flags & (f))
> >  
> >  enum nfsd_fsid {
> >  	FSID_DEV = 0,
> 
> 
> -- 
> Chuck Lever
> 

Re: [PATCH v5 02/11] nfsd: discard NFSD4_FH_FOREIGN

[Jeff Layton]

On Wed, 2025-11-19 at 14:28 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> NFSD4_FH_FOREIGN is not needed as the same information is elsewhere.
> 
> If fh_handle.fh_len is 0 then there is no filehandle
> else if fh_dentry is NULL then the filehandle is foreign
> else the filehandle is local.
> 
> So we can discard NFSD4_FH_FOREIGN and the related struct field,
> and code.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 7 ++-----
>  fs/nfsd/nfsfh.h    | 4 ----
>  2 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e5871e861dce..3160e899a5da 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -693,10 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	       putfh->pf_fhlen);
>  	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
>  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> -	if (ret == nfserr_stale && putfh->no_verify) {
> -		SET_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN);
> +	if (ret == nfserr_stale && putfh->no_verify)
>  		ret = 0;
> -	}
>  #endif
>  	return ret;
>  }
> @@ -734,8 +732,7 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	 * is not required, but fh_handle *is*.  Thus a foreign fh
>  	 * can be saved as needed for inter-server COPY.
>  	 */
> -	if (!current_fh->fh_dentry &&
> -	    !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN))
> +	if (cstate->current_fh.fh_handle.fh_size == 0)
>  		return nfserr_nofilehandle;
>  
>  	fh_dup2(&cstate->save_fh, &cstate->current_fh);
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 5ef7191f8ad8..43fcc1dcf69a 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -93,7 +93,6 @@ typedef struct svc_fh {
>  						 */
>  	bool			fh_use_wgather;	/* NFSv2 wgather option */
>  	bool			fh_64bit_cookies;/* readdir cookie size */
> -	int			fh_flags;	/* FH flags */
>  	bool			fh_post_saved;	/* post-op attrs saved */
>  	bool			fh_pre_saved;	/* pre-op attrs saved */
>  
> @@ -111,9 +110,6 @@ typedef struct svc_fh {
>  	struct kstat		fh_post_attr;	/* full attrs after operation */
>  	u64			fh_post_change; /* nfsv4 change; see above */
>  } svc_fh;
> -#define NFSD4_FH_FOREIGN (1<<0)
> -#define SET_FH_FLAG(c, f) ((c)->fh_flags |= (f))
> -#define HAS_FH_FLAG(c, f) ((c)->fh_flags & (f))
>  
>  enum nfsd_fsid {
>  	FSID_DEV = 0,

Nice.

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

[PATCH v5 03/11] nfsd: simplify foreign-filehandle handling to better match RFC-7862

[NeilBrown]

From: NeilBrown <neil@brown.name>

When the COPY v4.2 op is used in the inter-server copy mode, the file
handle of the source file (presented as the saved filehandle to COPY)
is for a different ("foreign") file server which would not be expected
to be understood by this server.  fh_verify() might return nfserr_stale
or nfserr_badhandle.

In order of this filehandle to still be available to COPY, both PUTFH
and SAVEFH much not trigger an error.

RFC 7862 section 15.2.3 says:

   If the request is for an inter-server copy, the source-fh is a
   filehandle from the source server and the COMPOUND procedure is being
   executed on the destination server.  In this case, the source-fh is a
   foreign filehandle on the server receiving the COPY request.  If
   either PUTFH or SAVEFH checked the validity of the filehandle, the
   operation would likely fail and return NFS4ERR_STALE.

   If a server supports the inter-server copy feature, a PUTFH followed
   by a SAVEFH MUST NOT return NFS4ERR_STALE for either operation.
   These restrictions do not pose substantial difficulties for servers.
   CURRENT_FH and SAVED_FH may be validated in the context of the
   operation referencing them and an NFS4ERR_STALE error returned for an
   invalid filehandle at that point.

[The RFC neglects the possibility of NFS4ERR_BADHANDLE]

Linux nfsd currently takes a different approach.  Rather than just
checking for "a PUTFH followed by a SAVEFH", it goes further to consider
only that case when this filehandle is then used for COPY, and allows
that it might have been subject of RESTOREFH and SAVEFH in between.

This is not a problem in itself except for the extra code with little
benefit.  This analysis of the COMPOUND to detect PUTFH ops which need
care is performed on every COMPOUND request, which is not necessary.

It is sufficient to check if the relevant conditions are met only when a
PUTFH op actually receives an error from fh_verify().

This patch removes the checking code from common paths and places it in
nfsd4_putfh() only when fh_verify() returns a relevant error.

Rather than scanning ahead for a COPY, this patch notes (in
nfsd4_compoundargs) when an inter-server COPY is decoded, and in that
case only checks the next op to see if it is SAVEFH as this is what the
RFC requires.

A test on "inter_copy_offload_enable" is also added to be completely
consistent with the "If a server supports the inter-server copy feature"
precondition.

As we do this test in nfsd4_putfh() there is no now need to mark the
putfh op as "no_verify".

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 64 ++++++++++++++++------------------------------
 fs/nfsd/nfs4xdr.c  |  2 +-
 fs/nfsd/xdr4.h     |  2 +-
 3 files changed, 24 insertions(+), 44 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 3160e899a5da..e6f8b5b907a9 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -693,8 +693,28 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	       putfh->pf_fhlen);
 	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
 #ifdef CONFIG_NFSD_V4_2_INTER_SSC
-	if (ret == nfserr_stale && putfh->no_verify)
-		ret = 0;
+	if ((ret == nfserr_badhandle || ret == nfserr_stale) &&
+	    inter_copy_offload_enable) {
+		struct nfsd4_compoundargs *args = rqstp->rq_argp;
+		struct nfsd4_compoundres *resp = rqstp->rq_resp;
+		struct nfsd4_op	*next_op = &args->ops[resp->opcnt];
+
+		if (resp->opcnt <= args->opcnt &&
+		    next_op->opnum == OP_SAVEFH &&
+		    args->is_inter_server_copy) {
+			/*
+			 * RFC 7862 section 15.2.3 says:
+			 *  If a server supports the inter-server copy
+			 *  feature, a PUTFH followed by a SAVEFH MUST
+			 *  NOT return NFS4ERR_STALE for either
+			 *  operation.
+			 * We limit this to when there is a COPY
+			 * in the COMPOUND, and extend it to
+			 * NFS4ERR_BADHANDLE.
+			 */
+			ret = 0;
+		}
+	}
 #endif
 	return ret;
 }
@@ -2809,45 +2829,6 @@ static bool need_wrongsec_check(struct svc_rqst *rqstp)
 	return !(nextd->op_flags & OP_HANDLES_WRONGSEC);
 }
 
-#ifdef CONFIG_NFSD_V4_2_INTER_SSC
-static void
-check_if_stalefh_allowed(struct nfsd4_compoundargs *args)
-{
-	struct nfsd4_op	*op, *current_op = NULL, *saved_op = NULL;
-	struct nfsd4_copy *copy;
-	struct nfsd4_putfh *putfh;
-	int i;
-
-	/* traverse all operation and if it's a COPY compound, mark the
-	 * source filehandle to skip verification
-	 */
-	for (i = 0; i < args->opcnt; i++) {
-		op = &args->ops[i];
-		if (op->opnum == OP_PUTFH)
-			current_op = op;
-		else if (op->opnum == OP_SAVEFH)
-			saved_op = current_op;
-		else if (op->opnum == OP_RESTOREFH)
-			current_op = saved_op;
-		else if (op->opnum == OP_COPY) {
-			copy = (struct nfsd4_copy *)&op->u;
-			if (!saved_op) {
-				op->status = nfserr_nofilehandle;
-				return;
-			}
-			putfh = (struct nfsd4_putfh *)&saved_op->u;
-			if (nfsd4_ssc_is_inter(copy))
-				putfh->no_verify = true;
-		}
-	}
-}
-#else
-static void
-check_if_stalefh_allowed(struct nfsd4_compoundargs *args)
-{
-}
-#endif
-
 /*
  * COMPOUND call.
  */
@@ -2897,7 +2878,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 		resp->opcnt = 1;
 		goto encode_op;
 	}
-	check_if_stalefh_allowed(args);
 
 	rqstp->rq_lease_breaker = (void **)&cstate->clp;
 
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 51ef97c25456..7e44af3d10b9 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1250,7 +1250,6 @@ nfsd4_decode_putfh(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
 	if (!putfh->pf_fhval)
 		return nfserr_jukebox;
 
-	putfh->no_verify = false;
 	return nfs_ok;
 }
 
@@ -2047,6 +2046,7 @@ nfsd4_decode_copy(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
 	if (status)
 		return status;
 
+	argp->is_inter_server_copy = true;
 	ns_dummy = kmalloc(sizeof(struct nl4_server), GFP_KERNEL);
 	if (ns_dummy == NULL)
 		return nfserr_jukebox;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 1f0967236cc2..3de8f4e07c49 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -335,7 +335,6 @@ struct nfsd4_lookup {
 struct nfsd4_putfh {
 	u32		pf_fhlen;           /* request */
 	char		*pf_fhval;          /* request */
-	bool		no_verify;	    /* represents foreigh fh */
 };
 
 struct nfsd4_getxattr {
@@ -907,6 +906,7 @@ struct nfsd4_compoundargs {
 	u32				client_opcnt;
 	u32				opcnt;
 	bool				splice_ok;
+	bool				is_inter_server_copy;
 	struct nfsd4_op			*ops;
 	struct nfsd4_op			iops[8];
 };
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v5 03/11] nfsd: simplify foreign-filehandle handling to better match RFC-7862

[Chuck Lever]

On 11/18/25 10:28 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> When the COPY v4.2 op is used in the inter-server copy mode, the file
> handle of the source file (presented as the saved filehandle to COPY)
> is for a different ("foreign") file server which would not be expected
> to be understood by this server.  fh_verify() might return nfserr_stale
> or nfserr_badhandle.
> 
> In order of this filehandle to still be available to COPY, both PUTFH
> and SAVEFH much not trigger an error.
> 
> RFC 7862 section 15.2.3 says:
> 
>    If the request is for an inter-server copy, the source-fh is a
>    filehandle from the source server and the COMPOUND procedure is being
>    executed on the destination server.  In this case, the source-fh is a
>    foreign filehandle on the server receiving the COPY request.  If
>    either PUTFH or SAVEFH checked the validity of the filehandle, the
>    operation would likely fail and return NFS4ERR_STALE.
> 
>    If a server supports the inter-server copy feature, a PUTFH followed
>    by a SAVEFH MUST NOT return NFS4ERR_STALE for either operation.
>    These restrictions do not pose substantial difficulties for servers.
>    CURRENT_FH and SAVED_FH may be validated in the context of the
>    operation referencing them and an NFS4ERR_STALE error returned for an
>    invalid filehandle at that point.
> 
> [The RFC neglects the possibility of NFS4ERR_BADHANDLE]
> 
> Linux nfsd currently takes a different approach.  Rather than just
> checking for "a PUTFH followed by a SAVEFH", it goes further to consider
> only that case when this filehandle is then used for COPY, and allows
> that it might have been subject of RESTOREFH and SAVEFH in between.
> 
> This is not a problem in itself except for the extra code with little
> benefit.  This analysis of the COMPOUND to detect PUTFH ops which need
> care is performed on every COMPOUND request, which is not necessary.
> 
> It is sufficient to check if the relevant conditions are met only when a
> PUTFH op actually receives an error from fh_verify().
> 
> This patch removes the checking code from common paths and places it in
> nfsd4_putfh() only when fh_verify() returns a relevant error.
> 
> Rather than scanning ahead for a COPY, this patch notes (in
> nfsd4_compoundargs) when an inter-server COPY is decoded, and in that
> case only checks the next op to see if it is SAVEFH as this is what the
> RFC requires.
> 
> A test on "inter_copy_offload_enable" is also added to be completely
> consistent with the "If a server supports the inter-server copy feature"
> precondition.
> 
> As we do this test in nfsd4_putfh() there is no now need to mark the
> putfh op as "no_verify".
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 64 ++++++++++++++++------------------------------
>  fs/nfsd/nfs4xdr.c  |  2 +-
>  fs/nfsd/xdr4.h     |  2 +-
>  3 files changed, 24 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 3160e899a5da..e6f8b5b907a9 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -693,8 +693,28 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	       putfh->pf_fhlen);
>  	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
>  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> -	if (ret == nfserr_stale && putfh->no_verify)
> -		ret = 0;
> +	if ((ret == nfserr_badhandle || ret == nfserr_stale) &&
> +	    inter_copy_offload_enable) {
> +		struct nfsd4_compoundargs *args = rqstp->rq_argp;
> +		struct nfsd4_compoundres *resp = rqstp->rq_resp;
> +		struct nfsd4_op	*next_op = &args->ops[resp->opcnt];
> +

I find the initializer confusing -- it's only generating an address,
but not yet dereferencing it -- but it can generate an address beyond
the end of the args->ops array.


> +		if (resp->opcnt <= args->opcnt &&

In fact the "resp->opcnt <= args->opcnt" check allows accessing
the N+1th array element, since the array is indexed 0 to N-1. So
the condition here needs to by "<" not "<="


> +		    next_op->opnum == OP_SAVEFH &&
> +		    args->is_inter_server_copy) {
> +			/*
> +			 * RFC 7862 section 15.2.3 says:
> +			 *  If a server supports the inter-server copy
> +			 *  feature, a PUTFH followed by a SAVEFH MUST
> +			 *  NOT return NFS4ERR_STALE for either
> +			 *  operation.
> +			 * We limit this to when there is a COPY
> +			 * in the COMPOUND, and extend it to
> +			 * NFS4ERR_BADHANDLE.

Extending to match NFS4ERR_BADHANDLE as well explicitly does /not/
comply with RFC 7862, as you say above. So the short description is
misleading.


> +			 */
> +			ret = 0;
> +		}
> +	}
>  #endif
>  	return ret;
>  }
> @@ -2809,45 +2829,6 @@ static bool need_wrongsec_check(struct svc_rqst *rqstp)
>  	return !(nextd->op_flags & OP_HANDLES_WRONGSEC);
>  }
>  
> -#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> -static void
> -check_if_stalefh_allowed(struct nfsd4_compoundargs *args)
> -{
> -	struct nfsd4_op	*op, *current_op = NULL, *saved_op = NULL;
> -	struct nfsd4_copy *copy;
> -	struct nfsd4_putfh *putfh;
> -	int i;
> -
> -	/* traverse all operation and if it's a COPY compound, mark the
> -	 * source filehandle to skip verification
> -	 */
> -	for (i = 0; i < args->opcnt; i++) {
> -		op = &args->ops[i];
> -		if (op->opnum == OP_PUTFH)
> -			current_op = op;
> -		else if (op->opnum == OP_SAVEFH)
> -			saved_op = current_op;
> -		else if (op->opnum == OP_RESTOREFH)
> -			current_op = saved_op;
> -		else if (op->opnum == OP_COPY) {
> -			copy = (struct nfsd4_copy *)&op->u;
> -			if (!saved_op) {
> -				op->status = nfserr_nofilehandle;
> -				return;
> -			}
> -			putfh = (struct nfsd4_putfh *)&saved_op->u;
> -			if (nfsd4_ssc_is_inter(copy))
> -				putfh->no_verify = true;
> -		}
> -	}
> -}
> -#else
> -static void
> -check_if_stalefh_allowed(struct nfsd4_compoundargs *args)
> -{
> -}
> -#endif
> -
>  /*
>   * COMPOUND call.
>   */
> @@ -2897,7 +2878,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  		resp->opcnt = 1;
>  		goto encode_op;
>  	}
> -	check_if_stalefh_allowed(args);
>  
>  	rqstp->rq_lease_breaker = (void **)&cstate->clp;
>  
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 51ef97c25456..7e44af3d10b9 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1250,7 +1250,6 @@ nfsd4_decode_putfh(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
>  	if (!putfh->pf_fhval)
>  		return nfserr_jukebox;
>  
> -	putfh->no_verify = false;
>  	return nfs_ok;
>  }
>  
> @@ -2047,6 +2046,7 @@ nfsd4_decode_copy(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
>  	if (status)
>  		return status;
>  
> +	argp->is_inter_server_copy = true;
>  	ns_dummy = kmalloc(sizeof(struct nl4_server), GFP_KERNEL);
>  	if (ns_dummy == NULL)
>  		return nfserr_jukebox;
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 1f0967236cc2..3de8f4e07c49 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -335,7 +335,6 @@ struct nfsd4_lookup {
>  struct nfsd4_putfh {
>  	u32		pf_fhlen;           /* request */
>  	char		*pf_fhval;          /* request */
> -	bool		no_verify;	    /* represents foreigh fh */
>  };
>  
>  struct nfsd4_getxattr {
> @@ -907,6 +906,7 @@ struct nfsd4_compoundargs {
>  	u32				client_opcnt;
>  	u32				opcnt;
>  	bool				splice_ok;
> +	bool				is_inter_server_copy;
>  	struct nfsd4_op			*ops;
>  	struct nfsd4_op			iops[8];
>  };


-- 
Chuck Lever

Re: [PATCH v5 03/11] nfsd: simplify foreign-filehandle handling to better match RFC-7862

[NeilBrown]

On Thu, 20 Nov 2025, Chuck Lever wrote:
> On 11/18/25 10:28 PM, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > When the COPY v4.2 op is used in the inter-server copy mode, the file
> > handle of the source file (presented as the saved filehandle to COPY)
> > is for a different ("foreign") file server which would not be expected
> > to be understood by this server.  fh_verify() might return nfserr_stale
> > or nfserr_badhandle.
> > 
> > In order of this filehandle to still be available to COPY, both PUTFH
> > and SAVEFH much not trigger an error.
> > 
> > RFC 7862 section 15.2.3 says:
> > 
> >    If the request is for an inter-server copy, the source-fh is a
> >    filehandle from the source server and the COMPOUND procedure is being
> >    executed on the destination server.  In this case, the source-fh is a
> >    foreign filehandle on the server receiving the COPY request.  If
> >    either PUTFH or SAVEFH checked the validity of the filehandle, the
> >    operation would likely fail and return NFS4ERR_STALE.
> > 
> >    If a server supports the inter-server copy feature, a PUTFH followed
> >    by a SAVEFH MUST NOT return NFS4ERR_STALE for either operation.
> >    These restrictions do not pose substantial difficulties for servers.
> >    CURRENT_FH and SAVED_FH may be validated in the context of the
> >    operation referencing them and an NFS4ERR_STALE error returned for an
> >    invalid filehandle at that point.
> > 
> > [The RFC neglects the possibility of NFS4ERR_BADHANDLE]
> > 
> > Linux nfsd currently takes a different approach.  Rather than just
> > checking for "a PUTFH followed by a SAVEFH", it goes further to consider
> > only that case when this filehandle is then used for COPY, and allows
> > that it might have been subject of RESTOREFH and SAVEFH in between.
> > 
> > This is not a problem in itself except for the extra code with little
> > benefit.  This analysis of the COMPOUND to detect PUTFH ops which need
> > care is performed on every COMPOUND request, which is not necessary.
> > 
> > It is sufficient to check if the relevant conditions are met only when a
> > PUTFH op actually receives an error from fh_verify().
> > 
> > This patch removes the checking code from common paths and places it in
> > nfsd4_putfh() only when fh_verify() returns a relevant error.
> > 
> > Rather than scanning ahead for a COPY, this patch notes (in
> > nfsd4_compoundargs) when an inter-server COPY is decoded, and in that
> > case only checks the next op to see if it is SAVEFH as this is what the
> > RFC requires.
> > 
> > A test on "inter_copy_offload_enable" is also added to be completely
> > consistent with the "If a server supports the inter-server copy feature"
> > precondition.
> > 
> > As we do this test in nfsd4_putfh() there is no now need to mark the
> > putfh op as "no_verify".
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/nfsd/nfs4proc.c | 64 ++++++++++++++++------------------------------
> >  fs/nfsd/nfs4xdr.c  |  2 +-
> >  fs/nfsd/xdr4.h     |  2 +-
> >  3 files changed, 24 insertions(+), 44 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 3160e899a5da..e6f8b5b907a9 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -693,8 +693,28 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	       putfh->pf_fhlen);
> >  	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
> >  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > -	if (ret == nfserr_stale && putfh->no_verify)
> > -		ret = 0;
> > +	if ((ret == nfserr_badhandle || ret == nfserr_stale) &&
> > +	    inter_copy_offload_enable) {
> > +		struct nfsd4_compoundargs *args = rqstp->rq_argp;
> > +		struct nfsd4_compoundres *resp = rqstp->rq_resp;
> > +		struct nfsd4_op	*next_op = &args->ops[resp->opcnt];
> > +
> 
> I find the initializer confusing -- it's only generating an address,
> but not yet dereferencing it -- but it can generate an address beyond
> the end of the args->ops array.

"confusing", yet you understand it perfectly!

The var is only used once so I'll drop it.
Though then I need a comment explaining that

		/*
		 * args->opcnt is the number of ops in the request.
		 * resp->opcnt is the number of ops, including this
		 * one, that have been processed, so it points
		 * to the next op.
		 */

> 
> 
> > +		if (resp->opcnt <= args->opcnt &&
> 
> In fact the "resp->opcnt <= args->opcnt" check allows accessing
> the N+1th array element, since the array is indexed 0 to N-1. So
> the condition here needs to by "<" not "<="

and having that new comment make this observation clear too - thanks.

> 
> 
> > +		    next_op->opnum == OP_SAVEFH &&
> > +		    args->is_inter_server_copy) {
> > +			/*
> > +			 * RFC 7862 section 15.2.3 says:
> > +			 *  If a server supports the inter-server copy
> > +			 *  feature, a PUTFH followed by a SAVEFH MUST
> > +			 *  NOT return NFS4ERR_STALE for either
> > +			 *  operation.
> > +			 * We limit this to when there is a COPY
> > +			 * in the COMPOUND, and extend it to
> > +			 * NFS4ERR_BADHANDLE.
> 
> Extending to match NFS4ERR_BADHANDLE as well explicitly does /not/
> comply with RFC 7862, as you say above. So the short description is
> misleading.

Does:
			/*
			 * RFC 7862 section 15.2.3 says:
			 *  If a server supports the inter-server copy
			 *  feature, a PUTFH followed by a SAVEFH MUST
			 *  NOT return NFS4ERR_STALE for either
			 *  operation.
			 * We limit this to when there is a COPY
			 * in the COMPOUND, and extend it to
			 * also ignore NFS4ERR_BADHANDLE despite the
			 * RFC not requiring this.  If the remote
			 * server is running a different NFS implementation,
			 * NFS4ERR_BADHANDLE is a likely error.
			 */

resolve your concern?

Thanks,
NeilBrown

Re: [PATCH v5 03/11] nfsd: simplify foreign-filehandle handling to better match RFC-7862

[Chuck Lever]

On 11/19/25 4:38 PM, NeilBrown wrote:
>> Extending to match NFS4ERR_BADHANDLE as well explicitly does /not/
>> comply with RFC 7862, as you say above. So the short description is
>> misleading.
> Does:
> 			/*
> 			 * RFC 7862 section 15.2.3 says:
> 			 *  If a server supports the inter-server copy
> 			 *  feature, a PUTFH followed by a SAVEFH MUST
> 			 *  NOT return NFS4ERR_STALE for either
> 			 *  operation.
> 			 * We limit this to when there is a COPY
> 			 * in the COMPOUND, and extend it to
> 			 * also ignore NFS4ERR_BADHANDLE despite the
> 			 * RFC not requiring this.  If the remote
> 			 * server is running a different NFS implementation,
> 			 * NFS4ERR_BADHANDLE is a likely error.
> 			 */
> 
> resolve your concern?

Well the concern was mostly about the mismatch between the intent of the
patch, as stated in the short description, and this comment.

But, without elaborating, the RFC does not place any mandate on
NFS4ERR_BADHANDLE. It's hard to say whether ignoring that status code is
going to be consequential. I'll have to study up.


-- 
Chuck Lever

Re: [PATCH v5 03/11] nfsd: simplify foreign-filehandle handling to better match RFC-7862

[NeilBrown]

On Fri, 21 Nov 2025, Chuck Lever wrote:
> On 11/19/25 4:38 PM, NeilBrown wrote:
> >> Extending to match NFS4ERR_BADHANDLE as well explicitly does /not/
> >> comply with RFC 7862, as you say above. So the short description is
> >> misleading.
> > Does:
> > 			/*
> > 			 * RFC 7862 section 15.2.3 says:
> > 			 *  If a server supports the inter-server copy
> > 			 *  feature, a PUTFH followed by a SAVEFH MUST
> > 			 *  NOT return NFS4ERR_STALE for either
> > 			 *  operation.
> > 			 * We limit this to when there is a COPY
> > 			 * in the COMPOUND, and extend it to
> > 			 * also ignore NFS4ERR_BADHANDLE despite the
> > 			 * RFC not requiring this.  If the remote
> > 			 * server is running a different NFS implementation,
> > 			 * NFS4ERR_BADHANDLE is a likely error.
> > 			 */
> > 
> > resolve your concern?
> 
> Well the concern was mostly about the mismatch between the intent of the
> patch, as stated in the short description, and this comment.
> 
> But, without elaborating, the RFC does not place any mandate on
> NFS4ERR_BADHANDLE. It's hard to say whether ignoring that status code is
> going to be consequential. I'll have to study up.

Ah - I think I understand now. The text

 nfsd: simplify foreign-filehandle handling to better match RFC-7862

is inconsistent with adding adding NFS4ERR_BADHANDLE handling which is
*not* consistent with RFC-7862.
I'll move that bit to a separate patch.

Thanks,
NeilBrown

Re: [PATCH v5 03/11] nfsd: simplify foreign-filehandle handling to better match RFC-7862

[Jeff Layton]

On Wed, 2025-11-19 at 14:28 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> When the COPY v4.2 op is used in the inter-server copy mode, the file
> handle of the source file (presented as the saved filehandle to COPY)
> is for a different ("foreign") file server which would not be expected
> to be understood by this server.  fh_verify() might return nfserr_stale
> or nfserr_badhandle.
> 
> In order of this filehandle to still be available to COPY, both PUTFH
> and SAVEFH much not trigger an error.
> 
> RFC 7862 section 15.2.3 says:
> 
>    If the request is for an inter-server copy, the source-fh is a
>    filehandle from the source server and the COMPOUND procedure is being
>    executed on the destination server.  In this case, the source-fh is a
>    foreign filehandle on the server receiving the COPY request.  If
>    either PUTFH or SAVEFH checked the validity of the filehandle, the
>    operation would likely fail and return NFS4ERR_STALE.
> 
>    If a server supports the inter-server copy feature, a PUTFH followed
>    by a SAVEFH MUST NOT return NFS4ERR_STALE for either operation.
>    These restrictions do not pose substantial difficulties for servers.
>    CURRENT_FH and SAVED_FH may be validated in the context of the
>    operation referencing them and an NFS4ERR_STALE error returned for an
>    invalid filehandle at that point.
> 
> [The RFC neglects the possibility of NFS4ERR_BADHANDLE]
> 
> Linux nfsd currently takes a different approach.  Rather than just
> checking for "a PUTFH followed by a SAVEFH", it goes further to consider
> only that case when this filehandle is then used for COPY, and allows
> that it might have been subject of RESTOREFH and SAVEFH in between.
> 
> This is not a problem in itself except for the extra code with little
> benefit.  This analysis of the COMPOUND to detect PUTFH ops which need
> care is performed on every COMPOUND request, which is not necessary.
> 
> It is sufficient to check if the relevant conditions are met only when a
> PUTFH op actually receives an error from fh_verify().
> 
> This patch removes the checking code from common paths and places it in
> nfsd4_putfh() only when fh_verify() returns a relevant error.
> 
> Rather than scanning ahead for a COPY, this patch notes (in
> nfsd4_compoundargs) when an inter-server COPY is decoded, and in that
> case only checks the next op to see if it is SAVEFH as this is what the
> RFC requires.
> 
> A test on "inter_copy_offload_enable" is also added to be completely
> consistent with the "If a server supports the inter-server copy feature"
> precondition.
> 
> As we do this test in nfsd4_putfh() there is no now need to mark the
> putfh op as "no_verify".
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 64 ++++++++++++++++------------------------------
>  fs/nfsd/nfs4xdr.c  |  2 +-
>  fs/nfsd/xdr4.h     |  2 +-
>  3 files changed, 24 insertions(+), 44 deletions(-)
> 

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

[PATCH v5 04/11] nfsd: report correct error for attempt to use foreign filehandle

[NeilBrown]

From: NeilBrown <neil@brown.name>

A COMPOUND with
   PUTFH foreign filehandle
   SAVFH
   RESTOREFH
   some-op-that-uses-current_fh
   COPY

will report NFS4ERR_NOHANDLE for that 4th op as ALLOWED_WITHOUT_LOCAL_FH
is set and ->fh_dentry is NULL.  However that error is not correct.
NFS4ERR_STALE or NFS4ERR_BADHANDLE would be the correct error.

This patch saves the correct error and reports it when appropriate.

It is highly unlikely that a client would ever notice this difference as
the COMPOUND that would produce it is bizarre.  Maybe we don't need this
patch.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 4 +++-
 fs/nfsd/xdr4.h     | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index e6f8b5b907a9..e61b1ee6c8d8 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -712,6 +712,7 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			 * in the COMPOUND, and extend it to
 			 * NFS4ERR_BADHANDLE.
 			 */
+			cstate->saved_status = ret;
 			ret = 0;
 		}
 	}
@@ -2856,6 +2857,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 	resp->tag = args->tag;
 	resp->rqstp = rqstp;
 	cstate->minorversion = args->minorversion;
+	cstate->saved_status = nfserr_nofilehandle;
 	fh_init(current_fh, NFS4_FHSIZE);
 	fh_init(save_fh, NFS4_FHSIZE);
 	/*
@@ -2907,7 +2909,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 		}
 		if (!current_fh->fh_dentry) {
 			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_LOCAL_FH)) {
-				op->status = nfserr_nofilehandle;
+				op->status = cstate->saved_status;
 				goto encode_op;
 			}
 		} else if (current_fh->fh_export->ex_fslocs.migrated &&
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 3de8f4e07c49..bcaf631ec12d 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -187,10 +187,11 @@ struct nfsd4_compound_state {
 	struct nfsd4_session	*session;
 	struct nfsd4_slot	*slot;
 	int			data_offset;
-	bool                    spo_must_allowed;
+	bool			spo_must_allowed;
 	size_t			iovlen;
 	u32			minorversion;
 	__be32			status;
+	__be32			saved_status;
 	stateid_t	current_stateid;
 	stateid_t	save_stateid;
 	/* to indicate current and saved state id presents */
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v5 04/11] nfsd: report correct error for attempt to use foreign filehandle

[Jeff Layton]

On Wed, 2025-11-19 at 14:28 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> A COMPOUND with
>    PUTFH foreign filehandle
>    SAVFH
>    RESTOREFH
>    some-op-that-uses-current_fh
>    COPY
> 
> will report NFS4ERR_NOHANDLE for that 4th op as ALLOWED_WITHOUT_LOCAL_FH
> is set and ->fh_dentry is NULL.  However that error is not correct.
> NFS4ERR_STALE or NFS4ERR_BADHANDLE would be the correct error.
> 
> This patch saves the correct error and reports it when appropriate.
> 
> It is highly unlikely that a client would ever notice this difference as
> the COMPOUND that would produce it is bizarre.  Maybe we don't need this
> patch.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 4 +++-
>  fs/nfsd/xdr4.h     | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e6f8b5b907a9..e61b1ee6c8d8 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -712,6 +712,7 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  			 * in the COMPOUND, and extend it to
>  			 * NFS4ERR_BADHANDLE.
>  			 */
> +			cstate->saved_status = ret;
>  			ret = 0;
>  		}
>  	}
> @@ -2856,6 +2857,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  	resp->tag = args->tag;
>  	resp->rqstp = rqstp;
>  	cstate->minorversion = args->minorversion;
> +	cstate->saved_status = nfserr_nofilehandle;
>  	fh_init(current_fh, NFS4_FHSIZE);
>  	fh_init(save_fh, NFS4_FHSIZE);
>  	/*
> @@ -2907,7 +2909,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  		}
>  		if (!current_fh->fh_dentry) {
>  			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_LOCAL_FH)) {
> -				op->status = nfserr_nofilehandle;
> +				op->status = cstate->saved_status;
>  				goto encode_op;
>  			}
>  		} else if (current_fh->fh_export->ex_fslocs.migrated &&
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 3de8f4e07c49..bcaf631ec12d 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -187,10 +187,11 @@ struct nfsd4_compound_state {
>  	struct nfsd4_session	*session;
>  	struct nfsd4_slot	*slot;
>  	int			data_offset;
> -	bool                    spo_must_allowed;
> +	bool			spo_must_allowed;
>  	size_t			iovlen;
>  	u32			minorversion;
>  	__be32			status;
> +	__be32			saved_status;
>  	stateid_t	current_stateid;
>  	stateid_t	save_stateid;
>  	/* to indicate current and saved state id presents */

It seems like a reasonable thing to do, even if not strictly needed.
nfsd does occasionally have to deal with weird compounds coming from
stuff like pynfs. Hmm...maybe we need a pynfs test that crafts such a
compound?

Anyway, you can add this, but I'm ok with dropping it if you prefer.

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

[PATCH v5 05/11] nfsd: drop explicit tests for special stateids which would be invalid.

[NeilBrown]

From: NeilBrown <neil@brown.name>

In two places nfsd has code to test for special stateids and to report
nfserr_bad_stateid if they are found.
One is for handling TEST_STATEID ops which always forbid these stateids,
and one is for all other places that a stateid is used, and the code is
*after* any checks for special stateids which might be permitted.

These tests add no value.  In each case there is a subsequent lookup for
the stateid which will return the same error code if the stateid is not
found, and special stateids never will be found.

Special stateids have a si.opaque.so_id which is either 0 or UINT_MAX.
Stateids stored in the idr only have so_id ranging from 1 or INT_MAX.
So there is no possibility of a special stateid being found.

Having the explicit test optimises the unexpected case where a special
stateid is incorrectly given, and adds unnecessary comparisons to the
common case of a non-special stateid being given.

In nfsd4_lookup_stateid(), simply removing the test would mean that
a special stateid could result in the incorrect nfserr_stale_stateid
error, as the validity of so_clid is checked before so_id.  So we
add extra checks to only return nfserr_stale_stateid if the stateid
looks like it might have been locally generated - so_id not
all zeroes or all ones.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4state.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 35004568d43e..ea931e606f40 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -74,6 +74,23 @@ static const stateid_t close_stateid = {
 	.si_generation = 0xffffffffU,
 };
 
+/*
+ * In NFSv4.0 there is a case were we should return NFS4ERR_STALE_STATEID
+ * if the stateid looks like one we might have created previously, and
+ * NFS4ERR_BAD_STATEID if it looks like it was never valid.
+ * There is not a lot of redundancy in the stateid that can be used to make
+ * this distinction, but it would be useful to differentiate special
+ * stateids from locally generated stateid.
+ * Special stateids have si.opaque.so_id being either all zeros or all 1s,
+ * so 0 or (u32)-1. Locally generated stateids have si.opaque.so_id as
+ * a number from 1 to INT_MAX (as generated by idr_alloc_cyclic()).
+ * We can test for the later range with some simple arithmetic.
+ */
+static inline bool stateid_well_formed(stateid_t *stid)
+{
+	return (stid->si_opaque.so_id - 1) < INT_MAX;
+}
+
 static u64 current_sessionid = 1;
 
 #define ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
@@ -7129,9 +7146,6 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 	struct nfs4_stid *s;
 	__be32 status = nfserr_bad_stateid;
 
-	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
-		CLOSE_STATEID(stateid))
-		return status;
 	spin_lock(&cl->cl_lock);
 	s = find_stateid_locked(cl, stateid);
 	if (!s)
@@ -7186,14 +7200,15 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 
 	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
 
-	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
-		CLOSE_STATEID(stateid))
-		return nfserr_bad_stateid;
 	status = set_client(&stateid->si_opaque.so_clid, cstate, nn);
 	if (status == nfserr_stale_clientid) {
-		if (cstate->session)
-			return nfserr_bad_stateid;
-		return nfserr_stale_stateid;
+		if (!cstate->session && stateid_well_formed(stateid))
+			/*
+			 * Might be from earlier instance - v4.0 likes
+			 * to know
+			 */
+			return nfserr_stale_stateid;
+		return nfserr_bad_stateid;
 	}
 	if (status)
 		return status;
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v5 05/11] nfsd: drop explicit tests for special stateids which would be invalid.

[Chuck Lever]

On 11/18/25 10:28 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> In two places nfsd has code to test for special stateids and to report
> nfserr_bad_stateid if they are found.
> One is for handling TEST_STATEID ops which always forbid these stateids,
> and one is for all other places that a stateid is used, and the code is
> *after* any checks for special stateids which might be permitted.
> 
> These tests add no value.  In each case there is a subsequent lookup for
> the stateid which will return the same error code if the stateid is not
> found, and special stateids never will be found.
> 
> Special stateids have a si.opaque.so_id which is either 0 or UINT_MAX.
> Stateids stored in the idr only have so_id ranging from 1 or INT_MAX.
> So there is no possibility of a special stateid being found.
> 
> Having the explicit test optimises the unexpected case where a special
> stateid is incorrectly given, and adds unnecessary comparisons to the
> common case of a non-special stateid being given.
> 
> In nfsd4_lookup_stateid(), simply removing the test would mean that
> a special stateid could result in the incorrect nfserr_stale_stateid
> error, as the validity of so_clid is checked before so_id.  So we
> add extra checks to only return nfserr_stale_stateid if the stateid
> looks like it might have been locally generated - so_id not
> all zeroes or all ones.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 35004568d43e..ea931e606f40 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -74,6 +74,23 @@ static const stateid_t close_stateid = {
>  	.si_generation = 0xffffffffU,
>  };
>  
> +/*
> + * In NFSv4.0 there is a case were we should return NFS4ERR_STALE_STATEID

s/were/where

Nit: Throughout, when referring to the implementation, instead of "we" I
like "NFSD" a little better. Using "we" to refer to developers or the
community interest is OK.


> + * if the stateid looks like one we might have created previously, and
> + * NFS4ERR_BAD_STATEID if it looks like it was never valid.
> + * There is not a lot of redundancy in the stateid that can be used to make
> + * this distinction, but it would be useful to differentiate special
> + * stateids from locally generated stateid.
> + * Special stateids have si.opaque.so_id being either all zeros or all 1s,
> + * so 0 or (u32)-1. Locally generated stateids have si.opaque.so_id as
> + * a number from 1 to INT_MAX (as generated by idr_alloc_cyclic()).
> + * We can test for the later range with some simple arithmetic.
> + */
> +static inline bool stateid_well_formed(stateid_t *stid)
> +{
> +	return (stid->si_opaque.so_id - 1) < INT_MAX;
> +}
> +
>  static u64 current_sessionid = 1;
>  
>  #define ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
> @@ -7129,9 +7146,6 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  	struct nfs4_stid *s;
>  	__be32 status = nfserr_bad_stateid;
>  
> -	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> -		CLOSE_STATEID(stateid))
> -		return status;
>  	spin_lock(&cl->cl_lock);
>  	s = find_stateid_locked(cl, stateid);
>  	if (!s)
> @@ -7186,14 +7200,15 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  
>  	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
>  
> -	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> -		CLOSE_STATEID(stateid))
> -		return nfserr_bad_stateid;
>  	status = set_client(&stateid->si_opaque.so_clid, cstate, nn);
>  	if (status == nfserr_stale_clientid) {
> -		if (cstate->session)
> -			return nfserr_bad_stateid;
> -		return nfserr_stale_stateid;
> +		if (!cstate->session && stateid_well_formed(stateid))
> +			/*
> +			 * Might be from earlier instance - v4.0 likes
> +			 * to know
> +			 */
> +			return nfserr_stale_stateid;
> +		return nfserr_bad_stateid;
>  	}
>  	if (status)
>  		return status;


-- 
Chuck Lever

Re: [PATCH v5 05/11] nfsd: drop explicit tests for special stateids which would be invalid.

[Jeff Layton]

On Wed, 2025-11-19 at 14:28 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> In two places nfsd has code to test for special stateids and to report
> nfserr_bad_stateid if they are found.
> One is for handling TEST_STATEID ops which always forbid these stateids,
> and one is for all other places that a stateid is used, and the code is
> *after* any checks for special stateids which might be permitted.
> 
> These tests add no value.  In each case there is a subsequent lookup for
> the stateid which will return the same error code if the stateid is not
> found, and special stateids never will be found.
> 
> Special stateids have a si.opaque.so_id which is either 0 or UINT_MAX.
> Stateids stored in the idr only have so_id ranging from 1 or INT_MAX.
> So there is no possibility of a special stateid being found.
> 
> Having the explicit test optimises the unexpected case where a special
> stateid is incorrectly given, and adds unnecessary comparisons to the
> common case of a non-special stateid being given.
> 
> In nfsd4_lookup_stateid(), simply removing the test would mean that
> a special stateid could result in the incorrect nfserr_stale_stateid
> error, as the validity of so_clid is checked before so_id.  So we
> add extra checks to only return nfserr_stale_stateid if the stateid
> looks like it might have been locally generated - so_id not
> all zeroes or all ones.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 35004568d43e..ea931e606f40 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -74,6 +74,23 @@ static const stateid_t close_stateid = {
>  	.si_generation = 0xffffffffU,
>  };
>  
> +/*
> + * In NFSv4.0 there is a case were we should return NFS4ERR_STALE_STATEID
> + * if the stateid looks like one we might have created previously, and
> + * NFS4ERR_BAD_STATEID if it looks like it was never valid.
> + * There is not a lot of redundancy in the stateid that can be used to make
> + * this distinction, but it would be useful to differentiate special
> + * stateids from locally generated stateid.
> + * Special stateids have si.opaque.so_id being either all zeros or all 1s,
> + * so 0 or (u32)-1. Locally generated stateids have si.opaque.so_id as
> + * a number from 1 to INT_MAX (as generated by idr_alloc_cyclic()).
> + * We can test for the later range with some simple arithmetic.
> + */
> +static inline bool stateid_well_formed(stateid_t *stid)
> +{
> +	return (stid->si_opaque.so_id - 1) < INT_MAX;
> +}
> +
>  static u64 current_sessionid = 1;
>  
>  #define ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
> @@ -7129,9 +7146,6 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  	struct nfs4_stid *s;
>  	__be32 status = nfserr_bad_stateid;
>  
> -	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> -		CLOSE_STATEID(stateid))
> -		return status;
>  	spin_lock(&cl->cl_lock);
>  	s = find_stateid_locked(cl, stateid);
>  	if (!s)
> @@ -7186,14 +7200,15 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  
>  	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
>  
> -	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> -		CLOSE_STATEID(stateid))
> -		return nfserr_bad_stateid;
>  	status = set_client(&stateid->si_opaque.so_clid, cstate, nn);
>  	if (status == nfserr_stale_clientid) {
> -		if (cstate->session)
> -			return nfserr_bad_stateid;
> -		return nfserr_stale_stateid;
> +		if (!cstate->session && stateid_well_formed(stateid))
> +			/*
> +			 * Might be from earlier instance - v4.0 likes
> +			 * to know
> +			 */
> +			return nfserr_stale_stateid;
> +		return nfserr_bad_stateid;
>  	}
>  	if (status)
>  		return status;

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

[PATCH v5 06/11] nfsd: revise names of special stateid, and predicate functions.

[NeilBrown]

From: NeilBrown <neil@brown.name>

When I see "CURRENT_STATEID(foo)" in the code it is not clear that this
is testing the stateid to see if it is a special stateid.  I find that
IS_CURRENT_STATEID(foo) is clearer.  But an inline function is even
better, so is_current_stateid().

There are other special stateids which are described in RFC 8881 Section
8.2.3 as "anonymous", "READ bypass", and "invalid".  The nfsd code
currently names them "zero", "one" and "close" which doesn't help with
comparing the code to the RFC.

So this patch changes the names of those special stateids and adds
"is_" to the front of the predicates.

As CLOSE_STATEID() was not needed, it is discarded rather than replacing
with is_invalid_stateid().

I felt that is_read_bypass_stateid() was a little too verbose, so I made
it is_bypass_stateid().

For consistency, invalid_stateid is changed to use ~0 rather than
0xffffffffU for the generation number.  (RFC 8881 say to use
"NFS4_UINT32_MAX" for the generation number here, and "all ones" for the
generation and opaque of anon_stateid).

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ea931e606f40..f92b01bdb4dd 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -60,18 +60,18 @@
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
 #define all_ones {{ ~0, ~0}, ~0}
-static const stateid_t one_stateid = {
+static const stateid_t read_bypass_stateid = {
 	.si_generation = ~0,
 	.si_opaque = all_ones,
 };
-static const stateid_t zero_stateid = {
+static const stateid_t anon_stateid = {
 	/* all fields zero */
 };
-static const stateid_t currentstateid = {
+static const stateid_t current_stateid = {
 	.si_generation = 1,
 };
-static const stateid_t close_stateid = {
-	.si_generation = 0xffffffffU,
+static const stateid_t invalid_stateid = {
+	.si_generation = ~0,
 };
 
 /*
@@ -93,10 +93,16 @@ static inline bool stateid_well_formed(stateid_t *stid)
 
 static u64 current_sessionid = 1;
 
-#define ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
-#define ONE_STATEID(stateid)  (!memcmp((stateid), &one_stateid, sizeof(stateid_t)))
-#define CURRENT_STATEID(stateid) (!memcmp((stateid), &currentstateid, sizeof(stateid_t)))
-#define CLOSE_STATEID(stateid)  (!memcmp((stateid), &close_stateid, sizeof(stateid_t)))
+/* These special stateid are defined in RFC 8881 Section 8.2.3 */
+static inline bool is_anon_stateid(stateid_t *stateid) {
+	return memcmp(stateid, &anon_stateid, sizeof(stateid_t));
+}
+static inline bool is_bypass_stateid(stateid_t *stateid) {
+	return memcmp(stateid, &read_bypass_stateid, sizeof(stateid_t));
+}
+static inline bool is_current_stateid(stateid_t *stateid) {
+	return memcmp(stateid, &current_stateid, sizeof(stateid_t));
+}
 
 /* forward declarations */
 static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
@@ -388,7 +394,7 @@ nfsd4_cb_notify_lock_prepare(struct nfsd4_callback *cb)
 static int
 nfsd4_cb_notify_lock_done(struct nfsd4_callback *cb, struct rpc_task *task)
 {
-	trace_nfsd_cb_notify_lock_done(&zero_stateid, task);
+	trace_nfsd_cb_notify_lock_done(&anon_stateid, task);
 
 	/*
 	 * Since this is just an optimization, we don't try very hard if it
@@ -6512,7 +6518,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 * open stateid would have to be created.
 	 */
 	if (new_stp && open_xor_delegation(open)) {
-		memcpy(&open->op_stateid, &zero_stateid, sizeof(open->op_stateid));
+		memcpy(&open->op_stateid, &anon_stateid, sizeof(open->op_stateid));
 		open->op_rflags |= OPEN4_RESULT_NO_OPEN_STATEID;
 		release_open_stateid(stp);
 	}
@@ -7076,7 +7082,7 @@ __be32 nfs4_check_openmode(struct nfs4_ol_stateid *stp, int flags)
 static inline __be32
 check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid, int flags)
 {
-	if (ONE_STATEID(stateid) && (flags & RD_STATE))
+	if (is_bypass_stateid(stateid) && (flags & RD_STATE))
 		return nfs_ok;
 	else if (opens_in_grace(net)) {
 		/* Answer in remaining cases depends on existence of
@@ -7085,7 +7091,7 @@ check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid,
 	} else if (flags & WR_STATE)
 		return nfs4_share_conflict(current_fh,
 				NFS4_SHARE_DENY_WRITE);
-	else /* (flags & RD_STATE) && ZERO_STATEID(stateid) */
+	else /* (flags & RD_STATE) && is_anon_stateid(stateid) */
 		return nfs4_share_conflict(current_fh,
 				NFS4_SHARE_DENY_READ);
 }
@@ -7401,7 +7407,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 	if (nfp)
 		*nfp = NULL;
 
-	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) {
+	if (is_anon_stateid(stateid) || is_bypass_stateid(stateid)) {
 		status = check_special_stateids(net, fhp, stateid, flags);
 		goto done;
 	}
@@ -7823,12 +7829,12 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	/* v4.1+ suggests that we send a special stateid in here, since the
 	 * clients should just ignore this anyway. Since this is not useful
-	 * for v4.0 clients either, we set it to the special close_stateid
+	 * for v4.0 clients either, we set it to the special invalid_stateid
 	 * universally.
 	 *
 	 * See RFC5661 section 18.2.4, and RFC7530 section 16.2.5
 	 */
-	memcpy(&close->cl_stateid, &close_stateid, sizeof(close->cl_stateid));
+	memcpy(&close->cl_stateid, &invalid_stateid, sizeof(close->cl_stateid));
 
 	/* put reference from nfs4_preprocess_seqid_op */
 	nfs4_put_stid(&stp->st_stid);
@@ -9101,7 +9107,7 @@ static void
 get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
 {
 	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG) &&
-	    CURRENT_STATEID(stateid))
+	    is_current_stateid(stateid))
 		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
 }
 
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v5 06/11] nfsd: revise names of special stateid, and predicate functions.

[Chuck Lever]

On 11/18/25 10:28 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> When I see "CURRENT_STATEID(foo)" in the code it is not clear that this
> is testing the stateid to see if it is a special stateid.  I find that
> IS_CURRENT_STATEID(foo) is clearer.  But an inline function is even
> better, so is_current_stateid().
> 
> There are other special stateids which are described in RFC 8881 Section
> 8.2.3 as "anonymous", "READ bypass", and "invalid".  The nfsd code
> currently names them "zero", "one" and "close" which doesn't help with
> comparing the code to the RFC.
> 
> So this patch changes the names of those special stateids and adds
> "is_" to the front of the predicates.
> 
> As CLOSE_STATEID() was not needed, it is discarded rather than replacing
> with is_invalid_stateid().
> 
> I felt that is_read_bypass_stateid() was a little too verbose, so I made
> it is_bypass_stateid().
> 
> For consistency, invalid_stateid is changed to use ~0 rather than
> 0xffffffffU for the generation number.  (RFC 8881 say to use
> "NFS4_UINT32_MAX" for the generation number here, and "all ones" for the
> generation and opaque of anon_stateid).
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ea931e606f40..f92b01bdb4dd 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -60,18 +60,18 @@
>  #define NFSDDBG_FACILITY                NFSDDBG_PROC
>  
>  #define all_ones {{ ~0, ~0}, ~0}
> -static const stateid_t one_stateid = {
> +static const stateid_t read_bypass_stateid = {
>  	.si_generation = ~0,
>  	.si_opaque = all_ones,
>  };
> -static const stateid_t zero_stateid = {
> +static const stateid_t anon_stateid = {
>  	/* all fields zero */
>  };
> -static const stateid_t currentstateid = {
> +static const stateid_t current_stateid = {
>  	.si_generation = 1,
>  };
> -static const stateid_t close_stateid = {
> -	.si_generation = 0xffffffffU,
> +static const stateid_t invalid_stateid = {
> +	.si_generation = ~0,
>  };
>  
>  /*
> @@ -93,10 +93,16 @@ static inline bool stateid_well_formed(stateid_t *stid)
>  
>  static u64 current_sessionid = 1;
>  
> -#define ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
> -#define ONE_STATEID(stateid)  (!memcmp((stateid), &one_stateid, sizeof(stateid_t)))
> -#define CURRENT_STATEID(stateid) (!memcmp((stateid), &currentstateid, sizeof(stateid_t)))
> -#define CLOSE_STATEID(stateid)  (!memcmp((stateid), &close_stateid, sizeof(stateid_t)))
> +/* These special stateid are defined in RFC 8881 Section 8.2.3 */
> +static inline bool is_anon_stateid(stateid_t *stateid) {
> +	return memcmp(stateid, &anon_stateid, sizeof(stateid_t));
> +}
> +static inline bool is_bypass_stateid(stateid_t *stateid) {
> +	return memcmp(stateid, &read_bypass_stateid, sizeof(stateid_t));
> +}
> +static inline bool is_current_stateid(stateid_t *stateid) {
> +	return memcmp(stateid, &current_stateid, sizeof(stateid_t));
> +}

The new static inline functions appear to invert the logic -- the macros
use "!memcmp" but the new functions omit the "!". memcmp() returns an
int, so there is an implicit type conversion here as well. So maybe you
want "memcmp(stateid, ... ) == 0" ?

And now we can use "sizeof(*stateid)" here which is slightly less
brittle.


>  /* forward declarations */
>  static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
> @@ -388,7 +394,7 @@ nfsd4_cb_notify_lock_prepare(struct nfsd4_callback *cb)
>  static int
>  nfsd4_cb_notify_lock_done(struct nfsd4_callback *cb, struct rpc_task *task)
>  {
> -	trace_nfsd_cb_notify_lock_done(&zero_stateid, task);
> +	trace_nfsd_cb_notify_lock_done(&anon_stateid, task);
>  
>  	/*
>  	 * Since this is just an optimization, we don't try very hard if it
> @@ -6512,7 +6518,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>  	 * open stateid would have to be created.
>  	 */
>  	if (new_stp && open_xor_delegation(open)) {
> -		memcpy(&open->op_stateid, &zero_stateid, sizeof(open->op_stateid));
> +		memcpy(&open->op_stateid, &anon_stateid, sizeof(open->op_stateid));
>  		open->op_rflags |= OPEN4_RESULT_NO_OPEN_STATEID;
>  		release_open_stateid(stp);
>  	}
> @@ -7076,7 +7082,7 @@ __be32 nfs4_check_openmode(struct nfs4_ol_stateid *stp, int flags)
>  static inline __be32
>  check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid, int flags)
>  {
> -	if (ONE_STATEID(stateid) && (flags & RD_STATE))
> +	if (is_bypass_stateid(stateid) && (flags & RD_STATE))
>  		return nfs_ok;
>  	else if (opens_in_grace(net)) {
>  		/* Answer in remaining cases depends on existence of
> @@ -7085,7 +7091,7 @@ check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid,
>  	} else if (flags & WR_STATE)
>  		return nfs4_share_conflict(current_fh,
>  				NFS4_SHARE_DENY_WRITE);
> -	else /* (flags & RD_STATE) && ZERO_STATEID(stateid) */
> +	else /* (flags & RD_STATE) && is_anon_stateid(stateid) */
>  		return nfs4_share_conflict(current_fh,
>  				NFS4_SHARE_DENY_READ);
>  }
> @@ -7401,7 +7407,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  	if (nfp)
>  		*nfp = NULL;
>  
> -	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) {
> +	if (is_anon_stateid(stateid) || is_bypass_stateid(stateid)) {
>  		status = check_special_stateids(net, fhp, stateid, flags);
>  		goto done;
>  	}
> @@ -7823,12 +7829,12 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	/* v4.1+ suggests that we send a special stateid in here, since the
>  	 * clients should just ignore this anyway. Since this is not useful
> -	 * for v4.0 clients either, we set it to the special close_stateid
> +	 * for v4.0 clients either, we set it to the special invalid_stateid
>  	 * universally.
>  	 *
>  	 * See RFC5661 section 18.2.4, and RFC7530 section 16.2.5
>  	 */
> -	memcpy(&close->cl_stateid, &close_stateid, sizeof(close->cl_stateid));
> +	memcpy(&close->cl_stateid, &invalid_stateid, sizeof(close->cl_stateid));
>  
>  	/* put reference from nfs4_preprocess_seqid_op */
>  	nfs4_put_stid(&stp->st_stid);
> @@ -9101,7 +9107,7 @@ static void
>  get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>  {
>  	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG) &&
> -	    CURRENT_STATEID(stateid))
> +	    is_current_stateid(stateid))
>  		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
>  }
>  


-- 
Chuck Lever

Re: [PATCH v5 06/11] nfsd: revise names of special stateid, and predicate functions.

[NeilBrown]

On Thu, 20 Nov 2025, Chuck Lever wrote:
> On 11/18/25 10:28 PM, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > When I see "CURRENT_STATEID(foo)" in the code it is not clear that this
> > is testing the stateid to see if it is a special stateid.  I find that
> > IS_CURRENT_STATEID(foo) is clearer.  But an inline function is even
> > better, so is_current_stateid().
> > 
> > There are other special stateids which are described in RFC 8881 Section
> > 8.2.3 as "anonymous", "READ bypass", and "invalid".  The nfsd code
> > currently names them "zero", "one" and "close" which doesn't help with
> > comparing the code to the RFC.
> > 
> > So this patch changes the names of those special stateids and adds
> > "is_" to the front of the predicates.
> > 
> > As CLOSE_STATEID() was not needed, it is discarded rather than replacing
> > with is_invalid_stateid().
> > 
> > I felt that is_read_bypass_stateid() was a little too verbose, so I made
> > it is_bypass_stateid().
> > 
> > For consistency, invalid_stateid is changed to use ~0 rather than
> > 0xffffffffU for the generation number.  (RFC 8881 say to use
> > "NFS4_UINT32_MAX" for the generation number here, and "all ones" for the
> > generation and opaque of anon_stateid).
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++-----------------
> >  1 file changed, 23 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index ea931e606f40..f92b01bdb4dd 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -60,18 +60,18 @@
> >  #define NFSDDBG_FACILITY                NFSDDBG_PROC
> >  
> >  #define all_ones {{ ~0, ~0}, ~0}
> > -static const stateid_t one_stateid = {
> > +static const stateid_t read_bypass_stateid = {
> >  	.si_generation = ~0,
> >  	.si_opaque = all_ones,
> >  };
> > -static const stateid_t zero_stateid = {
> > +static const stateid_t anon_stateid = {
> >  	/* all fields zero */
> >  };
> > -static const stateid_t currentstateid = {
> > +static const stateid_t current_stateid = {
> >  	.si_generation = 1,
> >  };
> > -static const stateid_t close_stateid = {
> > -	.si_generation = 0xffffffffU,
> > +static const stateid_t invalid_stateid = {
> > +	.si_generation = ~0,
> >  };
> >  
> >  /*
> > @@ -93,10 +93,16 @@ static inline bool stateid_well_formed(stateid_t *stid)
> >  
> >  static u64 current_sessionid = 1;
> >  
> > -#define ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
> > -#define ONE_STATEID(stateid)  (!memcmp((stateid), &one_stateid, sizeof(stateid_t)))
> > -#define CURRENT_STATEID(stateid) (!memcmp((stateid), &currentstateid, sizeof(stateid_t)))
> > -#define CLOSE_STATEID(stateid)  (!memcmp((stateid), &close_stateid, sizeof(stateid_t)))
> > +/* These special stateid are defined in RFC 8881 Section 8.2.3 */
> > +static inline bool is_anon_stateid(stateid_t *stateid) {
> > +	return memcmp(stateid, &anon_stateid, sizeof(stateid_t));
> > +}
> > +static inline bool is_bypass_stateid(stateid_t *stateid) {
> > +	return memcmp(stateid, &read_bypass_stateid, sizeof(stateid_t));
> > +}
> > +static inline bool is_current_stateid(stateid_t *stateid) {
> > +	return memcmp(stateid, &current_stateid, sizeof(stateid_t));
> > +}
> 
> The new static inline functions appear to invert the logic -- the macros
> use "!memcmp" but the new functions omit the "!". memcmp() returns an
> int, so there is an implicit type conversion here as well. So maybe you
> want "memcmp(stateid, ... ) == 0" ?

I must have been asleep ?  Thanks.
(I really don't like "!memcmp" or "!strcmp" so I definitely intended to
change it.  I have now).

> 
> And now we can use "sizeof(*stateid)" here which is slightly less
> brittle.

Good idea.

Thanks,
NeilBrown

[PATCH v5 07/11] nfsd: simplify clearing of current-state-id

[NeilBrown]

From: NeilBrown <neil@brown.name>

In NFSv4.1 the current and saved filehandle each have a corresponding
stateid.  RFC 8881 requires that in certain circumstances - particularly
when the current filehandle is changed - the current stateid should be
set to the anonymous stateid (all zeros).  It also requires that when a
request tries to use the current stateid, if that stateid is "special"
(which includes the anonymous stateid), then a BAD_STATEID error must
be produced.

Linux nfsd current implements these requirements using a flag to record
if the stateid (current or saved) is valid rather than actually storing
the anon stateid when invalid.  This has the same net effect.

The aim of this patch is to simplify this code and particularly to
remove the per-op OP_CLEAR_STATEID flag which is unnecessary.

The "is valid" flag is moved from 'struct nfsd4_compound_state' to
'struct svc_fh' which already has a number of flags related to the
filehandle.  This flag will only ever be set for the v4 current_fh and
saved_fh and records if the corresponding stateid is valid.

fh_put() is changed to clear this flag.  As this is called on current_fh
in each op that currently has OP_CLEAR_STATEID set, this automatically
clears the stored stateid when required so OP_CLEAR_STATEID is no longer
needed.

As fh_dup2() already copies all of the svc_fh, it now copies the new
fh_have_stateid flag as well, so savefh and restorefh are simplified.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/current_stateid.h |  1 -
 fs/nfsd/nfs4proc.c        | 25 ++++++++-----------------
 fs/nfsd/nfs4state.c       | 10 ++--------
 fs/nfsd/nfsfh.h           |  1 +
 fs/nfsd/xdr4.h            | 13 -------------
 5 files changed, 11 insertions(+), 39 deletions(-)

diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
index c28540d86742..24d769043207 100644
--- a/fs/nfsd/current_stateid.h
+++ b/fs/nfsd/current_stateid.h
@@ -5,7 +5,6 @@
 #include "state.h"
 #include "xdr4.h"
 
-extern void clear_current_stateid(struct nfsd4_compound_state *cstate);
 /*
  * functions to set current state id
  */
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index e61b1ee6c8d8..ae4094ff12dc 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -737,10 +737,7 @@ nfsd4_restorefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		return nfserr_restorefh;
 
 	fh_dup2(&cstate->current_fh, &cstate->save_fh);
-	if (HAS_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG)) {
-		memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
-		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
-	}
+	memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
 	return nfs_ok;
 }
 
@@ -757,10 +754,7 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		return nfserr_nofilehandle;
 
 	fh_dup2(&cstate->save_fh, &cstate->current_fh);
-	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
-		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
-		SET_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG);
-	}
+	memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
 	return nfs_ok;
 }
 
@@ -2954,9 +2948,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 			if (op->opdesc->op_set_currentstateid)
 				op->opdesc->op_set_currentstateid(cstate, &op->u);
 
-			if (op->opdesc->op_flags & OP_CLEAR_STATEID)
-				clear_current_stateid(cstate);
-
 			if (current_fh->fh_export &&
 					need_wrongsec_check(rqstp))
 				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
@@ -3401,7 +3392,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_CREATE] = {
 		.op_func = nfsd4_create,
-		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME | OP_CLEAR_STATEID,
+		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
 		.op_name = "OP_CREATE",
 		.op_rsize_bop = nfsd4_create_rsize,
 	},
@@ -3455,13 +3446,13 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_LOOKUP] = {
 		.op_func = nfsd4_lookup,
-		.op_flags = OP_HANDLES_WRONGSEC | OP_CLEAR_STATEID,
+		.op_flags = OP_HANDLES_WRONGSEC,
 		.op_name = "OP_LOOKUP",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_LOOKUPP] = {
 		.op_func = nfsd4_lookupp,
-		.op_flags = OP_HANDLES_WRONGSEC | OP_CLEAR_STATEID,
+		.op_flags = OP_HANDLES_WRONGSEC,
 		.op_name = "OP_LOOKUPP",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
@@ -3494,21 +3485,21 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	[OP_PUTFH] = {
 		.op_func = nfsd4_putfh,
 		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
-				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
+				| OP_IS_PUTFH_LIKE,
 		.op_name = "OP_PUTFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_PUTPUBFH] = {
 		.op_func = nfsd4_putrootfh,
 		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
-				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
+				| OP_IS_PUTFH_LIKE,
 		.op_name = "OP_PUTPUBFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_PUTROOTFH] = {
 		.op_func = nfsd4_putrootfh,
 		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
-				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
+				| OP_IS_PUTFH_LIKE,
 		.op_name = "OP_PUTROOTFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f92b01bdb4dd..7ffe0b8e44de 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -9106,7 +9106,7 @@ nfs4_state_shutdown(void)
 static void
 get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
 {
-	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG) &&
+	if (cstate->current_fh.fh_have_stateid &&
 	    is_current_stateid(stateid))
 		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
 }
@@ -9116,16 +9116,10 @@ put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
 {
 	if (cstate->minorversion) {
 		memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
-		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
+		cstate->current_fh.fh_have_stateid = true;
 	}
 }
 
-void
-clear_current_stateid(struct nfsd4_compound_state *cstate)
-{
-	CLEAR_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
-}
-
 /*
  * functions to set current state id
  */
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 43fcc1dcf69a..0142227c90e6 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -93,6 +93,7 @@ typedef struct svc_fh {
 						 */
 	bool			fh_use_wgather;	/* NFSv2 wgather option */
 	bool			fh_64bit_cookies;/* readdir cookie size */
+	bool			fh_have_stateid; /* associated v4.1 stateid is not special */
 	bool			fh_post_saved;	/* post-op attrs saved */
 	bool			fh_pre_saved;	/* pre-op attrs saved */
 
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index bcaf631ec12d..b6ab32c7b243 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -43,13 +43,6 @@
 #define NFSD4_MAX_TAGLEN	128
 #define XDR_LEN(n)                     (((n) + 3) & ~3)
 
-#define CURRENT_STATE_ID_FLAG (1<<0)
-#define SAVED_STATE_ID_FLAG (1<<1)
-
-#define SET_CSTATE_FLAG(c, f) ((c)->sid_flags |= (f))
-#define HAS_CSTATE_FLAG(c, f) ((c)->sid_flags & (f))
-#define CLEAR_CSTATE_FLAG(c, f) ((c)->sid_flags &= ~(f))
-
 /**
  * nfsd4_encode_bool - Encode an XDR bool type result
  * @xdr: target XDR stream
@@ -194,8 +187,6 @@ struct nfsd4_compound_state {
 	__be32			saved_status;
 	stateid_t	current_stateid;
 	stateid_t	save_stateid;
-	/* to indicate current and saved state id presents */
-	u32		sid_flags;
 };
 
 static inline bool nfsd4_has_session(struct nfsd4_compound_state *cs)
@@ -1030,10 +1021,6 @@ enum nfsd4_op_flags {
 	 * the v4.0 case).
 	 */
 	OP_CACHEME = 1 << 6,
-	/*
-	 * These are ops which clear current state id.
-	 */
-	OP_CLEAR_STATEID = 1 << 7,
 	/* Most ops return only an error on failure; some may do more: */
 	OP_NONTRIVIAL_ERROR_ENCODE = 1 << 8,
 };
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v5 07/11] nfsd: simplify clearing of current-state-id

[Chuck Lever]

On 11/18/25 10:28 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> In NFSv4.1 the current and saved filehandle each have a corresponding
> stateid.  RFC 8881 requires that in certain circumstances - particularly
> when the current filehandle is changed - the current stateid should be
> set to the anonymous stateid (all zeros).  It also requires that when a
> request tries to use the current stateid, if that stateid is "special"
> (which includes the anonymous stateid), then a BAD_STATEID error must
> be produced.
> 
> Linux nfsd current implements these requirements using a flag to record
> if the stateid (current or saved) is valid rather than actually storing
> the anon stateid when invalid.  This has the same net effect.
> 
> The aim of this patch is to simplify this code and particularly to
> remove the per-op OP_CLEAR_STATEID flag which is unnecessary.
> 
> The "is valid" flag is moved from 'struct nfsd4_compound_state' to
> 'struct svc_fh' which already has a number of flags related to the
> filehandle.  This flag will only ever be set for the v4 current_fh and
> saved_fh and records if the corresponding stateid is valid.
> 
> fh_put() is changed to clear this flag.

I don't see a hunk in the patch that implements this.


> As this is called on current_fh
> in each op that currently has OP_CLEAR_STATEID set, this automatically
> clears the stored stateid when required so OP_CLEAR_STATEID is no longer
> needed.
> 
> As fh_dup2() already copies all of the svc_fh, it now copies the new
> fh_have_stateid flag as well, so savefh and restorefh are simplified.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/current_stateid.h |  1 -
>  fs/nfsd/nfs4proc.c        | 25 ++++++++-----------------
>  fs/nfsd/nfs4state.c       | 10 ++--------
>  fs/nfsd/nfsfh.h           |  1 +
>  fs/nfsd/xdr4.h            | 13 -------------
>  5 files changed, 11 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
> index c28540d86742..24d769043207 100644
> --- a/fs/nfsd/current_stateid.h
> +++ b/fs/nfsd/current_stateid.h
> @@ -5,7 +5,6 @@
>  #include "state.h"
>  #include "xdr4.h"
>  
> -extern void clear_current_stateid(struct nfsd4_compound_state *cstate);
>  /*
>   * functions to set current state id
>   */
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e61b1ee6c8d8..ae4094ff12dc 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -737,10 +737,7 @@ nfsd4_restorefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		return nfserr_restorefh;
>  
>  	fh_dup2(&cstate->current_fh, &cstate->save_fh);
> -	if (HAS_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG)) {
> -		memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
> -		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
> -	}
> +	memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
>  	return nfs_ok;
>  }
>  
> @@ -757,10 +754,7 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		return nfserr_nofilehandle;
>  
>  	fh_dup2(&cstate->save_fh, &cstate->current_fh);
> -	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
> -		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
> -		SET_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG);
> -	}
> +	memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
>  	return nfs_ok;
>  }
>  
> @@ -2954,9 +2948,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  			if (op->opdesc->op_set_currentstateid)
>  				op->opdesc->op_set_currentstateid(cstate, &op->u);
>  
> -			if (op->opdesc->op_flags & OP_CLEAR_STATEID)
> -				clear_current_stateid(cstate);
> -
>  			if (current_fh->fh_export &&
>  					need_wrongsec_check(rqstp))
>  				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
> @@ -3401,7 +3392,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_CREATE] = {
>  		.op_func = nfsd4_create,
> -		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME | OP_CLEAR_STATEID,
> +		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
>  		.op_name = "OP_CREATE",
>  		.op_rsize_bop = nfsd4_create_rsize,
>  	},
> @@ -3455,13 +3446,13 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_LOOKUP] = {
>  		.op_func = nfsd4_lookup,
> -		.op_flags = OP_HANDLES_WRONGSEC | OP_CLEAR_STATEID,
> +		.op_flags = OP_HANDLES_WRONGSEC,
>  		.op_name = "OP_LOOKUP",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_LOOKUPP] = {
>  		.op_func = nfsd4_lookupp,
> -		.op_flags = OP_HANDLES_WRONGSEC | OP_CLEAR_STATEID,
> +		.op_flags = OP_HANDLES_WRONGSEC,
>  		.op_name = "OP_LOOKUPP",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
> @@ -3494,21 +3485,21 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	[OP_PUTFH] = {
>  		.op_func = nfsd4_putfh,
>  		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
> -				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
> +				| OP_IS_PUTFH_LIKE,
>  		.op_name = "OP_PUTFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_PUTPUBFH] = {
>  		.op_func = nfsd4_putrootfh,
>  		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
> -				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
> +				| OP_IS_PUTFH_LIKE,
>  		.op_name = "OP_PUTPUBFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_PUTROOTFH] = {
>  		.op_func = nfsd4_putrootfh,
>  		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
> -				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
> +				| OP_IS_PUTFH_LIKE,
>  		.op_name = "OP_PUTROOTFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f92b01bdb4dd..7ffe0b8e44de 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -9106,7 +9106,7 @@ nfs4_state_shutdown(void)
>  static void
>  get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>  {
> -	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG) &&
> +	if (cstate->current_fh.fh_have_stateid &&
>  	    is_current_stateid(stateid))
>  		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
>  }
> @@ -9116,16 +9116,10 @@ put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>  {
>  	if (cstate->minorversion) {
>  		memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
> -		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
> +		cstate->current_fh.fh_have_stateid = true;
>  	}
>  }
>  
> -void
> -clear_current_stateid(struct nfsd4_compound_state *cstate)
> -{
> -	CLEAR_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
> -}
> -
>  /*
>   * functions to set current state id
>   */
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 43fcc1dcf69a..0142227c90e6 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -93,6 +93,7 @@ typedef struct svc_fh {
>  						 */
>  	bool			fh_use_wgather;	/* NFSv2 wgather option */
>  	bool			fh_64bit_cookies;/* readdir cookie size */
> +	bool			fh_have_stateid; /* associated v4.1 stateid is not special */
>  	bool			fh_post_saved;	/* post-op attrs saved */
>  	bool			fh_pre_saved;	/* pre-op attrs saved */
>  
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index bcaf631ec12d..b6ab32c7b243 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -43,13 +43,6 @@
>  #define NFSD4_MAX_TAGLEN	128
>  #define XDR_LEN(n)                     (((n) + 3) & ~3)
>  
> -#define CURRENT_STATE_ID_FLAG (1<<0)
> -#define SAVED_STATE_ID_FLAG (1<<1)
> -
> -#define SET_CSTATE_FLAG(c, f) ((c)->sid_flags |= (f))
> -#define HAS_CSTATE_FLAG(c, f) ((c)->sid_flags & (f))
> -#define CLEAR_CSTATE_FLAG(c, f) ((c)->sid_flags &= ~(f))
> -
>  /**
>   * nfsd4_encode_bool - Encode an XDR bool type result
>   * @xdr: target XDR stream
> @@ -194,8 +187,6 @@ struct nfsd4_compound_state {
>  	__be32			saved_status;
>  	stateid_t	current_stateid;
>  	stateid_t	save_stateid;
> -	/* to indicate current and saved state id presents */
> -	u32		sid_flags;
>  };
>  
>  static inline bool nfsd4_has_session(struct nfsd4_compound_state *cs)
> @@ -1030,10 +1021,6 @@ enum nfsd4_op_flags {
>  	 * the v4.0 case).
>  	 */
>  	OP_CACHEME = 1 << 6,
> -	/*
> -	 * These are ops which clear current state id.
> -	 */
> -	OP_CLEAR_STATEID = 1 << 7,
>  	/* Most ops return only an error on failure; some may do more: */
>  	OP_NONTRIVIAL_ERROR_ENCODE = 1 << 8,
>  };


-- 
Chuck Lever

Re: [PATCH v5 07/11] nfsd: simplify clearing of current-state-id

[NeilBrown]

On Thu, 20 Nov 2025, Chuck Lever wrote:
> On 11/18/25 10:28 PM, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > In NFSv4.1 the current and saved filehandle each have a corresponding
> > stateid.  RFC 8881 requires that in certain circumstances - particularly
> > when the current filehandle is changed - the current stateid should be
> > set to the anonymous stateid (all zeros).  It also requires that when a
> > request tries to use the current stateid, if that stateid is "special"
> > (which includes the anonymous stateid), then a BAD_STATEID error must
> > be produced.
> > 
> > Linux nfsd current implements these requirements using a flag to record
> > if the stateid (current or saved) is valid rather than actually storing
> > the anon stateid when invalid.  This has the same net effect.
> > 
> > The aim of this patch is to simplify this code and particularly to
> > remove the per-op OP_CLEAR_STATEID flag which is unnecessary.
> > 
> > The "is valid" flag is moved from 'struct nfsd4_compound_state' to
> > 'struct svc_fh' which already has a number of flags related to the
> > filehandle.  This flag will only ever be set for the v4 current_fh and
> > saved_fh and records if the corresponding stateid is valid.
> > 
> > fh_put() is changed to clear this flag.
> 
> I don't see a hunk in the patch that implements this.

Nor do I.  I don't see it in my "git reflog" either.
This is why I like the commit message to give plenty of detail, it
helps reviewers find my mistakes.

Thanks!
NeilBrown

[PATCH v5 08/11] nfsd: simplify use of the current stateid.

[NeilBrown]

From: NeilBrown <neil@brown.name>

In NFSv4.1 any use of a stateid in an OP should check if it is
the special stateid that
   has "other" set to zero and "seqid" set to one

If it is, then the stateid saved from a previous operation should be
used unless that stateid as "special" stateid, in which case BAD_STATEID
must be returned.

Most ops that include a stateid pass it (indirectly) to
nfsd4_lookup_stateid().  The one exception is FREE_STATEID which uses
find_stateid_locked() directly.  (TEST_STATEID is not considered.  It is
given a list of stateids, among which any special stateid is explicitly
forbidden).

So rather than providing dedicated op_get_currentstateid functions for
each relevant op, we can include the required logic directly in
nfsd4_lookup_stateid() and nfsd4_free_stateid().  That is what this
patch does.

We also include an explicit return of nfserr_bad_stateid when the
special stateid is used, but no valid stateid is available.  This does
not change behaviour as the error would have been returned eventually.

This patch also removes the "minorversion" test in put_stateid() and
instead checks that sessions are in use when the stateid is used.  This
seems more natural.  Note that nfsd4_free_stateid() doesn't need this
explicit test as the function cannot be called for v4.0.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/current_stateid.h | 21 ----------
 fs/nfsd/nfs4proc.c        | 11 -----
 fs/nfsd/nfs4state.c       | 85 ++++++---------------------------------
 fs/nfsd/xdr4.h            |  2 -
 4 files changed, 13 insertions(+), 106 deletions(-)

diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
index 24d769043207..eaa4b8e40dc8 100644
--- a/fs/nfsd/current_stateid.h
+++ b/fs/nfsd/current_stateid.h
@@ -16,25 +16,4 @@ extern void nfsd4_set_lockstateid(struct nfsd4_compound_state *,
 		union nfsd4_op_u *);
 extern void nfsd4_set_closestateid(struct nfsd4_compound_state *,
 		union nfsd4_op_u *);
-
-/*
- * functions to consume current state id
- */
-extern void nfsd4_get_opendowngradestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_delegreturnstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_freestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_setattrstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_closestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_lockustateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_readstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_writestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-
 #endif   /* _NFSD4_CURRENT_STATE_H */
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ae4094ff12dc..ca1ed0a1bcab 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2933,8 +2933,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 		if (op->status)
 			goto encode_op;
 
-		if (op->opdesc->op_get_currentstateid)
-			op->opdesc->op_get_currentstateid(cstate, &op->u);
 		op->status = op->opdesc->op_func(rqstp, cstate, &op->u);
 		trace_nfsd_compound_op_err(rqstp, op->opnum, op->status);
 
@@ -3381,7 +3379,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_CLOSE",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_get_currentstateid = nfsd4_get_closestateid,
 		.op_set_currentstateid = nfsd4_set_closestateid,
 	},
 	[OP_COMMIT] = {
@@ -3401,7 +3398,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_DELEGRETURN",
 		.op_rsize_bop = nfsd4_only_status_rsize,
-		.op_get_currentstateid = nfsd4_get_delegreturnstateid,
 	},
 	[OP_GETATTR] = {
 		.op_func = nfsd4_getattr,
@@ -3442,7 +3438,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_LOCKU",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_get_currentstateid = nfsd4_get_lockustateid,
 	},
 	[OP_LOOKUP] = {
 		.op_func = nfsd4_lookup,
@@ -3479,7 +3474,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_OPEN_DOWNGRADE",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_get_currentstateid = nfsd4_get_opendowngradestateid,
 		.op_set_currentstateid = nfsd4_set_opendowngradestateid,
 	},
 	[OP_PUTFH] = {
@@ -3508,7 +3502,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_release = nfsd4_read_release,
 		.op_name = "OP_READ",
 		.op_rsize_bop = nfsd4_read_rsize,
-		.op_get_currentstateid = nfsd4_get_readstateid,
 	},
 	[OP_READDIR] = {
 		.op_func = nfsd4_readdir,
@@ -3567,7 +3560,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME
 				| OP_NONTRIVIAL_ERROR_ENCODE,
 		.op_rsize_bop = nfsd4_setattr_rsize,
-		.op_get_currentstateid = nfsd4_get_setattrstateid,
 	},
 	[OP_SETCLIENTID] = {
 		.op_func = nfsd4_setclientid,
@@ -3594,7 +3586,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
 		.op_name = "OP_WRITE",
 		.op_rsize_bop = nfsd4_write_rsize,
-		.op_get_currentstateid = nfsd4_get_writestateid,
 	},
 	[OP_RELEASE_LOCKOWNER] = {
 		.op_func = nfsd4_release_lockowner,
@@ -3676,7 +3667,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_func = nfsd4_free_stateid,
 		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_FREE_STATEID",
-		.op_get_currentstateid = nfsd4_get_freestateid,
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_GET_DIR_DELEGATION] = {
@@ -3744,7 +3734,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_release = nfsd4_read_release,
 		.op_name = "OP_READ_PLUS",
 		.op_rsize_bop = nfsd4_read_plus_rsize,
-		.op_get_currentstateid = nfsd4_get_readstateid,
 	},
 	[OP_SEEK] = {
 		.op_func = nfsd4_seek,
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7ffe0b8e44de..ce7b958fa076 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7192,6 +7192,11 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 	struct nfs4_stid *stid;
 	bool return_revoked = false;
 
+	if (nfsd4_has_session(cstate) && is_current_stateid(stateid)) {
+		if (!cstate->current_fh.fh_have_stateid)
+			return nfserr_bad_stateid;
+		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
+	}
 	/*
 	 *  only return revoked delegations if explicitly asked.
 	 *  otherwise we report revoked or bad_stateid status.
@@ -7508,6 +7513,12 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	struct nfs4_client *cl = cstate->clp;
 	__be32 ret = nfserr_bad_stateid;
 
+	if (is_current_stateid(stateid)) {
+		if (!cstate->current_fh.fh_have_stateid)
+			return nfserr_bad_stateid;
+		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
+	}
+
 	spin_lock(&cl->cl_lock);
 	s = find_stateid_locked(cl, stateid);
 	if (!s || s->sc_status & SC_STATUS_CLOSED)
@@ -9103,21 +9114,11 @@ nfs4_state_shutdown(void)
 	shrinker_free(nfsd_slot_shrinker);
 }
 
-static void
-get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
-{
-	if (cstate->current_fh.fh_have_stateid &&
-	    is_current_stateid(stateid))
-		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
-}
-
 static void
 put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
 {
-	if (cstate->minorversion) {
-		memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
-		cstate->current_fh.fh_have_stateid = true;
-	}
+	memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
+	cstate->current_fh.fh_have_stateid = true;
 }
 
 /*
@@ -9151,66 +9152,6 @@ nfsd4_set_lockstateid(struct nfsd4_compound_state *cstate,
 	put_stateid(cstate, &u->lock.lk_resp_stateid);
 }
 
-/*
- * functions to consume current state id
- */
-
-void
-nfsd4_get_opendowngradestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->open_downgrade.od_stateid);
-}
-
-void
-nfsd4_get_delegreturnstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->delegreturn.dr_stateid);
-}
-
-void
-nfsd4_get_freestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->free_stateid.fr_stateid);
-}
-
-void
-nfsd4_get_setattrstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->setattr.sa_stateid);
-}
-
-void
-nfsd4_get_closestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->close.cl_stateid);
-}
-
-void
-nfsd4_get_lockustateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->locku.lu_stateid);
-}
-
-void
-nfsd4_get_readstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->read.rd_stateid);
-}
-
-void
-nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->write.wr_stateid);
-}
-
 /**
  * nfsd4_vet_deleg_time - vet and set the timespec for a delegated timestamp update
  * @req: timestamp from the client
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index b6ab32c7b243..55dab88b011c 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -1034,8 +1034,6 @@ struct nfsd4_operation {
 	/* Try to get response size before operation */
 	u32 (*op_rsize_bop)(const struct svc_rqst *rqstp,
 			const struct nfsd4_op *op);
-	void (*op_get_currentstateid)(struct nfsd4_compound_state *,
-			union nfsd4_op_u *);
 	void (*op_set_currentstateid)(struct nfsd4_compound_state *,
 			union nfsd4_op_u *);
 };
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 09/11] nfsd: simplify saving of the current stateid

[NeilBrown]

From: NeilBrown <neil@brown.name>

OPs that return a stateid must update the current stateid stored in the
COMPOUND state so it can be used in later OPs.

Every OP that returns a stateid uses nfs4_inc_and_copy_stateid() to
provide the stateid to return.

This patch updates that function to store the resulting stateid in
cstate->current_stateid, and passes cstate (the COMPOUND state) through
to the function in all cases except when used in a callback.

This removes the need for op_set_currentstateid.

This also sets the current stateid in more cases, such as LOCKU and
LAYOUTGET.

One case that this does not cover is CLOSE.  CLOSE always reports a
special (invalid) stateid, so it is changed to simply mark the current
stateid as not valid.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/current_stateid.h | 11 -------
 fs/nfsd/nfs4layouts.c     | 10 ++++---
 fs/nfsd/nfs4proc.c        | 11 ++-----
 fs/nfsd/nfs4state.c       | 62 ++++++++++-----------------------------
 fs/nfsd/pnfs.h            |  5 ++--
 fs/nfsd/state.h           |  3 +-
 fs/nfsd/xdr4.h            |  4 ++-
 7 files changed, 32 insertions(+), 74 deletions(-)

diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
index eaa4b8e40dc8..9dce3004b846 100644
--- a/fs/nfsd/current_stateid.h
+++ b/fs/nfsd/current_stateid.h
@@ -5,15 +5,4 @@
 #include "state.h"
 #include "xdr4.h"
 
-/*
- * functions to set current state id
- */
-extern void nfsd4_set_opendowngradestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_set_openstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_set_lockstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_set_closestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
 #endif   /* _NFSD4_CURRENT_STATE_H */
diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 683bd1130afe..4519a01bb354 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -414,7 +414,8 @@ nfsd4_recall_conflict(struct nfs4_layout_stateid *ls)
 }
 
 __be32
-nfsd4_insert_layout(struct nfsd4_layoutget *lgp, struct nfs4_layout_stateid *ls)
+nfsd4_insert_layout(struct nfsd4_compound_state *cstate,
+		    struct nfsd4_layoutget *lgp, struct nfs4_layout_stateid *ls)
 {
 	struct nfsd4_layout_seg *seg = &lgp->lg_seg;
 	struct nfs4_file *fp = ls->ls_stid.sc_file;
@@ -453,7 +454,7 @@ nfsd4_insert_layout(struct nfsd4_layoutget *lgp, struct nfs4_layout_stateid *ls)
 	list_add_tail(&new->lo_perstate, &ls->ls_layouts);
 	new = NULL;
 done:
-	nfs4_inc_and_copy_stateid(&lgp->lg_sid, &ls->ls_stid);
+	nfs4_inc_and_copy_stateid(cstate, &lgp->lg_sid, &ls->ls_stid);
 	spin_unlock(&ls->ls_lock);
 out:
 	spin_unlock(&fp->fi_lock);
@@ -528,7 +529,8 @@ nfsd4_return_file_layouts(struct svc_rqst *rqstp,
 	}
 	if (!list_empty(&ls->ls_layouts)) {
 		if (found)
-			nfs4_inc_and_copy_stateid(&lrp->lr_sid, &ls->ls_stid);
+			nfs4_inc_and_copy_stateid(cstate, &lrp->lr_sid,
+						  &ls->ls_stid);
 		lrp->lrs_present = true;
 	} else {
 		trace_nfsd_layoutstate_unhash(&ls->ls_stid.sc_stateid);
@@ -659,7 +661,7 @@ nfsd4_cb_layout_prepare(struct nfsd4_callback *cb)
 		container_of(cb, struct nfs4_layout_stateid, ls_recall);
 
 	mutex_lock(&ls->ls_mutex);
-	nfs4_inc_and_copy_stateid(&ls->ls_recall_sid, &ls->ls_stid);
+	nfs4_inc_and_copy_stateid(NULL, &ls->ls_recall_sid, &ls->ls_stid);
 	mutex_unlock(&ls->ls_mutex);
 }
 
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ca1ed0a1bcab..eaa6a8c25ed6 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -630,7 +630,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
-	status = nfsd4_process_open2(rqstp, resfh, open);
+	status = nfsd4_process_open2(rqstp, cstate, resfh, open);
 	if (status && open->op_created)
 		pr_warn("nfsd4_process_open2 failed to open newly-created file: status=%u\n",
 			be32_to_cpu(status));
@@ -2526,7 +2526,7 @@ nfsd4_layoutget(struct svc_rqst *rqstp,
 	if (nfserr)
 		goto out_put_stid;
 
-	nfserr = nfsd4_insert_layout(lgp, ls);
+	nfserr = nfsd4_insert_layout(cstate, lgp, ls);
 
 out_put_stid:
 	mutex_unlock(&ls->ls_mutex);
@@ -2943,9 +2943,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 			goto out;
 		}
 		if (!op->status) {
-			if (op->opdesc->op_set_currentstateid)
-				op->opdesc->op_set_currentstateid(cstate, &op->u);
-
 			if (current_fh->fh_export &&
 					need_wrongsec_check(rqstp))
 				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
@@ -3379,7 +3376,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_CLOSE",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_set_currentstateid = nfsd4_set_closestateid,
 	},
 	[OP_COMMIT] = {
 		.op_func = nfsd4_commit,
@@ -3424,7 +3420,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 				OP_NONTRIVIAL_ERROR_ENCODE,
 		.op_name = "OP_LOCK",
 		.op_rsize_bop = nfsd4_lock_rsize,
-		.op_set_currentstateid = nfsd4_set_lockstateid,
 	},
 	[OP_LOCKT] = {
 		.op_func = nfsd4_lockt,
@@ -3461,7 +3456,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_OPEN",
 		.op_rsize_bop = nfsd4_open_rsize,
-		.op_set_currentstateid = nfsd4_set_openstateid,
 	},
 	[OP_OPEN_CONFIRM] = {
 		.op_func = nfsd4_open_confirm,
@@ -3474,7 +3468,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_OPEN_DOWNGRADE",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_set_currentstateid = nfsd4_set_opendowngradestateid,
 	},
 	[OP_PUTFH] = {
 		.op_func = nfsd4_putfh,
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ce7b958fa076..f01c72876ca9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1216,7 +1216,8 @@ nfs4_put_stid(struct nfs4_stid *s)
 }
 
 void
-nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid)
+nfs4_inc_and_copy_stateid(struct nfsd4_compound_state *cstate,
+			  stateid_t *dst, struct nfs4_stid *stid)
 {
 	stateid_t *src = &stid->sc_stateid;
 
@@ -1225,6 +1226,10 @@ nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid)
 		src->si_generation = 1;
 	memcpy(dst, src, sizeof(*dst));
 	spin_unlock(&stid->sc_lock);
+	if (cstate) {
+		memcpy(&cstate->current_stateid, dst, sizeof(stateid_t));
+		cstate->current_fh.fh_have_stateid = true;
+	}
 }
 
 static void put_deleg_file(struct nfs4_file *fp)
@@ -6402,6 +6407,7 @@ static bool open_xor_delegation(struct nfsd4_open *open)
 /**
  * nfsd4_process_open2 - finish open processing
  * @rqstp: the RPC transaction being executed
+ * @cstate: the nfsd4_compound_state for the current COMPOUND.
  * @current_fh: NFSv4 COMPOUND's current filehandle
  * @open: OPEN arguments
  *
@@ -6412,7 +6418,8 @@ static bool open_xor_delegation(struct nfsd4_open *open)
  * network byte order is returned.
  */
 __be32
-nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_open *open)
+nfsd4_process_open2(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
+		    struct svc_fh *current_fh, struct nfsd4_open *open)
 {
 	struct nfsd4_compoundres *resp = rqstp->rq_resp;
 	struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
@@ -6494,7 +6501,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 			open->op_odstate = NULL;
 	}
 
-	nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &open->op_stateid, &stp->st_stid);
 	mutex_unlock(&stp->st_mutex);
 
 	if (nfsd4_has_session(&resp->cstate)) {
@@ -7689,7 +7696,7 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto put_stateid;
 	}
 	oo->oo_flags |= NFS4_OO_CONFIRMED;
-	nfs4_inc_and_copy_stateid(&oc->oc_resp_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &oc->oc_resp_stateid, &stp->st_stid);
 	mutex_unlock(&stp->st_mutex);
 	trace_nfsd_open_confirm(oc->oc_seqid, &stp->st_stid.sc_stateid);
 	nfsd4_client_record_create(oo->oo_owner.so_client);
@@ -7761,7 +7768,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
 	}
 	nfs4_stateid_downgrade(stp, od->od_share_access);
 	reset_union_bmap_deny(od->od_share_deny, stp);
-	nfs4_inc_and_copy_stateid(&od->od_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &od->od_stateid, &stp->st_stid);
 	status = nfs_ok;
 put_stateid:
 	mutex_unlock(&stp->st_mutex);
@@ -7831,7 +7838,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * copied value below, but we continue to do so here just to ensure
 	 * that racing ops see that there was a state change.
 	 */
-	nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &close->cl_stateid, &stp->st_stid);
 
 	need_move_to_close_list = nfsd4_close_open_stateid(stp);
 	mutex_unlock(&stp->st_mutex);
@@ -7846,6 +7853,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * See RFC5661 section 18.2.4, and RFC7530 section 16.2.5
 	 */
 	memcpy(&close->cl_stateid, &invalid_stateid, sizeof(close->cl_stateid));
+	cstate->current_fh.fh_have_stateid = false;
 
 	/* put reference from nfs4_preprocess_seqid_op */
 	nfs4_put_stid(&stp->st_stid);
@@ -8420,7 +8428,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	err = vfs_lock_file(nf->nf_file, F_SETLK, file_lock, conflock);
 	switch (err) {
 	case 0: /* success! */
-		nfs4_inc_and_copy_stateid(&lock->lk_resp_stateid, &lock_stp->st_stid);
+		nfs4_inc_and_copy_stateid(cstate, &lock->lk_resp_stateid, &lock_stp->st_stid);
 		status = 0;
 		if (lock->lk_reclaim)
 			nn->somebody_reclaimed = true;
@@ -8665,7 +8673,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		dprintk("NFSD: nfs4_locku: vfs_lock_file failed!\n");
 		goto out_nfserr;
 	}
-	nfs4_inc_and_copy_stateid(&locku->lu_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &locku->lu_stateid, &stp->st_stid);
 put_file:
 	nfsd_file_put(nf);
 put_stateid:
@@ -9114,44 +9122,6 @@ nfs4_state_shutdown(void)
 	shrinker_free(nfsd_slot_shrinker);
 }
 
-static void
-put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
-{
-	memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
-	cstate->current_fh.fh_have_stateid = true;
-}
-
-/*
- * functions to set current state id
- */
-void
-nfsd4_set_opendowngradestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	put_stateid(cstate, &u->open_downgrade.od_stateid);
-}
-
-void
-nfsd4_set_openstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	put_stateid(cstate, &u->open.op_stateid);
-}
-
-void
-nfsd4_set_closestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	put_stateid(cstate, &u->close.cl_stateid);
-}
-
-void
-nfsd4_set_lockstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	put_stateid(cstate, &u->lock.lk_resp_stateid);
-}
-
 /**
  * nfsd4_vet_deleg_time - vet and set the timespec for a delegated timestamp update
  * @req: timestamp from the client
diff --git a/fs/nfsd/pnfs.h b/fs/nfsd/pnfs.h
index db9af780438b..1714471d173d 100644
--- a/fs/nfsd/pnfs.h
+++ b/fs/nfsd/pnfs.h
@@ -56,8 +56,9 @@ extern const struct nfsd4_layout_ops ff_layout_ops;
 __be32 nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *cstate, stateid_t *stateid,
 		bool create, u32 layout_type, struct nfs4_layout_stateid **lsp);
-__be32 nfsd4_insert_layout(struct nfsd4_layoutget *lgp,
-		struct nfs4_layout_stateid *ls);
+__be32 nfsd4_insert_layout(struct nfsd4_compound_state *cstate,
+			   struct nfsd4_layoutget *lgp,
+			   struct nfs4_layout_stateid *ls);
 __be32 nfsd4_return_file_layouts(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *cstate,
 		struct nfsd4_layoutreturn *lrp);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 1e736f402426..c6e97d6daa5f 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -797,7 +797,8 @@ void nfs4_free_copy_state(struct nfsd4_copy *copy);
 struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
 			struct nfs4_stid *p_stid);
 void nfs4_put_stid(struct nfs4_stid *s);
-void nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid);
+void nfs4_inc_and_copy_stateid(struct nfsd4_compound_state *cstate,
+			       stateid_t *dst, struct nfs4_stid *stid);
 void nfs4_remove_reclaim_record(struct nfs4_client_reclaim *, struct nfsd_net *);
 extern void nfs4_release_reclaim(struct nfsd_net *);
 extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(struct xdr_netobj name,
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 55dab88b011c..1f26438168a8 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -965,7 +965,9 @@ __be32 nfsd4_reclaim_complete(struct svc_rqst *, struct nfsd4_compound_state *,
 extern __be32 nfsd4_process_open1(struct nfsd4_compound_state *,
 		struct nfsd4_open *open, struct nfsd_net *nn);
 extern __be32 nfsd4_process_open2(struct svc_rqst *rqstp,
-		struct svc_fh *current_fh, struct nfsd4_open *open);
+				  struct nfsd4_compound_state *cstate,
+				  struct svc_fh *current_fh,
+				  struct nfsd4_open *open);
 extern void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate);
 extern void nfsd4_cleanup_open_state(struct nfsd4_compound_state *cstate,
 		struct nfsd4_open *open);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 10/11] nfsd: discard current_stateid.h

[NeilBrown]

From: NeilBrown <neil@brown.name>

current_stateid.h no longer contains anything useful.  So remove it.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/current_stateid.h | 8 --------
 fs/nfsd/nfs4proc.c        | 1 -
 fs/nfsd/nfs4state.c       | 1 -
 3 files changed, 10 deletions(-)
 delete mode 100644 fs/nfsd/current_stateid.h

diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
deleted file mode 100644
index 9dce3004b846..000000000000
--- a/fs/nfsd/current_stateid.h
+++ /dev/null
@@ -1,8 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _NFSD4_CURRENT_STATE_H
-#define _NFSD4_CURRENT_STATE_H
-
-#include "state.h"
-#include "xdr4.h"
-
-#endif   /* _NFSD4_CURRENT_STATE_H */
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index eaa6a8c25ed6..0c980dfc5177 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -46,7 +46,6 @@
 #include "cache.h"
 #include "xdr4.h"
 #include "vfs.h"
-#include "current_stateid.h"
 #include "netns.h"
 #include "acl.h"
 #include "pnfs.h"
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f01c72876ca9..7508158f5f8c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -50,7 +50,6 @@
 #include "xdr4.h"
 #include "xdr4cb.h"
 #include "vfs.h"
-#include "current_stateid.h"
 
 #include "netns.h"
 #include "pnfs.h"
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v5 11/11] nfsd: conditionally clear seqid when current_stateid is used.

[NeilBrown]

From: NeilBrown <neil@brown.name>

As described in RFC 8881 scions 8.2.3 on Special Stateids:

    The stateid passed to the operation in place of the special value
    has its "seqid" value set to zero, except when the current stateid
    is used by the operation CLOSE or OPEN_DOWNGRADE.

Linux NFSD does not currently follow this guidance.  The seqid (known as
si_generation) is left unchanged.

This patch introduces a new status flag SC_STATUS_KEEP_SEQID which is
only used for lookup requests and sets it for the two exceptions: CLOSE
and OPEN_DOWNGRADE.  When this flag is not present, the value copied
from the current stateid has the si_generation (aka seqid) cleared.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4state.c | 24 +++++++++++++++---------
 fs/nfsd/state.h     |  6 ++++++
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7508158f5f8c..f4b2ae7bc44d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7202,6 +7202,8 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		if (!cstate->current_fh.fh_have_stateid)
 			return nfserr_bad_stateid;
 		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
+		if (!(statusmask & SC_STATUS_KEEP_SEQID))
+			stateid->si_generation = 0;
 	}
 	/*
 	 *  only return revoked delegations if explicitly asked.
@@ -7523,6 +7525,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		if (!cstate->current_fh.fh_have_stateid)
 			return nfserr_bad_stateid;
 		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
+		stateid->si_generation = 0;
 	}
 
 	spin_lock(&cl->cl_lock);
@@ -7645,15 +7648,17 @@ nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
 	return status;
 }
 
-static __be32 nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
-						 stateid_t *stateid, struct nfs4_ol_stateid **stpp, struct nfsd_net *nn)
+static __be32
+nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
+				   stateid_t *stateid, struct nfs4_ol_stateid **stpp,
+				   struct nfsd_net *nn, unsigned short statusmask)
 {
 	__be32 status;
 	struct nfs4_openowner *oo;
 	struct nfs4_ol_stateid *stp;
 
 	status = nfs4_preprocess_seqid_op(cstate, seqid, stateid,
-					  SC_TYPE_OPEN, 0, &stp, nn);
+					  SC_TYPE_OPEN, statusmask, &stp, nn);
 	if (status)
 		return status;
 	oo = openowner(stp->st_stateowner);
@@ -7751,7 +7756,8 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
 			od->od_deleg_want);
 
 	status = nfs4_preprocess_confirmed_seqid_op(cstate, od->od_seqid,
-					&od->od_stateid, &stp, nn);
+						    &od->od_stateid, &stp, nn,
+						    SC_STATUS_KEEP_SEQID);
 	if (status)
 		goto out; 
 	status = nfserr_inval;
@@ -7821,7 +7827,8 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	status = nfs4_preprocess_seqid_op(cstate, close->cl_seqid,
 					  &close->cl_stateid,
-					  SC_TYPE_OPEN, SC_STATUS_CLOSED,
+					  SC_TYPE_OPEN,
+					  SC_STATUS_CLOSED | SC_STATUS_KEEP_SEQID,
 					  &stp, nn);
 	nfsd4_bump_seqid(cstate, status);
 	if (status)
@@ -8313,10 +8320,9 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 				sizeof(clientid_t));
 
 		/* validate and update open stateid and open seqid */
-		status = nfs4_preprocess_confirmed_seqid_op(cstate,
-				        lock->lk_new_open_seqid,
-		                        &lock->lk_new_open_stateid,
-					&open_stp, nn);
+		status = nfs4_preprocess_confirmed_seqid_op(
+			cstate,	lock->lk_new_open_seqid,
+			&lock->lk_new_open_stateid, &open_stp, nn, 0);
 		if (status)
 			goto out;
 		mutex_unlock(&open_stp->st_mutex);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index c6e97d6daa5f..6043baf10ceb 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -138,6 +138,12 @@ struct nfs4_stid {
 #define SC_STATUS_ADMIN_REVOKED	BIT(2)
 #define SC_STATUS_FREEABLE	BIT(3)
 #define SC_STATUS_FREED		BIT(4)
+/*
+ * Ops other than CLOSE and OPEN_DOWNGRADE which use the "current stateid"
+ * must clear the seqid (aka si_generation). Following flag is never stored
+ * in states but is passed through to request the seq not be cleared.
+ */
+#define SC_STATUS_KEEP_SEQID	BIT(5)
 	unsigned short		sc_status;
 
 	struct list_head	sc_cp_list;
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v5 00/11] nfsd: assorted cleanups involving v4 special stateids.

[Chuck Lever]

On 11/18/25 10:28 PM, NeilBrown wrote:
> This series started out as an attempt to simplify the management of the "current stateid" which is associated with the current filehandle.
> This end up including fixes for foreign filehandle handling as well,
> including the first patch which fixes a possibly NULL pointer dereference.
> 
> Other than that first patch, this is mostly cleanups with a few minor bug fixes.
Hi Neil, the series is looking better, but there are still some issues.
I stopped at 7/11 -- 8/11 needs is_current_stateid() to be fixed before
it makes sense.


-- 
Chuck Lever