[PATCH v4 0/4] Fix unwanted memory overwrites

[PATCH v4 0/4] 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 v3:
* Neil observes that in this code path, SEQUENCE always the first op
* Expanding the size of the replay cache buffer is unnecessary
* Reordered and simplified the remaining patches
* Haven't yet addressed imbalanced maxresponsesize values

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 (4):
  NFSD: Skip close replay processing if XDR encoding fails
  NFSD: Never cache a COMPOUND when the SEQUENCE operation fails
  NFSD: Fix the "is this a solo SEQUENCE" predicate
  NFSD: Move nfsd4_cache_this()

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

-- 
2.51.0

[PATCH v4 1/4] 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 v4 2/4] 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. So when SEQUENCE
fails, cstate.data_offset is not set, allowing
read_bytes_from_xdr_buf() to access uninitialized memory.

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")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4state.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c9053ef4d79f..4b4467e54ec9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3486,7 +3486,20 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
 	struct nfsd4_slot *slot = resp->cstate.slot;
 	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 ...
+	 *
+	 * Because nfsd4_store_cache_entry is called only by
+	 * nfsd4_sequence_done(), nfsd4_store_cache_entry() is called only
+	 * when a SEQUENCE operation was part of the COMPOUND.
+	 * nfs41_check_op_ordering() ensures SEQUENCE is the first op.
+	 */
+	if (resp->opcnt == 1 && resp->cstate.status != nfs_ok)
+		return;
 
 	slot->sl_flags |= NFSD4_SLOT_INITIALIZED;
 	slot->sl_opcnt = resp->opcnt;
-- 
2.51.0

[PATCH v4 3/4] NFSD: Fix the "is this a solo SEQUENCE" predicate

[Chuck Lever]

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

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.

Note that, because nfsd4_is_solo_sequence() is called only by
nfsd4_store_cache_entry(), it is assured that the first operation
in the COMPOUND being checked is a SEQUENCE op. Thus the opnum
check is redundant.

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..d4548a16a36e 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;
 }
 
 /*
-- 
2.51.0

[PATCH v4 4/4] 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 only 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 4b4467e54ec9..5fd2138cb074 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3476,6 +3476,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;
+}
+
+/*
+ * 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);
+}
+
 /*
  * 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 d4548a16a36e..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;
-}
-
-/*
- * 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 v4 1/4] NFSD: Skip close replay processing if XDR encoding fails

[NeilBrown]

On Mon, 13 Oct 2025, Chuck Lever wrote:
> 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.
> 

Nit: I don't think this is just for "close" replay.  I think it is for
reply for any "sequence id mutating" operation that uses an open-owner.

But the patch itself is good.

Reviewed-by: NeilBrown <neil@brown.name>

Thanks,
NeilBrown

> 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
> 
> 

Re: [PATCH v4 2/4] NFSD: Never cache a COMPOUND when the SEQUENCE operation fails

[NeilBrown]

On Mon, 13 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. So when SEQUENCE
> fails, cstate.data_offset is not set, allowing
> read_bytes_from_xdr_buf() to access uninitialized memory.
> 
> 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")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: NeilBrown <neil@brown.name>

Thanks,
NeilBrown

> ---
>  fs/nfsd/nfs4state.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c9053ef4d79f..4b4467e54ec9 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3486,7 +3486,20 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
>  	struct nfsd4_slot *slot = resp->cstate.slot;
>  	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 ...
> +	 *
> +	 * Because nfsd4_store_cache_entry is called only by
> +	 * nfsd4_sequence_done(), nfsd4_store_cache_entry() is called only
> +	 * when a SEQUENCE operation was part of the COMPOUND.
> +	 * nfs41_check_op_ordering() ensures SEQUENCE is the first op.
> +	 */
> +	if (resp->opcnt == 1 && resp->cstate.status != nfs_ok)
> +		return;
>  
>  	slot->sl_flags |= NFSD4_SLOT_INITIALIZED;
>  	slot->sl_opcnt = resp->opcnt;
> -- 
> 2.51.0
> 
> 

Re: [PATCH v4 3/4] NFSD: Fix the "is this a solo SEQUENCE" predicate

[NeilBrown]

On Mon, 13 Oct 2025, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> 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.
> 
> Note that, because nfsd4_is_solo_sequence() is called only by
> nfsd4_store_cache_entry(), it is assured that the first operation
> in the COMPOUND being checked is a SEQUENCE op. Thus the opnum
> check is redundant.

It is also assured that the SEQUENCE op didn't fail, so the distinction
between resp->opcnt and args->opcnt is moot.

I don't think nfsd4_is_solo_sequence() serves any useful purpose.
The only case were the result has any effect, the effect is to set
NFSD4_SLOT_CACHED, and to set sl_datalen to zero.

I would prefer that the code didn't pretend that solo sequence requests
were cached - they aren't.  They are simply performed again when needed.
But that can be for another day.

I don't think this patch achieves anything useful, but I don't object to
it.

NeilBrowjn



> 
> 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..d4548a16a36e 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;
>  }
>  
>  /*
> -- 
> 2.51.0
> 
> 

Re: [PATCH v4 4/4] NFSD: Move nfsd4_cache_this()

[NeilBrown]

On Mon, 13 Oct 2025, Chuck Lever wrote:
> 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 only caller.
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: NeilBrown <neil@brown.name>

Definitely better having the code local when possible.

Thanks,
NeilBrown


> ---
>  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 4b4467e54ec9..5fd2138cb074 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3476,6 +3476,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;
> +}
> +
> +/*
> + * 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);
> +}
> +
>  /*
>   * 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 d4548a16a36e..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;
> -}
> -
> -/*
> - * 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 v4 3/4] NFSD: Fix the "is this a solo SEQUENCE" predicate

[Chuck Lever]

On 10/13/25 12:43 AM, NeilBrown wrote:
> On Mon, 13 Oct 2025, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>>
>> 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.
>>
>> Note that, because nfsd4_is_solo_sequence() is called only by
>> nfsd4_store_cache_entry(), it is assured that the first operation
>> in the COMPOUND being checked is a SEQUENCE op. Thus the opnum
>> check is redundant.
> 
> It is also assured that the SEQUENCE op didn't fail, so the distinction
> between resp->opcnt and args->opcnt is moot.
> 
> I don't think nfsd4_is_solo_sequence() serves any useful purpose.
> The only case were the result has any effect, the effect is to set
> NFSD4_SLOT_CACHED, and to set sl_datalen to zero.
> 
> I would prefer that the code didn't pretend that solo sequence requests
> were cached - they aren't.  They are simply performed again when needed.
> But that can be for another day.

One wonders why the comments take such pains to call out "caching solo
sequence" operations if these operations aren't actually cached. Perhaps
I should replace this patch with one that removes
nfsd4_is_solo_sequence() .

Here's why this is relevant:

 941 static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)

 942 {

 943         return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)

 944                 || nfsd4_is_solo_sequence(resp);

 945 }

If nfsd4_is_solo_sequence always returns true, then so does
nfsd4_cache_this, it looks like.


> I don't think this patch achieves anything useful, but I don't object to
> it.
> 
> NeilBrowjn
> 
> 
> 
>>
>> 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..d4548a16a36e 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;
>>  }
>>  
>>  /*
>> -- 
>> 2.51.0
>>
>>
> 


-- 
Chuck Lever

Re: [PATCH v4 3/4] NFSD: Fix the "is this a solo SEQUENCE" predicate

[NeilBrown]

On Tue, 14 Oct 2025, Chuck Lever wrote:
> On 10/13/25 12:43 AM, NeilBrown wrote:
> > On Mon, 13 Oct 2025, Chuck Lever wrote:
> >> From: Chuck Lever <chuck.lever@oracle.com>
> >>
> >> 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.
> >>
> >> Note that, because nfsd4_is_solo_sequence() is called only by
> >> nfsd4_store_cache_entry(), it is assured that the first operation
> >> in the COMPOUND being checked is a SEQUENCE op. Thus the opnum
> >> check is redundant.
> > 
> > It is also assured that the SEQUENCE op didn't fail, so the distinction
> > between resp->opcnt and args->opcnt is moot.
> > 
> > I don't think nfsd4_is_solo_sequence() serves any useful purpose.
> > The only case were the result has any effect, the effect is to set
> > NFSD4_SLOT_CACHED, and to set sl_datalen to zero.
> > 
> > I would prefer that the code didn't pretend that solo sequence requests
> > were cached - they aren't.  They are simply performed again when needed.
> > But that can be for another day.
> 
> One wonders why the comments take such pains to call out "caching solo
> sequence" operations if these operations aren't actually cached. Perhaps
> I should replace this patch with one that removes
> nfsd4_is_solo_sequence() .
> 
> Here's why this is relevant:
> 
>  941 static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
> 
>  942 {
> 
>  943         return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
> 
>  944                 || nfsd4_is_solo_sequence(resp);
> 
>  945 }
> 
> If nfsd4_is_solo_sequence always returns true, then so does
> nfsd4_cache_this, it looks like.

True, but nfsd4_is_solo_sequence() doesn't always return true.
If any op before the SEQUENCE has been attempted, then it will fail.

But yes, lets remove nfsd4_is_solo_sequence().  I'll send a couple of
patches to show what I am thinking.

NeilBrown


> 
> 
> > I don't think this patch achieves anything useful, but I don't object to
> > it.
> > 
> > NeilBrowjn
> > 
> > 
> > 
> >>
> >> 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..d4548a16a36e 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;
> >>  }
> >>  
> >>  /*
> >> -- 
> >> 2.51.0
> >>
> >>
> > 
> 
> 
> -- 
> Chuck Lever
>