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

[Jeff Layton <jlayton@kernel.org>]

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>