[PATCH v3 0/5] Fix unwanted memory overwrites

[PATCH v3 0/5] Fix unwanted memory overwrites

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

<rtm@csail.mit.edu> reported some memory overwrites that can be
triggered by NFS client input. I was able to observe overwrites
by enabling KASAN and running his reproducer [1].

NFSD caches COMPOUNDs containing only a single SEQUENCE operation
whether the client requests it to or not, in order to work around a
quirk in the NFSv4.1 protocol. However, the predicate that
identifies solo SEQUENCE operations was incorrect.

Changes since v2:
* Never cache a COMPOUND if SEQUENCE fails
* Enable caching of solo SEQUENCE operations again
* Reserve enough slot replay cache space to cache solo SEQUENCE

Changes since v1:
* Reordered patches
* Disable caching of solo SEQUENCE operations
* Additional clean up

Chuck Lever (5):
  NFSD: Skip close replay processing if XDR encoding fails
  NFSD: Fix the "is this a solo SEQUENCE" predicate
  nfsd: Never cache a COMPOUND when the SEQUENCE operation fails
  NFSD: Increase minimum size of slot replay cache
  NFSD: Move nfsd4_cache_this()

 fs/nfsd/nfs4state.c | 45 +++++++++++++++++++++++++++++++++++++++------
 fs/nfsd/nfs4xdr.c   |  3 +--
 fs/nfsd/xdr4.h      | 21 ---------------------
 3 files changed, 40 insertions(+), 29 deletions(-)

-- 
2.51.0

[PATCH v3 1/5] NFSD: Skip close replay processing if XDR encoding fails

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

The close replay logic added by commit 9411b1d4c7df ("nfsd4: cleanup
handling of nfsv4.0 closed stateid's") cannot be done if encoding
failed due to a short send buffer; there's no guarantee that the
operation encoder has actually encoded the data that is being copied
to the replay cache.

Reported-by: <rtm@csail.mit.edu>
Closes: https://lore.kernel.org/linux-nfs/c3628d57-94ae-48cf-8c9e-49087a28cec9@oracle.com/T/#t
Fixes: 9411b1d4c7df ("nfsd4: cleanup handling of nfsv4.0 closed stateid's")
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 230bf53e39f7..85b773a65670 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -5937,8 +5937,7 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
 		 */
 		warn_on_nonidempotent_op(op);
 		xdr_truncate_encode(xdr, op_status_offset + XDR_UNIT);
-	}
-	if (so) {
+	} else if (so) {
 		int len = xdr->buf->len - (op_status_offset + XDR_UNIT);
 
 		so->so_replay.rp_status = op->status;
-- 
2.51.0

[PATCH v3 2/5] NFSD: Fix the "is this a solo SEQUENCE" predicate

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

RFC 8881 Section 2.10.6.4 says:

> If sa_cachethis or csa_cachethis is TRUE, then the replier MUST
> cache a reply except if an error is returned by the SEQUENCE or
> CB_SEQUENCE operation (see Section 2.10.6.1.2).

This text also appears in RFC 5661. Further, Section 2.10.6.1.2
says:

> Any time SEQUENCE or CB_SEQUENCE returns an error, the sequence ID
> of the slot MUST NOT change. The replier MUST NOT modify the reply
> cache entry for the slot whenever an error is returned from
> SEQUENCE or CB_SEQUENCE.

NFSD caches the result of solo SEQUENCE operations, but rtm's
reproducer sends two operations in the failing COMPOUND. NFSD should
not attempt to cache the reply.

The logic in nfsd4_is_solo_sequence() is incorrect: it checks the
current operation index, not the total count of operations in the
COMPOUND. If the SEQUENCE operation, which is always operation 1,
fails in a multi-operation compound, resp->opcnt is always 1. Thus
when a SEQUENCE operation fails, nfsd4_is_solo_sequence() always
returns true.

Reported-by: <rtm@csail.mit.edu>
Closes: https://lore.kernel.org/linux-nfs/c3628d57-94ae-48cf-8c9e-49087a28cec9@oracle.com/T/#t
Fixes: 468de9e54a90 ("nfsd41: expand solo sequence check")
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/xdr4.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index ee0570cbdd9e..d1837a10b0c2 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -926,7 +926,8 @@ struct nfsd4_compoundres {
 static inline bool nfsd4_is_solo_sequence(struct nfsd4_compoundres *resp)
 {
 	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
-	return resp->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;
+
+	return args->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;
 }
 
 /*
-- 
2.51.0

[PATCH v3 3/5] nfsd: Never cache a COMPOUND when the SEQUENCE operation fails

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

RFC 8881 normatively mandates that operations where the initial
SEQUENCE operation in a compound fails must not modify the slot's
replay cache.

nfsd4_cache_this() doesn't prevent such caching.

Fixes: 468de9e54a90 ("nfsd41: expand solo sequence check")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4state.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c9053ef4d79f..7b80f00fb32c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3477,16 +3477,26 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
 }
 
 /*
- * Cache a reply. nfsd4_check_resp_size() has bounded the cache size.
+ * Maybe cache a reply. nfsd4_check_resp_size() has bounded the cache size.
  */
 static void
 nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
 {
-	struct xdr_buf *buf = resp->xdr->buf;
+	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
 	struct nfsd4_slot *slot = resp->cstate.slot;
+	struct xdr_buf *buf = resp->xdr->buf;
 	unsigned int base;
 
-	dprintk("--> %s slot %p\n", __func__, slot);
+	/*
+	 * RFC 5661 Section 2.10.6.1.2:
+	 *
+	 * Any time SEQUENCE ... returns an error ... [t]he replier MUST NOT
+	 * modify the reply cache entry for the slot whenever an error is
+	 * returned from SEQUENCE ...
+	 */
+	if (resp->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE &&
+	    resp->cstate.status != nfs_ok)
+		return;
 
 	slot->sl_flags |= NFSD4_SLOT_INITIALIZED;
 	slot->sl_opcnt = resp->opcnt;
-- 
2.51.0

[PATCH v3 4/5] NFSD: Increase minimum size of slot replay cache

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

NFSD caches solo SEQUENCE even when sa_cachethis is false. Ensure
there is enough space in each slot replay cache, even when the
client requested a zero ca_maxresponsesize_cached.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4state.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7b80f00fb32c..7d297ac2bf2b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2024,11 +2024,12 @@ static struct nfsd4_slot *nfsd4_alloc_slot(struct nfsd4_channel_attrs *fattrs,
 	size_t size;
 
 	/*
-	 * The RPC and NFS session headers are never saved in
-	 * the slot reply cache buffer.
+	 * Reserve enough space to handle solo SEQUENCE operations,
+	 * which are always cached.
 	 */
 	size = fattrs->maxresp_cached < NFSD_MIN_HDR_SEQ_SZ ?
-		0 : fattrs->maxresp_cached - NFSD_MIN_HDR_SEQ_SZ;
+		NFSD_MIN_HDR_SEQ_SZ :
+		fattrs->maxresp_cached - NFSD_MIN_HDR_SEQ_SZ;
 
 	slot = kzalloc(struct_size(slot, sl_data, size), gfp);
 	if (!slot)
-- 
2.51.0

[PATCH v3 5/5] NFSD: Move nfsd4_cache_this()

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

nfsd4_cache_this() has one call site, and is not related to XDR at
all. It doesn't belong in fs/nfsd/xdr4.h.

As a clean-up, move this function (and its helper) to nfs4state.c,
next to its caller.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4state.c | 22 ++++++++++++++++++++++
 fs/nfsd/xdr4.h      | 22 ----------------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7d297ac2bf2b..e8143bbc7974 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3477,6 +3477,28 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
 	return;
 }
 
+static inline bool nfsd4_is_solo_sequence(struct nfsd4_compoundres *resp)
+{
+	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
+
+	return args->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;
+}
+
+/*
+ * The session reply cache only needs to cache replies that the client
+ * actually asked us to.  But it's almost free for us to cache compounds
+ * consisting of only a SEQUENCE op, so we may as well cache those too.
+ * Also, the protocol doesn't give us a convenient response in the case
+ * of a replay of a solo SEQUENCE op that wasn't cached
+ * (RETRY_UNCACHED_REP can only be returned in the second op of a
+ * compound).
+ */
+static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
+{
+	return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
+		|| nfsd4_is_solo_sequence(resp);
+}
+
 /*
  * Maybe cache a reply. nfsd4_check_resp_size() has bounded the cache size.
  */
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index d1837a10b0c2..6f0129ea754d 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -923,28 +923,6 @@ struct nfsd4_compoundres {
 	struct nfsd4_compound_state	cstate;
 };
 
-static inline bool nfsd4_is_solo_sequence(struct nfsd4_compoundres *resp)
-{
-	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
-
-	return args->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;
-}
-
-/*
- * The session reply cache only needs to cache replies that the client
- * actually asked us to.  But it's almost free for us to cache compounds
- * consisting of only a SEQUENCE op, so we may as well cache those too.
- * Also, the protocol doesn't give us a convenient response in the case
- * of a replay of a solo SEQUENCE op that wasn't cached
- * (RETRY_UNCACHED_REP can only be returned in the second op of a
- * compound).
- */
-static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
-{
-	return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
-		|| nfsd4_is_solo_sequence(resp);
-}
-
 static inline bool nfsd4_last_compound_op(struct svc_rqst *rqstp)
 {
 	struct nfsd4_compoundres *resp = rqstp->rq_resp;
-- 
2.51.0

Re: [PATCH v3 3/5] nfsd: Never cache a COMPOUND when the SEQUENCE operation fails

[Jeff Layton]

On Fri, 2025-10-10 at 09:56 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> RFC 8881 normatively mandates that operations where the initial
> SEQUENCE operation in a compound fails must not modify the slot's
> replay cache.
> 
> nfsd4_cache_this() doesn't prevent such caching.
> 
> Fixes: 468de9e54a90 ("nfsd41: expand solo sequence check")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c9053ef4d79f..7b80f00fb32c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3477,16 +3477,26 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>  }
>  
>  /*
> - * Cache a reply. nfsd4_check_resp_size() has bounded the cache size.
> + * Maybe cache a reply. nfsd4_check_resp_size() has bounded the cache size.
>   */
>  static void
>  nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
>  {
> -	struct xdr_buf *buf = resp->xdr->buf;
> +	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
>  	struct nfsd4_slot *slot = resp->cstate.slot;
> +	struct xdr_buf *buf = resp->xdr->buf;
>  	unsigned int base;
>  
> -	dprintk("--> %s slot %p\n", __func__, slot);
> +	/*
> +	 * RFC 5661 Section 2.10.6.1.2:
> +	 *
> +	 * Any time SEQUENCE ... returns an error ... [t]he replier MUST NOT
> +	 * modify the reply cache entry for the slot whenever an error is
> +	 * returned from SEQUENCE ...
> +	 */
> +	if (resp->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE &&
> +	    resp->cstate.status != nfs_ok)
> +		return;
>  
>  	slot->sl_flags |= NFSD4_SLOT_INITIALIZED;
>  	slot->sl_opcnt = resp->opcnt;

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

Re: [PATCH v3 4/5] NFSD: Increase minimum size of slot replay cache

[Jeff Layton]

On Fri, 2025-10-10 at 09:56 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> NFSD caches solo SEQUENCE even when sa_cachethis is false. Ensure
> there is enough space in each slot replay cache, even when the
> client requested a zero ca_maxresponsesize_cached.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 7b80f00fb32c..7d297ac2bf2b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2024,11 +2024,12 @@ static struct nfsd4_slot *nfsd4_alloc_slot(struct nfsd4_channel_attrs *fattrs,
>  	size_t size;
>  
>  	/*
> -	 * The RPC and NFS session headers are never saved in
> -	 * the slot reply cache buffer.
> +	 * Reserve enough space to handle solo SEQUENCE operations,
> +	 * which are always cached.
>  	 */
>  	size = fattrs->maxresp_cached < NFSD_MIN_HDR_SEQ_SZ ?
> -		0 : fattrs->maxresp_cached - NFSD_MIN_HDR_SEQ_SZ;
> +		NFSD_MIN_HDR_SEQ_SZ :
> +		fattrs->maxresp_cached - NFSD_MIN_HDR_SEQ_SZ;
>  
>  	slot = kzalloc(struct_size(slot, sl_data, size), gfp);
>  	if (!slot)

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

Re: [PATCH v3 4/5] NFSD: Increase minimum size of slot replay cache

[NeilBrown]

On Sat, 11 Oct 2025, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> NFSD caches solo SEQUENCE even when sa_cachethis is false. Ensure
> there is enough space in each slot replay cache, even when the
> client requested a zero ca_maxresponsesize_cached.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 7b80f00fb32c..7d297ac2bf2b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2024,11 +2024,12 @@ static struct nfsd4_slot *nfsd4_alloc_slot(struct nfsd4_channel_attrs *fattrs,
>  	size_t size;
>  
>  	/*
> -	 * The RPC and NFS session headers are never saved in
> -	 * the slot reply cache buffer.
> +	 * Reserve enough space to handle solo SEQUENCE operations,
> +	 * which are always cached.
>  	 */
>  	size = fattrs->maxresp_cached < NFSD_MIN_HDR_SEQ_SZ ?
> -		0 : fattrs->maxresp_cached - NFSD_MIN_HDR_SEQ_SZ;
> +		NFSD_MIN_HDR_SEQ_SZ :
> +		fattrs->maxresp_cached - NFSD_MIN_HDR_SEQ_SZ;

This seems incoherent.
The existence of, and documentation for, NFSD_MIN_HDR_SEQ_SZ implies
that the SEQUENCE reply is NOT stored in sl_data.  When maxresp_cached
is large, we STILL don't allocate space to store the SEQUENCE reply in
sl_data, only the subsequent ops.  So allocating NFSD_MIN_HDR_SEQ_SZ
(which is more that just the SEQUENCE reply, it is also the COMPOUND
reply and the RPC header) makes no sense.

It appears that (this part of) the code is designed to generate the
SEQUENCE reply (like the COMPOUND reply and the RPC reply) anew each
time, and only replay the remainder from sl_data.  Certainly that is
what the comment above NFSD_MIN_HDR_SEQ_SZ says.

nfsd4_encode_sequence() sets cstate.data_offset *after* encoding the
sequence reply.  So that seems to be doing the right thing.
nfsd4_replay_cache_entry() encodes a SEQUENCE reply before including the
data from sl_data.

So where is a problem occurring?

Looking back at the bug report, I think that the only problem was that
nfsd4_is_solo_sequence() was always reporting true, so we always try to
cache the result, even when too big.

Though ...  the bug report has the client send a maxresp_cached which is
LARGER than maxresp_size.  That is weird.  I'm not sure we handle that
well.  I think that if maxresp_cache is larger, we need to clip it to
maxresp_size as it makes no sense to try to cache a reply large than we
are willing to handle....

NeilBrown


>  
>  	slot = kzalloc(struct_size(slot, sl_data, size), gfp);
>  	if (!slot)
> -- 
> 2.51.0
> 
> 

Re: [PATCH v3 2/5] NFSD: Fix the "is this a solo SEQUENCE" predicate

[NeilBrown]

On Sat, 11 Oct 2025, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> RFC 8881 Section 2.10.6.4 says:
> 
> > If sa_cachethis or csa_cachethis is TRUE, then the replier MUST
> > cache a reply except if an error is returned by the SEQUENCE or
> > CB_SEQUENCE operation (see Section 2.10.6.1.2).
> 
> This text also appears in RFC 5661. Further, Section 2.10.6.1.2
> says:
> 
> > Any time SEQUENCE or CB_SEQUENCE returns an error, the sequence ID
> > of the slot MUST NOT change. The replier MUST NOT modify the reply
> > cache entry for the slot whenever an error is returned from
> > SEQUENCE or CB_SEQUENCE.
> 
> NFSD caches the result of solo SEQUENCE operations, but rtm's
> reproducer sends two operations in the failing COMPOUND. NFSD should
> not attempt to cache the reply.
> 
> The logic in nfsd4_is_solo_sequence() is incorrect: it checks the
> current operation index, not the total count of operations in the
> COMPOUND. If the SEQUENCE operation, which is always operation 1,
> fails in a multi-operation compound, resp->opcnt is always 1. Thus
> when a SEQUENCE operation fails, nfsd4_is_solo_sequence() always
> returns true.
> 
> Reported-by: <rtm@csail.mit.edu>
> Closes: https://lore.kernel.org/linux-nfs/c3628d57-94ae-48cf-8c9e-49087a28cec9@oracle.com/T/#t
> Fixes: 468de9e54a90 ("nfsd41: expand solo sequence check")
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/xdr4.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index ee0570cbdd9e..d1837a10b0c2 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -926,7 +926,8 @@ struct nfsd4_compoundres {
>  static inline bool nfsd4_is_solo_sequence(struct nfsd4_compoundres *resp)
>  {
>  	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
> -	return resp->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;
> +
> +	return args->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;

This looks right, but .....

nfsd4_is_solo_sequence() is only called from nfsd4_cache_this().

nfsd4_cache_this() is only called from nfsd4_store_cache_entry() from
nfsd4_sequence_done() from nfs4svc_encode_compound() which is *after*
all ops have been encoded.  So resp->opcnt will be a count of all
successful ops, plus possibly one unsuccessful op.

So if resp->opcnt is one then either the SEQUENCE failed, or there is
only a SEQUENCE.

So while I think this patch is good, I think the justification is more
subtle that given in the commit message.

(digs through the code some more).

Ah-ha.  The root cause for the reported bug is that the data to cache is
determined (in part) but cstate.data_offset, and that is not set if
SEQUENCE is no successful.  So we absolutely must not call
nfsd4_store_cache_entry() is SEQUENCE failed.

Patch 3/5 does that.  With that in place, this patch is just cosmetic.

NeilBrown



>  }
>  
>  /*
> -- 
> 2.51.0
> 
> 

Re: [PATCH v3 3/5] nfsd: Never cache a COMPOUND when the SEQUENCE operation fails

[NeilBrown]

On Sat, 11 Oct 2025, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> RFC 8881 normatively mandates that operations where the initial
> SEQUENCE operation in a compound fails must not modify the slot's
> replay cache.
> 
> nfsd4_cache_this() doesn't prevent such caching.
> 
> Fixes: 468de9e54a90 ("nfsd41: expand solo sequence check")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c9053ef4d79f..7b80f00fb32c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3477,16 +3477,26 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>  }
>  
>  /*
> - * Cache a reply. nfsd4_check_resp_size() has bounded the cache size.
> + * Maybe cache a reply. nfsd4_check_resp_size() has bounded the cache size.
>   */
>  static void
>  nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
>  {
> -	struct xdr_buf *buf = resp->xdr->buf;
> +	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
>  	struct nfsd4_slot *slot = resp->cstate.slot;
> +	struct xdr_buf *buf = resp->xdr->buf;
>  	unsigned int base;
>  
> -	dprintk("--> %s slot %p\n", __func__, slot);
> +	/*
> +	 * RFC 5661 Section 2.10.6.1.2:
> +	 *
> +	 * Any time SEQUENCE ... returns an error ... [t]he replier MUST NOT
> +	 * modify the reply cache entry for the slot whenever an error is
> +	 * returned from SEQUENCE ...
> +	 */
> +	if (resp->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE &&
> +	    resp->cstate.status != nfs_ok)
> +		return;

This function is only called from nfsd4_sequence_done() when
nfsd4_has_session(), and that can only succeed if a SEQUENCE op
(as the first op) has been performed.

So the OP_SEQUENCE test is redundant.  So args is not necessary.

I think this is the primary bug-fix and should be moved earlier in the
series.

It would be worth noting in the commit message that when SEQUENCE fails,
cstate.data_offset is not set, so the function can access uninitialised
data.

Thanks,
NeilBrown





>  
>  	slot->sl_flags |= NFSD4_SLOT_INITIALIZED;
>  	slot->sl_opcnt = resp->opcnt;
> -- 
> 2.51.0
> 
>