Skip to contentSkip to conversation

NFSD: Fix UAFs in client teardown and state revocation

PATCH Superseded Chuck Lever 1 participant 10 messages Jul 7

Subsystems (2)
NFS client NFS server

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Chuck Lever

author maintainer
#0 lore raw

v3 cover NFSD: Fix UAFs in client teardown and state revocation

An NFSv4 stateid, and a bare lock owner reachable through the client's owner hash, hold only a raw pointer to the owning nfs4_client. That client outlives its state only because __destroy_client() drains every stateid and owner before free_client() runs. A walk that dereferences the client, or the owner, through one of those raw pointers after dropping the lock that kept it reachable races a concurrent teardown that can free it first. The NULL-pointer dereference reported during client teardown was one such race; auditing the pattern found more across the revocation and laundromat-reaping paths.

Where the racing object is the client, the fix is uniform: pin it with cl_rpc_users across the window in which nn->client_lock is dropped. Skipping a client that is already expiring is part of the correctness argument, not cleanup. force_expire_client() stops consulting cl_rpc_users once its wait has passed, so a pin taken then would not hold the client; the walk must instead leave that client's state for its own teardown to drain. Testing the expiry and taking the pin under nn->client_lock is what makes the choice atomic against the expiry.

Pinning the client from these paths introduces a lock-order edge that did not exist before: nn->client_lock now nests outside the inner region of nn->deleg_lock and outside nn->blocked_locks_lock, both leaf acquisitions everywhere else. The nesting is one-directional, and netns.h records the deleg_lock relationship; a reaping path added later must take nn->client_lock first.

Two changes sit apart from the lifetime races. The revocation and reaping fixes left four open-coded copies of the pin-drop idiom, consolidated here into put_client_no_renew() helpers (patch 6). The same audit turned up an unrelated svc_export leak:
free_ol_stateid_reaplist() reaps open and lock stateids through ->sc_free() directly, bypassing the reference drop nfs4_put_stid() performs, so the export stays pinned and blocks unmount (patch 9).

---
Changes in v2:
- Add matching UAF fixes in several other paths

---
Changes in v3:
- Fix client UAF in laundromat blocked-lock reaping (Neil)
- Fix client UAF in laundromat close_lru reaping (sashiko)
- Fix svc_export leak when reaping open stateids (sashiko)
- Link to v2: https://patch.msgid.link/[email protected]

---
Chuck Lever (9):
NFSD: Prevent lock owner use-after-free during client teardown
NFSD: Prevent client use-after-free during delegation revoke
NFSD: Prevent client use-after-free during admin state revocation
NFSD: Prevent client use-after-free during export state revocation
NFSD: Prevent client use-after-free during NFSv4.0 revoked-state cleanup
NFSD: Consolidate the revocation-path client unpin
NFSD: Prevent client use-after-free during blocked-lock reaping
NFSD: Prevent client use-after-free during close_lru reaping
NFSD: Release the export reference when reaping open stateids

 fs/nfsd/netns.h     |   6 ++-
 fs/nfsd/nfs4state.c | 145 ++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 128 insertions(+), 23 deletions(-)

---
base-commit: ee6ae4a6bf3565b880dfb420017337475dfbc9ea
change-id: 20260705-cel-61c1c70caa03

Best regards,

Signature · 1 line
Chuck Lever
author maintainer
#1 lore raw

PATCH v3 1/9 NFSD: Prevent lock owner use-after-free during client teardown

__destroy_client() releases a client's open owners, but a lock owner whose only reference is a blocked lock (nbl) stays on cl_ownerstr_hashtbl. client_has_state() does not count a bare owner, so DESTROY_CLIENTID can reach __destroy_client() with such owners present.

__destroy_client() then walks the table, calling remove_blocked_locks() on each owner without a reference. Freeing a blocked lock drops the owner reference held via flc_owner. The per-net laundromat reaps blocked locks from nn->blocked_locks_lru independently of client state. The two paths share blocked_locks_lock only for the list splice, not the owner's lifetime. The laundromat therefore frees the owner as __destroy_client() dereferences it, a NULL dereference in remove_blocked_locks().

nfsd4_release_lockowner() holds a reference across the same call; __destroy_client() does not. Hold cl_lock across the walk, taking a reference and unhashing each owner, then drop it before remove_blocked_locks() and nfs4_put_stateowner(), which take blocked_locks_lock and cl_lock.

Reported-by Wolfgang Walter <[email protected]>
Closes https://lore.kernel.org/linux-nfs/[email protected]/ open here
Fixes 68ef3bc31664 ("nfsd: remove blocked locks on client teardown")
Reviewed-by NeilBrown <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfs4state.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
fs/nfsd/nfs4state.c +13 -3
index a4398dc861a5..e000ed3e96e9 100644
@@ -2758,14 +2758,24 @@ __destroy_client(struct nfs4_client *clp)
 		release_openowner(oo);
 	}
 	for (i = 0; i < OWNER_HASH_SIZE; i++) {
-		struct nfs4_stateowner *so, *tmp;
+		struct nfs4_stateowner *so;
 
-		list_for_each_entry_safe(so, tmp, &clp->cl_ownerstr_hashtbl[i],
-					 so_strhash) {
+		spin_lock(&clp->cl_lock);
+		while (!list_empty(&clp->cl_ownerstr_hashtbl[i])) {
+			so = list_first_entry(&clp->cl_ownerstr_hashtbl[i],
+					      struct nfs4_stateowner, so_strhash);
 			/* Should be no openowners at this point */
 			WARN_ON_ONCE(so->so_is_open_owner);
+			nfs4_get_stateowner(so);
+			unhash_lockowner_locked(lockowner(so));
+			spin_unlock(&clp->cl_lock);
+
 			remove_blocked_locks(lockowner(so));
+			nfs4_put_stateowner(so);
+
+			spin_lock(&clp->cl_lock);
 		}
+		spin_unlock(&clp->cl_lock);
 	}
 	nfsd4_return_all_client_layouts(clp);
 	nfsd4_shutdown_copy(clp);
Signature · 1 line
2.54.0
author maintainer
#2 lore raw

PATCH v3 2/9 NFSD: Prevent client use-after-free during delegation revoke

In reply to Chuck Lever

A delegation stateid holds only a bare pointer to its owning nfs4_client and does not keep it alive. The client survives its stateids only because __destroy_client() drains cl_delegations and cl_revoked before free_client() runs.

nfs4_laundromat() breaks that invariant: it unhashes an expired delegation from cl_delegations, drops deleg_lock, then revoke_delegation() relinks it onto cl_revoked under cl_lock. In that window the delegation is on neither list, so client_has_state() can report no remaining state.

Every teardown path first requires cl_rpc_users to be zero, but the laundromat holds no such reference. A client whose recalled delegation has just timed out can therefore reach free_client() while revoke_delegation() is still about to dereference cl_lock, a use-after-free.

Pin the client with cl_rpc_users across the revoke so teardown blocks until it completes, then reap the delegation from cl_revoked. A client already expiring reaps its own, so skip it and leave the delegation on del_recall_lru.

Fixes 3bd64a5ba171 ("nfsd4: implement SEQ4_STATUS_RECALLABLE_STATE_REVOKED")
Reviewed-by NeilBrown <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/netns.h     |  6 ++++--
 fs/nfsd/nfs4state.c | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)
fs/nfsd/netns.h +4 -2
index 03724bef10a7..a7bd7b67fa4f 100644
@@ -115,7 +115,8 @@ struct nfsd_net {
 	struct list_head client_lru;
 	struct list_head close_lru;
 
-	/* protects del_recall_lru and delegation hash/unhash */
+	/* protects del_recall_lru and delegation hash/unhash;
+	 * nests outside client_lock */
 	spinlock_t deleg_lock ____cacheline_aligned;
 	struct list_head del_recall_lru;
 

@@ -124,7 +125,8 @@ struct nfsd_net {
 
 	struct delayed_work laundromat_work;
 
-	/* client_lock protects the client lru list and session hash table */
+	/* client_lock protects the client lru list and session hash
+	 * table; nests inside deleg_lock */
 	spinlock_t client_lock;
 
 	/* protects blocked_locks_lru */
fs/nfsd/nfs4state.c +23 -0
index e000ed3e96e9..efeb2a2e9c8f 100644
@@ -7457,6 +7457,7 @@ nfs4_laundromat(struct nfsd_net *nn)
 		.new_timeo = nn->nfsd4_lease
 	};
 	struct nfs4_cpntf_state *cps;
+	struct nfs4_client *clp;
 	copy_stateid_t *cps_t;
 	int i;
 

@@ -7485,6 +7486,18 @@ nfs4_laundromat(struct nfsd_net *nn)
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
 		if (!state_expired(&lt, dp->dl_time))
 			break;
+		clp = dp->dl_stid.sc_client;
+		spin_lock(&nn->client_lock);
+		if (is_client_expired(clp)) {
+			spin_unlock(&nn->client_lock);
+			continue;
+		}
+		/*
+		 * Pin without reviving: get_client_locked() would
+		 * flip a courtesy client back to NFSD4_ACTIVE.
+		 */
+		atomic_inc(&clp->cl_rpc_users);
+		spin_unlock(&nn->client_lock);
 		refcount_inc(&dp->dl_stid.sc_count);
 		unhash_delegation_locked(dp, SC_STATUS_REVOKED);
 		list_add(&dp->dl_recall_lru, &reaplist);

@@ -7493,8 +7506,18 @@ nfs4_laundromat(struct nfsd_net *nn)
 	while (!list_empty(&reaplist)) {
 		dp = list_first_entry(&reaplist, struct nfs4_delegation,
 					dl_recall_lru);
+		clp = dp->dl_stid.sc_client;
 		list_del_init(&dp->dl_recall_lru);
 		revoke_delegation(dp);
+		/*
+		 * Unpin without renewing: put_client_renew() would
+		 * renew the reaped client's lease.
+		 */
+		if (atomic_dec_and_lock(&clp->cl_rpc_users, &nn->client_lock)) {
+			if (is_client_expired(clp))
+				wake_up_all(&expiry_wq);
+			spin_unlock(&nn->client_lock);
+		}
 	}
 
 	spin_lock(&nn->client_lock);
Signature · 1 line
2.54.0
author maintainer
#3 lore raw

PATCH v3 3/9 NFSD: Prevent client use-after-free during admin state revocation

In reply to Chuck Lever

A stateid holds only a bare pointer to its nfs4_client; a stateid reference does not pin it. The client survives only because __destroy_client() drains its stateids before free_client() runs.

nfsd4_revoke_states() drops nn->client_lock across revoke_one_stid(), which dereferences the client to revoke a stateid and read clp->cl_minorversion. A teardown racing the dropped lock can free the client first.

Pinning cl_rpc_users under client_lock blocks the DESTROY_CLIENTID and EXCHANGE_ID teardown, which refuses while cl_rpc_users is non-zero. force_expire_client() ignores it: once its wait for cl_rpc_users to reach zero has passed, a later pin goes unnoticed.

Under client_lock, skip a client whose cl_time is already zero -- force_expire_client() clears it there before waiting -- otherwise pin cl_rpc_users before dropping the lock. The walk then either sees the expiry and skips, or pins in time for that wait to cover the revoke.

Fixes 1c13bf9f2e3c ("nfsd: allow lock state ids to be revoked and then freed")
Reviewed-by NeilBrown <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfs4state.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)
fs/nfsd/nfs4state.c +20 -2
index efeb2a2e9c8f..565fa2ff5ba5 100644
@@ -1965,9 +1965,19 @@ void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb)
 		struct nfs4_client *clp;
 	retry:
 		list_for_each_entry(clp, head, cl_idhash) {
-			struct nfs4_stid *stid = find_one_sb_stid(clp, sb,
-								  sc_types);
+			struct nfs4_stid *stid;
+
+			/*
+			 * force_expire_client() ignores cl_rpc_users once
+			 * its wait_event() has passed, so pinning cannot
+			 * keep an already-expiring client alive; the
+			 * expiry path revokes its states instead.
+			 */
+			if (is_client_expired(clp))
+				continue;
+			stid = find_one_sb_stid(clp, sb, sc_types);
 			if (stid) {
+				atomic_inc(&clp->cl_rpc_users);
 				spin_unlock(&nn->client_lock);
 				revoke_one_stid(nn, clp, stid);
 				nfs4_put_stid(stid);

@@ -1980,6 +1990,9 @@ void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb)
 					 */
 					nn->nfs40_last_revoke =
 						ktime_get_boottime_seconds();
+				if (atomic_dec_and_test(&clp->cl_rpc_users) &&
+				    is_client_expired(clp))
+					wake_up_all(&expiry_wq);
 				goto retry;
 			}
 		}

@@ -3394,6 +3407,11 @@ static void force_expire_client(struct nfs4_client *clp)
 
 	trace_nfsd_clid_admin_expired(&clp->cl_clientid);
 
+	/*
+	 * cl_time is cleared under client_lock before the wait so a
+	 * revocation walk pinning cl_rpc_users under it either skips
+	 * this client or is seen by this wait_event().
+	 */
 	spin_lock(&nn->client_lock);
 	clp->cl_time = 0;
 	spin_unlock(&nn->client_lock);
Signature · 1 line
2.54.0
author maintainer
#4 lore raw

PATCH v3 4/9 NFSD: Prevent client use-after-free during export state revocation

In reply to Chuck Lever

nfsd4_revoke_export_states() has the same use-after-free as nfsd4_revoke_states(): it drops nn->client_lock across revoke_one_stid() and the following read of clp->cl_minorversion, but the stateid reference it holds does not pin the client. A teardown racing the dropped lock can free the client while revoke_one_stid() still dereferences it.

exportfs -u drives this path through NFSD_CMD_UNLOCK_EXPORT, so an administrator removing an export can race a client expiry.

Skip a client that is already expiring and otherwise pin it with cl_rpc_users under client_lock before dropping the lock, matching nfsd4_revoke_states().

Fixes 2eac189bb059 ("NFSD: Add NFSD_CMD_UNLOCK_EXPORT netlink command")
Reviewed-by NeilBrown <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfs4state.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
fs/nfsd/nfs4state.c +10 -3
index 565fa2ff5ba5..9de535a2a4bb 100644
@@ -2055,10 +2055,14 @@ void nfsd4_revoke_export_states(struct nfsd_net *nn, const struct path *path)
 		struct nfs4_client *clp;
 	retry:
 		list_for_each_entry(clp, head, cl_idhash) {
-			struct nfs4_stid *stid = find_one_export_stid(
-							clp, path,
-							sc_types);
+			struct nfs4_stid *stid;
+
+			/* Skip or pin clp as in nfsd4_revoke_states(). */
+			if (is_client_expired(clp))
+				continue;
+			stid = find_one_export_stid(clp, path, sc_types);
 			if (stid) {
+				atomic_inc(&clp->cl_rpc_users);
 				spin_unlock(&nn->client_lock);
 				revoke_one_stid(nn, clp, stid);
 				nfs4_put_stid(stid);

@@ -2066,6 +2070,9 @@ void nfsd4_revoke_export_states(struct nfsd_net *nn, const struct path *path)
 				if (clp->cl_minorversion == 0)
 					nn->nfs40_last_revoke =
 						ktime_get_boottime_seconds();
+				if (atomic_dec_and_test(&clp->cl_rpc_users) &&
+				    is_client_expired(clp))
+					wake_up_all(&expiry_wq);
 				goto retry;
 			}
 		}
Signature · 1 line
2.54.0
author maintainer
#5 lore raw

PATCH v3 5/9 NFSD: Prevent client use-after-free during NFSv4.0 revoked-state cleanup

In reply to Chuck Lever

nfs40_clean_admin_revoked() takes a stateid reference under clp->cl_lock, drops nn->client_lock, and calls nfsd4_drop_revoked_stid(), which dereferences the stateid's client through s->sc_client->cl_lock. The stateid reference does not pin the client, so a teardown racing the dropped lock can free the client while nfsd4_drop_revoked_stid() is still using it.

This cleanup runs from the laundromat, so a periodic sweep can race force_expire_client() driven by a write to the clients/<id>/ctl file.

Skip a client that is already expiring and otherwise pin it with cl_rpc_users under client_lock before dropping the lock, matching nfsd4_revoke_states().

Fixes d688d8585e6b ("nfsd: allow admin-revoked NFSv4.0 state to be freed.")
Reviewed-by NeilBrown <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfs4state.c | 6 ++++++
 1 file changed, 6 insertions(+)
fs/nfsd/nfs4state.c +6 -0
index 9de535a2a4bb..445a0f40d5ff 100644
@@ -7452,16 +7452,22 @@ static void nfs40_clean_admin_revoked(struct nfsd_net *nn,
 
 		if (atomic_read(&clp->cl_admin_revoked) == 0)
 			continue;
+		if (is_client_expired(clp))
+			continue;
 
 		spin_lock(&clp->cl_lock);
 		idr_for_each_entry_ul(&clp->cl_stateids, stid, tmp, id)
 			if (stid->sc_status & SC_STATUS_ADMIN_REVOKED) {
 				refcount_inc(&stid->sc_count);
+				atomic_inc(&clp->cl_rpc_users);
 				spin_unlock(&nn->client_lock);
 				/* this function drops ->cl_lock */
 				nfsd4_drop_revoked_stid(stid);
 				nfs4_put_stid(stid);
 				spin_lock(&nn->client_lock);
+				if (atomic_dec_and_test(&clp->cl_rpc_users) &&
+				    is_client_expired(clp))
+					wake_up_all(&expiry_wq);
 				goto retry;
 			}
 		spin_unlock(&clp->cl_lock);
Signature · 1 line
2.54.0
author maintainer
#6 lore raw

PATCH v3 6/9 NFSD: Consolidate the revocation-path client unpin

In reply to Chuck Lever

The client use-after-free fixes in the state-revocation paths left four open-coded copies of one idiom: drop a cl_rpc_users pin without renewing the client's lease, waking force_expire_client() when the last pin drops on a client it is tearing down. The accompanying "do not renew" rationale was documented at only one of the four sites.

put_client_renew_locked() and put_client_renew() already carry the same pin-drop logic, but they renew a non-expired client's lease and so would resurrect the client whose state is being revoked. Factor the common pin-drop into __put_client_locked(), parameterized by whether to renew. The renew helpers pass true; the new put_client_no_renew_locked() and put_client_no_renew() pass false and carry the revocation paths, which must not revive the client they are tearing down. No change in behavior.

Reviewed-by NeilBrown <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfs4state.c | 69 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 28 deletions(-)
fs/nfsd/nfs4state.c +41 -28
index 445a0f40d5ff..142ba7d80539 100644
@@ -206,18 +206,28 @@ renew_client_locked(struct nfs4_client *clp)
 	clp->cl_state = NFSD4_ACTIVE;
 }
 
+/*
+ * Finish a cl_rpc_users unpin with the client_lock held. A
+ * revocation walk clears @renew so the client whose state it is
+ * revoking is not revived; every other caller renews the lease of
+ * a still-active client.
+ */
+static void __put_client_locked(struct nfs4_client *clp, bool renew)
+{
+	if (is_client_expired(clp))
+		wake_up_all(&expiry_wq);
+	else if (renew)
+		renew_client_locked(clp);
+}
+
 static void put_client_renew_locked(struct nfs4_client *clp)
 {
 	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 
 	lockdep_assert_held(&nn->client_lock);
 
-	if (!atomic_dec_and_test(&clp->cl_rpc_users))
-		return;
-	if (!is_client_expired(clp))
-		renew_client_locked(clp);
-	else
-		wake_up_all(&expiry_wq);
+	if (atomic_dec_and_test(&clp->cl_rpc_users))
+		__put_client_locked(clp, true);
 }
 
 static void put_client_renew(struct nfs4_client *clp)

@@ -226,10 +236,27 @@ static void put_client_renew(struct nfs4_client *clp)
 
 	if (!atomic_dec_and_lock(&clp->cl_rpc_users, &nn->client_lock))
 		return;
-	if (!is_client_expired(clp))
-		renew_client_locked(clp);
-	else
-		wake_up_all(&expiry_wq);
+	__put_client_locked(clp, true);
+	spin_unlock(&nn->client_lock);
+}
+
+static void put_client_no_renew_locked(struct nfs4_client *clp)
+{
+	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
+
+	lockdep_assert_held(&nn->client_lock);
+
+	if (atomic_dec_and_test(&clp->cl_rpc_users))
+		__put_client_locked(clp, false);
+}
+
+static void put_client_no_renew(struct nfs4_client *clp)
+{
+	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
+
+	if (!atomic_dec_and_lock(&clp->cl_rpc_users, &nn->client_lock))
+		return;
+	__put_client_locked(clp, false);
 	spin_unlock(&nn->client_lock);
 }
 

@@ -1990,9 +2017,7 @@ void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb)
 					 */
 					nn->nfs40_last_revoke =
 						ktime_get_boottime_seconds();
-				if (atomic_dec_and_test(&clp->cl_rpc_users) &&
-				    is_client_expired(clp))
-					wake_up_all(&expiry_wq);
+				put_client_no_renew_locked(clp);
 				goto retry;
 			}
 		}

@@ -2070,9 +2095,7 @@ void nfsd4_revoke_export_states(struct nfsd_net *nn, const struct path *path)
 				if (clp->cl_minorversion == 0)
 					nn->nfs40_last_revoke =
 						ktime_get_boottime_seconds();
-				if (atomic_dec_and_test(&clp->cl_rpc_users) &&
-				    is_client_expired(clp))
-					wake_up_all(&expiry_wq);
+				put_client_no_renew_locked(clp);
 				goto retry;
 			}
 		}

@@ -7465,9 +7488,7 @@ static void nfs40_clean_admin_revoked(struct nfsd_net *nn,
 				nfsd4_drop_revoked_stid(stid);
 				nfs4_put_stid(stid);
 				spin_lock(&nn->client_lock);
-				if (atomic_dec_and_test(&clp->cl_rpc_users) &&
-				    is_client_expired(clp))
-					wake_up_all(&expiry_wq);
+				put_client_no_renew_locked(clp);
 				goto retry;
 			}
 		spin_unlock(&clp->cl_lock);

@@ -7540,15 +7561,7 @@ nfs4_laundromat(struct nfsd_net *nn)
 		clp = dp->dl_stid.sc_client;
 		list_del_init(&dp->dl_recall_lru);
 		revoke_delegation(dp);
-		/*
-		 * Unpin without renewing: put_client_renew() would
-		 * renew the reaped client's lease.
-		 */
-		if (atomic_dec_and_lock(&clp->cl_rpc_users, &nn->client_lock)) {
-			if (is_client_expired(clp))
-				wake_up_all(&expiry_wq);
-			spin_unlock(&nn->client_lock);
-		}
+		put_client_no_renew(clp);
 	}
 
 	spin_lock(&nn->client_lock);
Signature · 1 line
2.54.0
author maintainer
#7 lore raw

PATCH v3 7/9 NFSD: Prevent client use-after-free during blocked-lock reaping

In reply to Chuck Lever

A bare lock owner -- its only remaining reference a blocked lock on nn->blocked_locks_lru -- holds a raw pointer to its nfs4_client but no reference keeping the client alive. When the per-net laundromat reaps such a lock, freeing the nbl drops the owner reference held through flc_owner, and the final nfs4_put_stateowner() takes the client's cl_lock. Because the laundromat detaches the nbl first, __destroy_client() no longer finds it, so a concurrent force_expire_client() can free the client before nfs4_put_stateowner() runs, dereferencing cl_lock in freed memory.

Pin the client with cl_rpc_users before dropping nn->blocked_locks_lock, and skip clients already expiring, whose blocked locks __destroy_client() frees while holding an owner reference. Take nn->client_lock outside nn->blocked_locks_lock. Every other site holds nn->blocked_locks_lock as a leaf, acquiring no further lock, so placing nn->client_lock outside it cannot form a lock-order cycle.

Fixes 7919d0a27f1e ("nfsd: add a LRU list for blocked locks")
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfs4state.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)
fs/nfsd/nfs4state.c +20 -3
index 142ba7d80539..4acd02f1642c 100644
@@ -357,6 +357,16 @@ free_blocked_lock(struct nfsd4_blocked_lock *nbl)
 	kref_put(&nbl->nbl_kref, free_nbl);
 }
 
+/* A blocked lock's flc_owner is its nfs4_lockowner. */
+static struct nfs4_client *
+nbl_client(struct nfsd4_blocked_lock *nbl)
+{
+	struct nfs4_lockowner *lo;
+
+	lo = (struct nfs4_lockowner *)nbl->nbl_lock.c.flc_owner;
+	return lo->lo_owner.so_client;
+}
+
 static void
 remove_blocked_locks(struct nfs4_lockowner *lo)
 {

@@ -7591,22 +7601,29 @@ nfs4_laundromat(struct nfsd_net *nn)
 	 * indefinitely once the lock does become free.
 	 */
 	BUG_ON(!list_empty(&reaplist));
+	spin_lock(&nn->client_lock);
 	spin_lock(&nn->blocked_locks_lock);
-	while (!list_empty(&nn->blocked_locks_lru)) {
-		nbl = list_first_entry(&nn->blocked_locks_lru,
-					struct nfsd4_blocked_lock, nbl_lru);
+	list_for_each_safe(pos, next, &nn->blocked_locks_lru) {
+		nbl = list_entry(pos, struct nfsd4_blocked_lock, nbl_lru);
 		if (!state_expired(&lt, nbl->nbl_time))
 			break;
+		clp = nbl_client(nbl);
+		if (is_client_expired(clp))
+			continue;
+		atomic_inc(&clp->cl_rpc_users);
 		list_move(&nbl->nbl_lru, &reaplist);
 		list_del_init(&nbl->nbl_list);
 	}
 	spin_unlock(&nn->blocked_locks_lock);
+	spin_unlock(&nn->client_lock);
 
 	while (!list_empty(&reaplist)) {
 		nbl = list_first_entry(&reaplist,
 					struct nfsd4_blocked_lock, nbl_lru);
+		clp = nbl_client(nbl);
 		list_del_init(&nbl->nbl_lru);
 		free_blocked_lock(nbl);
+		put_client_no_renew(clp);
 	}
 #ifdef CONFIG_NFSD_V4_2_INTER_SSC
 	/* service the server-to-server copy delayed unmount list */
Signature · 1 line
2.54.0
author maintainer
#8 lore raw

PATCH v3 8/9 NFSD: Prevent client use-after-free during close_lru reaping

In reply to Chuck Lever

An nfs4_openowner left on nn->close_lru after its final CLOSE keeps its last closed stateid in oo_last_closed_stid, holding only a raw pointer to its nfs4_client. The laundromat reaps timed-out entries, drops nn->client_lock, and calls nfs4_put_stid(), which dereferences the client through cl_lock. Nothing pins the client across that window, so a concurrent force_expire_client() can free it and nfs4_put_stid() reads freed memory. __destroy_client() hits the same race, walking clp->cl_openowners without cl_lock.

Pin the client with cl_rpc_users before dropping client_lock, and skip clients already expiring. __destroy_client() then cleans up its own close_lru entries through release_last_closed_stateid(), so teardown no longer races the laundromat.

Fixes 217526e7ecc9 ("nfsd: protect the close_lru list and oo_last_closed_stid with client_lock")
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfs4state.c | 5 +++++
 1 file changed, 5 insertions(+)
fs/nfsd/nfs4state.c +5 -0
index 4acd02f1642c..20556b8f186a 100644
@@ -7581,11 +7581,16 @@ nfs4_laundromat(struct nfsd_net *nn)
 		if (!state_expired(&lt, oo->oo_time))
 			break;
 		list_del_init(&oo->oo_close_lru);
+		clp = oo->oo_owner.so_client;
+		if (is_client_expired(clp))
+			continue;
 		stp = oo->oo_last_closed_stid;
 		oo->oo_last_closed_stid = NULL;
+		atomic_inc(&clp->cl_rpc_users);
 		spin_unlock(&nn->client_lock);
 		nfs4_put_stid(&stp->st_stid);
 		spin_lock(&nn->client_lock);
+		put_client_no_renew_locked(clp);
 	}
 	spin_unlock(&nn->client_lock);
Signature · 1 line
2.54.0
author maintainer
#9 lore raw

PATCH v3 9/9 NFSD: Release the export reference when reaping open stateids

In reply to Chuck Lever

nfs4_put_stid() releases the svc_export tracked in nfs4_stid.sc_export, but free_ol_stateid_reaplist() frees open and lock stateids by calling ->sc_free() directly, bypassing that path. An open stateid takes an sc_export reference in nfs4_open() and a lock stateid takes its own in init_lock_stateid(); both reach free_ol_stateid_reaplist() through their normal teardown, the open stateid via release_open_stateid() and the lock stateid via nfsd4_release_lockowner(), each through put_ol_stateid_locked(). The reference is therefore never dropped, pinning the export and blocking unmount for the lifetime of the stateid.

Release sc_export in free_ol_stateid_reaplist() the way nfs4_put_stid() does. ->sc_free() runs once per stateid, and a stateid reaches free_ol_stateid_reaplist() or nfs4_put_stid() but never both, so the reference is dropped exactly once. Revoked stateids reach this path with sc_export already cleared by drop_stid_export(), so they are skipped rather than double-freed.

Fixes ba0cde5dc81d ("NFSD: Track svc_export in nfs4_stid")
Signed-off-by Chuck Lever <[email protected]>
---
 fs/nfsd/nfs4state.c | 4 ++++
 1 file changed, 4 insertions(+)
fs/nfsd/nfs4state.c +4 -0
index 20556b8f186a..8e4cee571994 100644
@@ -1744,6 +1744,7 @@ static void
 free_ol_stateid_reaplist(struct list_head *reaplist)
 {
 	struct nfs4_ol_stateid *stp;
+	struct svc_export *exp;
 	struct nfs4_file *fp;
 
 	might_sleep();

@@ -1753,9 +1754,12 @@ free_ol_stateid_reaplist(struct list_head *reaplist)
 				       st_locks);
 		list_del(&stp->st_locks);
 		fp = stp->st_stid.sc_file;
+		exp = stp->st_stid.sc_export;
 		stp->st_stid.sc_free(&stp->st_stid);
 		if (fp)
 			put_nfs4_file(fp);
+		if (exp)
+			exp_put(exp);
 	}
 }
Signature · 1 line
2.54.0