Re: [PATCH v3 1/2] nfsd: ensure SEQUENCE replay sends a valid reply.

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-10-16 at 12:31 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd4_enc_sequence_replay() uses nfsd4_encode_operation() to encode a
> new SEQUENCE reply when replaying a request from the slot cache - only
> ops after the SEQUENCE are replayed from the cache in ->sl_data.
> 
> However it does this in nfsd4_replay_cache_entry() which is called
> *before* nfsd4_sequence() has filled in reply fields.
> 
> This means that in the replayed SEQUENCE reply:
>  maxslots will be whatever the client sent
>  target_maxslots will be -1 (assuming init to zero, and
>       nfsd4_encode_sequence() subtracts 1)
>  status_flags will be zero
> 
> The incorrect maxslots value, in particular, can cause the client to
> think the slot table has been reduced in size so it can discard its
> knowledge of current sequence number of the later slots, though the
> server has not discarded those slots.  When the client later wants to
> use a later slot, it can get NFS4ERR_SEQ_MISORDERED from the server.
> 
> This patch moves the setup of the reply into a new helper function and
> call it *before* nfsd4_replay_cache_entry() is called.  Only one of the
> updated fields was used after this point - maxslots.  So the
> nfsd4_sequence struct has been extended to have separate maxslots for
> the request and the response.
> 
> Reported-by: Olga Kornievskaia <okorniev@redhat.com>
> Closes: https://lore.kernel.org/linux-nfs/20251010194449.10281-1-okorniev@redhat.com/
> Tested-by: Olga Kornievskaia <okorniev@redhat.com>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++++++++++---------------
>  fs/nfsd/nfs4xdr.c   |  2 +-
>  fs/nfsd/xdr4.h      |  3 ++-
>  3 files changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c9053ef4d79f..60451fd98bdf 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4349,6 +4349,36 @@ static bool replay_matches_cache(struct svc_rqst *rqstp,
>  	return true;
>  }
>  
> +/*
> + * Note that the response is constructed here both for the case
> + * of a new SEQUENCE request and for a replayed SEQUENCE request.
> + * We do not cache SEQUENCE responses as SEQUENCE is idempotent.
> + */
> +static void nfsd4_construct_sequence_response(struct nfsd4_session *session,
> +					      struct nfsd4_sequence *seq)
> +{
> +	struct nfs4_client *clp = session->se_client;
> +
> +	seq->maxslots_response = max(session->se_target_maxslots,
> +				     seq->maxslots);
> +	seq->target_maxslots = session->se_target_maxslots;
> +
> +	switch (clp->cl_cb_state) {
> +	case NFSD4_CB_DOWN:
> +		seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> +		break;
> +	case NFSD4_CB_FAULT:
> +		seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> +		break;
> +	default:
> +		seq->status_flags = 0;
> +	}
> +	if (!list_empty(&clp->cl_revoked))
> +		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> +	if (atomic_read(&clp->cl_admin_revoked))
> +		seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
> +}
> +
>  __be32
>  nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		union nfsd4_op_u *u)
> @@ -4398,6 +4428,9 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	dprintk("%s: slotid %d\n", __func__, seq->slotid);
>  
>  	trace_nfsd_slot_seqid_sequence(clp, seq, slot);
> +
> +	nfsd4_construct_sequence_response(session, seq);
> +
>  	status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
>  	if (status == nfserr_replay_cache) {
>  		status = nfserr_seq_misordered;
> @@ -4495,23 +4528,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	}
>  
>  out:
> -	seq->maxslots = max(session->se_target_maxslots, seq->maxslots);
> -	seq->target_maxslots = session->se_target_maxslots;
> -
> -	switch (clp->cl_cb_state) {
> -	case NFSD4_CB_DOWN:
> -		seq->status_flags = SEQ4_STATUS_CB_PATH_DOWN;
> -		break;
> -	case NFSD4_CB_FAULT:
> -		seq->status_flags = SEQ4_STATUS_BACKCHANNEL_FAULT;
> -		break;
> -	default:
> -		seq->status_flags = 0;
> -	}
> -	if (!list_empty(&clp->cl_revoked))
> -		seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
> -	if (atomic_read(&clp->cl_admin_revoked))
> -		seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
>  	trace_nfsd_seq4_status(rqstp, seq);
>  out_no_session:
>  	if (conn)
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 230bf53e39f7..6135b896b3fe 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5085,7 +5085,7 @@ nfsd4_encode_sequence(struct nfsd4_compoundres *resp, __be32 nfserr,
>  		return nfserr;
>  	/* Note slotid's are numbered from zero: */
>  	/* sr_highest_slotid */
> -	nfserr = nfsd4_encode_slotid4(xdr, seq->maxslots - 1);
> +	nfserr = nfsd4_encode_slotid4(xdr, seq->maxslots_response - 1);
>  	if (nfserr != nfs_ok)
>  		return nfserr;
>  	/* sr_target_highest_slotid */
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index ee0570cbdd9e..1ce8e12ae335 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -574,8 +574,9 @@ struct nfsd4_sequence {
>  	struct nfs4_sessionid	sessionid;		/* request/response */
>  	u32			seqid;			/* request/response */
>  	u32			slotid;			/* request/response */
> -	u32			maxslots;		/* request/response */
> +	u32			maxslots;		/* request */
>  	u32			cachethis;		/* request */
> +	u32			maxslots_response;	/* response */
>  	u32			target_maxslots;	/* response */
>  	u32			status_flags;		/* response */
>  };

I do like this version with the helper function better.

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