* [PATCH v5 1/4] NFSD: Skip close replay processing if XDR encoding fails
2025-10-16 13:49 [PATCH v5 0/4] Fix unwanted memory overwrites Chuck Lever
@ 2025-10-16 13:49 ` Chuck Lever
2025-10-16 13:49 ` [PATCH v5 2/4] NFSD: Never cache a COMPOUND when the SEQUENCE operation fails Chuck Lever
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Chuck Lever @ 2025-10-16 13:49 UTC (permalink / raw)
To: NeilBrown, Jeff Layton, Olga Kornievskaia, Dai Ngo, Tom Talpey
Cc: linux-nfs, Chuck Lever, rtm
From: Chuck Lever <chuck.lever@oracle.com>
The 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>
Reviewed-by: NeilBrown <neil@brown.name>
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v5 2/4] NFSD: Never cache a COMPOUND when the SEQUENCE operation fails
2025-10-16 13:49 [PATCH v5 0/4] Fix unwanted memory overwrites Chuck Lever
2025-10-16 13:49 ` [PATCH v5 1/4] NFSD: Skip close replay processing if XDR encoding fails Chuck Lever
@ 2025-10-16 13:49 ` Chuck Lever
2025-10-16 13:49 ` [PATCH v5 3/4] nfsd: ensure SEQUENCE replay sends a valid reply Chuck Lever
2025-10-16 13:49 ` [PATCH v5 4/4] nfsd: stop pretending that we cache the SEQUENCE reply Chuck Lever
3 siblings, 0 replies; 5+ messages in thread
From: Chuck Lever @ 2025-10-16 13:49 UTC (permalink / raw)
To: NeilBrown, Jeff Layton, Olga Kornievskaia, Dai Ngo, Tom Talpey
Cc: linux-nfs, Chuck Lever, rtm
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")
Reviewed-by: NeilBrown <neil@brown.name>
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v5 3/4] nfsd: ensure SEQUENCE replay sends a valid reply.
2025-10-16 13:49 [PATCH v5 0/4] Fix unwanted memory overwrites Chuck Lever
2025-10-16 13:49 ` [PATCH v5 1/4] NFSD: Skip close replay processing if XDR encoding fails Chuck Lever
2025-10-16 13:49 ` [PATCH v5 2/4] NFSD: Never cache a COMPOUND when the SEQUENCE operation fails Chuck Lever
@ 2025-10-16 13:49 ` Chuck Lever
2025-10-16 13:49 ` [PATCH v5 4/4] nfsd: stop pretending that we cache the SEQUENCE reply Chuck Lever
3 siblings, 0 replies; 5+ messages in thread
From: Chuck Lever @ 2025-10-16 13:49 UTC (permalink / raw)
To: NeilBrown, Jeff Layton, Olga Kornievskaia, Dai Ngo, Tom Talpey; +Cc: linux-nfs
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>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
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 4b4467e54ec9..28a79d23dd77 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4362,6 +4362,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)
@@ -4411,6 +4441,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;
@@ -4508,23 +4541,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 85b773a65670..30ce5851fe4c 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 */
};
--
2.51.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v5 4/4] nfsd: stop pretending that we cache the SEQUENCE reply.
2025-10-16 13:49 [PATCH v5 0/4] Fix unwanted memory overwrites Chuck Lever
` (2 preceding siblings ...)
2025-10-16 13:49 ` [PATCH v5 3/4] nfsd: ensure SEQUENCE replay sends a valid reply Chuck Lever
@ 2025-10-16 13:49 ` Chuck Lever
3 siblings, 0 replies; 5+ messages in thread
From: Chuck Lever @ 2025-10-16 13:49 UTC (permalink / raw)
To: NeilBrown, Jeff Layton, Olga Kornievskaia, Dai Ngo, Tom Talpey; +Cc: linux-nfs
From: NeilBrown <neil@brown.name>
nfsd does not cache the reply to a SEQUENCE. As the comment above
nfsd4_replay_cache_entry() says:
* The sequence operation is not cached because we can use the slot and
* session values.
The comment above nfsd4_cache_this() suggests otherwise.
* 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
The code in nfsd4_store_cache_entry() makes it clear that only responses
beyond 'cstate.data_offset' are actually cached, and data_offset is set
at the end of nfsd4_encode_sequence() *after* the sequence response has
been encoded.
This patch simplifies code and removes the confusing comments.
- nfsd4_is_solo_sequence() is discarded as not-useful.
- nfsd4_cache_this() is now trivial so it too is discarded with the
code placed in-line at the one call-site in nfsd4_store_cache_entry().
- nfsd4_enc_sequence_replay() is open-coded in to
nfsd4_replay_cache_entry(), and then simplified to (hopefully) make
the process of replaying a reply clearer.
Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
fs/nfsd/nfs4state.c | 58 ++++++++++++++-------------------------------
fs/nfsd/xdr4.h | 21 ----------------
2 files changed, 18 insertions(+), 61 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 28a79d23dd77..8ff7429ba060 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3507,7 +3507,7 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
free_svc_cred(&slot->sl_cred);
copy_cred(&slot->sl_cred, &resp->rqstp->rq_cred);
- if (!nfsd4_cache_this(resp)) {
+ if (!(resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)) {
slot->sl_flags &= ~NFSD4_SLOT_CACHED;
return;
}
@@ -3521,41 +3521,6 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
return;
}
-/*
- * Encode the replay sequence operation from the slot values.
- * If cachethis is FALSE encode the uncached rep error on the next
- * operation which sets resp->p and increments resp->opcnt for
- * nfs4svc_encode_compoundres.
- *
- */
-static __be32
-nfsd4_enc_sequence_replay(struct nfsd4_compoundargs *args,
- struct nfsd4_compoundres *resp)
-{
- struct nfsd4_op *op;
- struct nfsd4_slot *slot = resp->cstate.slot;
-
- /* Encode the replayed sequence operation */
- op = &args->ops[resp->opcnt - 1];
- nfsd4_encode_operation(resp, op);
-
- if (slot->sl_flags & NFSD4_SLOT_CACHED)
- return op->status;
- if (args->opcnt == 1) {
- /*
- * The original operation wasn't a solo sequence--we
- * always cache those--so this retry must not match the
- * original:
- */
- op->status = nfserr_seq_false_retry;
- } else {
- op = &args->ops[resp->opcnt++];
- op->status = nfserr_retry_uncached_rep;
- nfsd4_encode_operation(resp, op);
- }
- return op->status;
-}
-
/*
* The sequence operation is not cached because we can use the slot and
* session values.
@@ -3564,17 +3529,30 @@ static __be32
nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp,
struct nfsd4_sequence *seq)
{
+ struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
struct nfsd4_slot *slot = resp->cstate.slot;
struct xdr_stream *xdr = resp->xdr;
__be32 *p;
- __be32 status;
dprintk("--> %s slot %p\n", __func__, slot);
- status = nfsd4_enc_sequence_replay(resp->rqstp->rq_argp, resp);
- if (status)
- return status;
+ /* Always encode the SEQUENCE response. */
+ nfsd4_encode_operation(resp, &args->ops[0]);
+ if (args->opcnt == 1)
+ /* A solo SEQUENCE - nothing was cached */
+ return args->ops[0].status;
+ if (!(slot->sl_flags & NFSD4_SLOT_CACHED)) {
+ /* We weren't asked to cache this. */
+ struct nfsd4_op *op;
+
+ op = &args->ops[resp->opcnt++];
+ op->status = nfserr_retry_uncached_rep;
+ nfsd4_encode_operation(resp, op);
+ return op->status;
+ }
+
+ /* return reply from cache */
p = xdr_reserve_space(xdr, slot->sl_datalen);
if (!p) {
WARN_ON_ONCE(1);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 1ce8e12ae335..ae75846b3cd7 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -924,27 +924,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 resp->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
^ permalink raw reply related [flat|nested] 5+ messages in thread