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

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2025-10-14 at 11:04 +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 replay 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
> 
> which might mislead the client.
> 
> This patch moves the setup of the reply to *before*
> nfsd4_replay_cache_entry() is called.  Only one of the updated fields is
> used after this point - maxslots.  So that field is copied to
> client_maxslots so that can be used as needed.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c9053ef4d79f..1c01836e8507 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4360,6 +4360,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct nfs4_client *clp;
>  	struct nfsd4_slot *slot;
>  	struct nfsd4_conn *conn;
> +	u32 client_maxslots;
>  	__be32 status;
>  	int buflen;
>  	struct net *net = SVC_NET(rqstp);
> @@ -4398,6 +4399,27 @@ 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);
> +
> +	/* prepare reply so that it is ready for nfsd4_replay_cache_entry() */
> +	client_maxslots = seq->maxslots;
> +	seq->maxslots = max(session->se_target_maxslots, client_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;
> +
>  	status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
>  	if (status == nfserr_replay_cache) {
>  		status = nfserr_seq_misordered;
> @@ -4425,7 +4447,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	if (session->se_target_maxslots < session->se_fchannel.maxreqs &&
>  	    slot->sl_generation == session->se_slot_gen &&
> -	    seq->maxslots <= session->se_target_maxslots)
> +	    client_maxslots <= session->se_target_maxslots)
>  		/* Client acknowledged our reduce maxreqs */
>  		free_session_slots(session, session->se_target_maxslots);
>  
> @@ -4495,23 +4517,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)

This does seem like the right thing to do.

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