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

[Chuck Lever <chuck.lever@oracle.com>]

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