Re: [PATCH] nfsd: don't modify a session slot when replaying its cached reply

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2026-08-24 at 22:36 +0500, Ameer Hamza wrote:
> nfsd4_sequence() claims a session slot by setting NFSD4_SLOT_INUSE
> under nn->client_lock. A reply served from the slot's reply cache does
> not claim it, and that distinction lived only in cstate->status, which
> nfsd4_sequence() set to nfserr_replay_cache. Commit cc028a10a48c
> ("NFSD: Hoist status code encoding into XDR encoder functions") moved
> nfsd4_proc_compound()'s cstate->status assignment below the out:
> label, and the replay path's goto out was the one path that relied on
> skipping it. The test in nfsd4_sequence_done() therefore no longer
> identifies a replay, and every replay now stores its reply and clears
> NFSD4_SLOT_INUSE as though it owned the slot.
> 
> NFSD4_SLOT_INUSE is the interlock: check_slot_seqid() rejects every
> sequence id for a slot that is in use, before replay_matches_cache()
> runs. A replay never sets it, so two replays can be in flight on the
> same slot at once. One can read slot->sl_cred under nn->client_lock
> while the other's completion frees and rebuilds it from the XDR
> encoder without that lock. Two completions can also collide with each
> other and release the same group_info twice. free_svc_cred() leaves
> cr_uid, cr_gid and cr_flavor intact, so the reader passes every
> earlier test in same_creds() and dereferences a NULL cr_group_info.
> From a 6.12.91 production server:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000004
>   CPU: 63 UID: 0 PID: 39015 Comm: nfsd
>   RIP: 0010:same_creds+0x38/0xa0 [nfsd]
>   RDX: 0000000000000000
>   Call Trace:
>    nfsd4_sequence+0x6a8/0x910 [nfsd]
>    nfsd4_proc_compound+0x345/0x670 [nfsd]
>    nfsd_dispatch+0x100/0x220 [nfsd]
>    svc_process_common+0x311/0x700 [sunrpc]
>    svc_process+0x131/0x1c0 [sunrpc]
>    svc_recv+0x7ef/0x9c0 [sunrpc]
>    nfsd+0xa3/0x100 [nfsd]
>   Kernel panic - not syncing: Fatal exception
> 
> Since v6.14 a live session's slot table can shrink, and the unowned
> store becomes a use-after-free write. nfsd4_sequence() defers the
> shrink while a slot is in use, but it tests NFSD4_SLOT_INUSE, which a
> replay does not set, so free_session_slots() can kfree() the slot the
> replay still holds in cstate->slot.
> 
> Record the claim in cstate->slot_owned when nfsd4_sequence() accepts a
> request and test that in nfsd4_sequence_done(), so only the request
> that claimed the slot updates its cached reply and clears
> NFSD4_SLOT_INUSE. svc_generic_init_request() zeroes the compound
> response before each request, so the flag starts clear.
> 
> Fixes: cc028a10a48c ("NFSD: Hoist status code encoding into XDR encoder functions")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Ameer Hamza <ameer.hamza@truenas.com>
> ---
> Reproduced on an unmodified tree with a client that pipelines two
> copies of a cached request before reading either reply.
> 
>  fs/nfsd/nfs4state.c | 7 ++++++-
>  fs/nfsd/xdr4.h      | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2c1b8b2cbbb5b..a4a75a512e9f0 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5170,6 +5170,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		slot->sl_flags &= ~NFSD4_SLOT_CACHETHIS;
>  
>  	cstate->slot = slot;
> +	cstate->slot_owned = true;
>  	cstate->session = session;
>  	cstate->clp = clp;
>  
> @@ -5244,7 +5245,11 @@ nfsd4_sequence_done(struct nfsd4_compoundres *resp)
>  	struct nfsd4_compound_state *cs = &resp->cstate;
>  
>  	if (nfsd4_has_session(cs)) {
> -		if (cs->status != nfserr_replay_cache) {
> +		/*
> +		 * Only the request that claimed the slot may update its
> +		 * cached reply and clear NFSD4_SLOT_INUSE.
> +		 */
> +		if (cs->slot_owned) {
>  			nfsd4_store_cache_entry(resp);
>  			cs->slot->sl_flags &= ~NFSD4_SLOT_INUSE;
>  		}
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index b841bc462dac8..5b7edf1893e0a 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -62,6 +62,7 @@ struct nfsd4_compound_state {
>  	struct nfsd4_slot	*slot;
>  	int			data_offset;
>  	bool                    spo_must_allowed;
> +	bool			slot_owned;
>  	size_t			iovlen;
>  	u32			minorversion;
>  	__be32			status;
> 
> base-commit: 46ff234c7129c7c10ed493413d4f60d0cf4dd1f1

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