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

[Jeff Layton <jlayton@kernel.org>]

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>