[PATCH v3] nfsd: prevent a double put of revoked layout stateids

[Jinpyo Lee <bint4b13@gmail.com>]

nfsd4_free_stateid() finds an admin-revoked layout stateid with
find_stateid_locked(), which does not acquire a reference. The layout
case drops cl_lock without marking the stateid closed, then stops fence
work and calls nfs4_put_stid() without owning a reference. Concurrent
disposal can therefore access a freed layout stateid or drop a reference
owned by another holder.

Mark the layout stateid CLOSED under cl_lock. Take a temporary reference
before dropping cl_lock and hold it while stopping fence work and
draining the remaining layout segments, then release it with the final
put. The IDR owns no layout reference.

Use cl_lock for the CLOSED update in LAYOUTRETURN as well, matching the
common stateid locking rule. Retain ls_mutex through that update and
recheck CLOSED under cl_lock after taking ls_mutex in LAYOUTGET, so a
stale LAYOUTGET cannot add a segment after the drain. Also make the newly
allocated layout stateid the error-path put target.

Make revoked-stateid disposal idempotent with SC_STATUS_FREED under
cl_lock. An in-flight reference can keep an admin-revoked OPEN, LOCK, or
DELEG stateid in the IDR after disposal. Restrict
nfs40_clean_admin_revoked() to v4.0 clients and skip FREED entries so its
retry loop cannot repeatedly select the same stateid or consume references
owned by other callers. Use FREED rather than CLOSED to guard disposal:
delegation recall rejection can set CLOSED before the first disposal call.

Reject FREED stateids in FREE_STATEID and TEST_STATEID as well, so a
stateid retained by an in-flight reference returns BAD_STATEID after
successful disposal instead of success or ADMIN_REVOKED.

Validation used nfsd-testing at c32fef6c476d. The patched x86_64 kernel
and modules were built with GCC 12.2 and booted with Generic KASAN,
lockdep, modular NFSD, and pNFS SCSI layout support. Basic NFSv4.2,
NFSv4.0, and NFSv3 read/write/unmount smoke tests passed.

Targeted runs forced concurrent two-slot FREE_STATEID, LAYOUTGET and
LAYOUTRETURN overlaps around ls_mutex, and an NFSv4.0 cleaner retry while
a READ held an OPEN stateid reference. Stateids were reaped once, stale
requests returned BAD_STATEID, and the NFSv4.0 cleaner did not process
the NFSv4.1 client. No KASAN, lockdep, refcount, list-corruption, Oops, or
kernel BUG report occurred; kernel taint remained zero.

The vulnerability research and validation were conducted by members of
the Tobabz team as part of the Best of the Best 15th program.

Fixes: 86b9898920a6 ("NFSD: Handle layout stid in nfsd4_drop_revoked_stid()")
Suggested-by: Chuck Lever <cel@kernel.org>
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Jinpyo Lee <bint4b13@gmail.com>
---
Changes in v3:

- Address the v4.0 disposal issue reported by Sashiko and forwarded by
  Jeff Layton: claim disposal with FREED under cl_lock and skip FREED
  stateids in the admin-revocation scan.
- Preserve first-time disposal of delegations already marked CLOSED, and
  reject FREED stateids in FREE_STATEID and TEST_STATEID.
- Rebase onto nfsd-testing c32fef6c476daa5830f600614657baee59c83339
  and add targeted runtime validation.

 fs/nfsd/nfs4layouts.c | 34 ++++++++++++++++++++++++++++++++--
 fs/nfsd/nfs4state.c   | 29 +++++++++++++++++++++++++----
 fs/nfsd/pnfs.h        |  6 ++++++
 fs/nfsd/state.h       |  6 +++---
 4 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 898776c9d..749ace97e 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -312,6 +312,7 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
 		status = nfserr_jukebox;
 		if (!ls)
 			goto out;
+		stid = &ls->ls_stid;
 		mutex_lock(&ls->ls_mutex);
 	} else {
 		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
@@ -324,6 +325,15 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
 			goto out_unlock_stid;
 	}
 
+	/* A new or looked-up stateid may have been closed before ls_mutex. */
+	spin_lock(&stid->sc_client->cl_lock);
+	if (stid->sc_status & SC_STATUS_CLOSED) {
+		spin_unlock(&stid->sc_client->cl_lock);
+		status = nfserr_bad_stateid;
+		goto out_unlock_stid;
+	}
+	spin_unlock(&stid->sc_client->cl_lock);
+
 	*lsp = ls;
 	return 0;
 
@@ -537,12 +547,18 @@ nfsd4_return_file_layouts(struct svc_rqst *rqstp,
 			nfs4_inc_and_copy_stateid(&lrp->lr_sid, &ls->ls_stid);
 		lrp->lrs_present = true;
 	} else {
-		trace_nfsd_layoutstate_unhash(&ls->ls_stid.sc_stateid);
-		ls->ls_stid.sc_status |= SC_STATUS_CLOSED;
 		lrp->lrs_present = false;
 	}
 	spin_unlock(&ls->ls_lock);
 
+	if (!lrp->lrs_present) {
+		/* ls_mutex prevents a LAYOUTGET from adding another segment. */
+		spin_lock(&ls->ls_stid.sc_client->cl_lock);
+		ls->ls_stid.sc_status |= SC_STATUS_CLOSED;
+		spin_unlock(&ls->ls_stid.sc_client->cl_lock);
+		trace_nfsd_layoutstate_unhash(&ls->ls_stid.sc_stateid);
+	}
+
 	mutex_unlock(&ls->ls_mutex);
 	nfs4_put_stid(&ls->ls_stid);
 	nfsd4_free_layouts(&reaplist);
@@ -594,6 +610,20 @@ nfsd4_return_all_layouts(struct nfs4_layout_stateid *ls,
 	spin_unlock(&ls->ls_lock);
 }
 
+void nfsd4_return_revoked_layout(struct nfs4_layout_stateid *ls)
+{
+	LIST_HEAD(reaplist);
+
+	/*
+	 * The caller holds a stateid reference until segment draining ends.
+	 * Wait for any LAYOUTGET that passed the CLOSED check.
+	 */
+	mutex_lock(&ls->ls_mutex);
+	nfsd4_return_all_layouts(ls, &reaplist);
+	mutex_unlock(&ls->ls_mutex);
+	nfsd4_free_layouts(&reaplist);
+}
+
 void
 nfsd4_return_all_client_layouts(struct nfs4_client *clp)
 {
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 26d2d56a5..0f1641eab 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5849,6 +5849,17 @@ static void nfsd4_drop_revoked_stid(struct nfs4_stid *s)
 	struct nfs4_delegation *dp;
 	bool unhashed;
 
+	/*
+	 * In-flight references can keep a disposed stateid in the IDR.
+	 * CLOSED alone is not sufficient: a delegation may be closed
+	 * before its first call here.
+	 */
+	if (s->sc_status & SC_STATUS_FREED) {
+		spin_unlock(&cl->cl_lock);
+		return;
+	}
+	s->sc_status |= SC_STATUS_FREED;
+
 	switch (s->sc_type) {
 	case SC_TYPE_OPEN:
 		stp = openlockstateid(s);
@@ -5867,15 +5878,22 @@ static void nfsd4_drop_revoked_stid(struct nfs4_stid *s)
 	case SC_TYPE_DELEG:
 		dp = delegstateid(s);
 		list_del_init(&dp->dl_recall_lru);
-		s->sc_status |= SC_STATUS_FREED;
 		spin_unlock(&cl->cl_lock);
 		nfs4_put_stid(s);
 		break;
 	case SC_TYPE_LAYOUT:
 		ls = layoutstateid(s);
+		s->sc_status |= SC_STATUS_CLOSED;
+		/*
+		 * The IDR does not own a layout stateid reference. Keep ls
+		 * alive while draining worker and segment references below;
+		 * the final put releases this temporary reference.
+		 */
+		refcount_inc(&s->sc_count);
 		list_del_init(&ls->ls_perclnt);
 		spin_unlock(&cl->cl_lock);
 		nfsd4_stop_layout_fence(ls);
+		nfsd4_return_revoked_layout(ls);
 		nfs4_put_stid(s);
 		break;
 	default:
@@ -7901,6 +7919,8 @@ static void nfs40_clean_admin_revoked(struct nfsd_net *nn,
 		unsigned long id, tmp;
 		struct nfs4_stid *stid;
 
+		if (clp->cl_minorversion != 0)
+			continue;
 		if (atomic_read(&clp->cl_admin_revoked) == 0)
 			continue;
 		if (is_client_expired(clp))
@@ -7908,7 +7928,8 @@ static void nfs40_clean_admin_revoked(struct nfsd_net *nn,
 
 		spin_lock(&clp->cl_lock);
 		idr_for_each_entry_ul(&clp->cl_stateids, stid, tmp, id)
-			if (stid->sc_status & SC_STATUS_ADMIN_REVOKED) {
+			if ((stid->sc_status & SC_STATUS_ADMIN_REVOKED) &&
+			    !(stid->sc_status & SC_STATUS_FREED)) {
 				refcount_inc(&stid->sc_count);
 				atomic_inc(&clp->cl_rpc_users);
 				spin_unlock(&nn->client_lock);
@@ -8340,7 +8361,7 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 		return status;
 	spin_lock(&cl->cl_lock);
 	s = find_stateid_locked(cl, stateid);
-	if (!s)
+	if (!s || s->sc_status & SC_STATUS_FREED)
 		goto out_unlock;
 	status = nfsd4_stid_check_stateid_generation(stateid, s, 1);
 	if (status)
@@ -8725,7 +8746,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	spin_lock(&cl->cl_lock);
 	s = find_stateid_locked(cl, stateid);
-	if (!s || s->sc_status & SC_STATUS_CLOSED)
+	if (!s || s->sc_status & (SC_STATUS_CLOSED | SC_STATUS_FREED))
 		goto out_unlock;
 	if (s->sc_status & SC_STATUS_ADMIN_REVOKED) {
 		nfsd4_drop_revoked_stid(s);
diff --git a/fs/nfsd/pnfs.h b/fs/nfsd/pnfs.h
index 559df0e5b..9b0f17a4d 100644
--- a/fs/nfsd/pnfs.h
+++ b/fs/nfsd/pnfs.h
@@ -75,6 +75,7 @@ struct nfsd4_deviceid_map *nfsd4_find_devid_map(int idx);
 #ifdef CONFIG_NFSD_PNFS
 void nfsd4_setup_layout_type(struct svc_export *exp);
 void nfsd4_return_all_client_layouts(struct nfs4_client *);
+void nfsd4_return_revoked_layout(struct nfs4_layout_stateid *ls);
 void nfsd4_return_all_file_layouts(struct nfs4_client *clp,
 		struct nfs4_file *fp);
 void nfsd4_close_layout(struct nfs4_layout_stateid *ls);
@@ -93,6 +94,11 @@ static inline void nfsd4_setup_layout_type(struct svc_export *exp)
 static inline void nfsd4_return_all_client_layouts(struct nfs4_client *clp)
 {
 }
+
+static inline void nfsd4_return_revoked_layout(struct nfs4_layout_stateid *ls)
+{
+}
+
 static inline void nfsd4_return_all_file_layouts(struct nfs4_client *clp,
 		struct nfs4_file *fp)
 {
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 79dfa47b8..924a98af7 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -148,9 +148,8 @@ struct nfs4_stid {
  * nn->deleg_lock protects sc_status for hashed delegation stateids.
  * ->cl_lock protects the bits set as one is disposed of
  * (SC_STATUS_CLOSED, SC_STATUS_FREEABLE, SC_STATUS_FREED) and
- * sc_status for open and lock stateids. ->st_mutex also protects
- * sc_status for open stateids. ->ls_lock protects sc_status for
- * layout stateids.
+ * sc_status for open, lock, and layout stateids. ->st_mutex also protects
+ * sc_status for open stateids.
  */
 /*
  * For an open stateid kept around *only* to process close replays.
@@ -161,6 +160,7 @@ struct nfs4_stid {
 #define SC_STATUS_REVOKED	BIT(1)
 #define SC_STATUS_ADMIN_REVOKED	BIT(2)
 #define SC_STATUS_FREEABLE	BIT(3)
+/* Disposal has been claimed; in-flight references may still exist. */
 #define SC_STATUS_FREED		BIT(4)
 	unsigned short		sc_status;
 

base-commit: c32fef6c476daa5830f600614657baee59c83339