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

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

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