[PATCH v2 0/6] NFSD: Fix UAFs in client teardown and state revocation

[PATCH v2 0/6] NFSD: Fix UAFs in client teardown and state revocation

[Chuck Lever]

A NULL-pointer dereference reported during NFSv4 client teardown
(patch 1) proved to be one instance of a broader lifetime bug in
NFSD's state-revocation machinery. This series fixes the reported
crash and the sibling races found by auditing the same pattern, then
consolidates the fixes.

A stateid, and a bare lock owner reachable through the client's owner
hash, hold only a raw pointer to the owning nfs4_client; a reference
on the stateid or owner does not keep the client alive. The client
outlives its state solely because __destroy_client() drains that state
before free_client() runs. Several paths break that invariant. The
laundromat unhashes an expired delegation before revoke_delegation()
re-links it, leaving it momentarily on no client-reachable list
(patch 2). nfsd4_revoke_states() and its export and NFSv4.0
admin-revoke siblings drop nn->client_lock and then dereference the
client again (patches 3-5). __destroy_client() walks the owner hash
and frees blocked locks with no reference held (patch 1).

---
Changes since v1:
- Add matching UAF fixes in several other paths

---
Chuck Lever (6):
      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

 fs/nfsd/netns.h     |   6 ++-
 fs/nfsd/nfs4state.c | 108 +++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 94 insertions(+), 20 deletions(-)
---
base-commit: ee6ae4a6bf3565b880dfb420017337475dfbc9ea
change-id: 20260705-cel-61c1c70caa03

Best regards,
--  
Chuck Lever

[PATCH v2 1/6] NFSD: Prevent lock owner use-after-free during client teardown

[Chuck Lever]

After __destroy_client() releases a client's open owners, a lock owner
whose only remaining reference is a blocked lock (nbl) is left on
cl_ownerstr_hashtbl.  client_has_state() does not account for a bare
lock owner, so DESTROY_CLIENTID can reach __destroy_client() with these
lock owners still present.

__destroy_client() then walks cl_ownerstr_hashtbl and calls
remove_blocked_locks() on each lock owner without holding a reference.
Freeing a blocked lock drops the lock owner reference held through the
file_lock's flc_owner, so the per-net laundromat, which reaps timed-out
blocked locks from nn->blocked_locks_lru independently of client state,
can free the same lock owner concurrently.  The two paths serialize on
blocked_locks_lock for the list splice only, not for the lock owner's
lifetime.  The laundromat can therefore free the lock owner while
__destroy_client() is about to dereference it, and the freed, zeroed
slab object produces 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, and
take a reference and unhash each lock owner before dropping the
lock, so the laundromat cannot reap a blocked lock and free the
lock owner underneath this loop.  cl_lock is released before
remove_blocked_locks() and nfs4_put_stateowner(), which take
blocked_locks_lock and cl_lock respectively.

Reported-by: Wolfgang Walter <linux@stwm.de>
Closes: https://lore.kernel.org/linux-nfs/6eccafaaaa60651ef091257c3439c46b@stwm.de/
Fixes: 68ef3bc31664 ("nfsd: remove blocked locks on client teardown")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 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 a4398dc861a5..e000ed3e96e9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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);

-- 
2.54.0

[PATCH v2 2/6] NFSD: Prevent client use-after-free during delegation revoke

[Chuck Lever]

A delegation stateid stores only a bare pointer to its owning
nfs4_client; a reference on the stateid does not keep the client
alive.  The client outlives its stateids solely because
__destroy_client() drains every delegation from cl_delegations and
cl_revoked before free_client() runs.

nfs4_laundromat() breaks that invariant.  It unhashes an expired
delegation from cl_delegations under deleg_lock, drops the lock, and
calls revoke_delegation(), which reacquires clp->cl_lock and links the
delegation onto clp->cl_revoked.  Between the unhash and the revoke the
delegation is on neither client-reachable list, so client_has_state()
can report no remaining state.

Every path that can free a client holding delegations first requires
cl_rpc_users to be zero: DESTROY_CLIENTID and a superseding EXCHANGE_ID
gate on mark_client_expired_locked(), and the admin "expire" write
waits in force_expire_client().  The laundromat holds no such
reference.  A client whose recalled delegation has just timed out -- a
rebooted client whose new incarnation supersedes the old one, say --
can therefore run __destroy_client() to completion and free the client
while revoke_delegation() is still about to dereference clp->cl_lock
and clp->cl_revoked, a use-after-free.

Pin the client with cl_rpc_users across the revoke so the teardown
paths block until it completes and then reap it from cl_revoked
themselves.  A client already expiring reaps its own delegations, so
skip it and leave the delegation on del_recall_lru for
__destroy_client() to handle.

Fixes: 3bd64a5ba171 ("nfsd4: implement SEQ4_STATUS_RECALLABLE_STATE_REVOKED")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 fs/nfsd/netns.h     |  6 ++++--
 fs/nfsd/nfs4state.c | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 03724bef10a7..a7bd7b67fa4f 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -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 */
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e000ed3e96e9..efeb2a2e9c8f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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);

-- 
2.54.0

Re: [PATCH v2 2/6] NFSD: Prevent client use-after-free during delegation revoke

[Jeff Layton]

On Sun, 2026-07-05 at 21:25 -0400, Chuck Lever wrote:
> A delegation stateid stores only a bare pointer to its owning
> nfs4_client; a reference on the stateid does not keep the client
> alive.  The client outlives its stateids solely because
> __destroy_client() drains every delegation from cl_delegations and
> cl_revoked before free_client() runs.
> 
> nfs4_laundromat() breaks that invariant.  It unhashes an expired
> delegation from cl_delegations under deleg_lock, drops the lock, and
> calls revoke_delegation(), which reacquires clp->cl_lock and links the
> delegation onto clp->cl_revoked.  Between the unhash and the revoke the
> delegation is on neither client-reachable list, so client_has_state()
> can report no remaining state.
> 
> Every path that can free a client holding delegations first requires
> cl_rpc_users to be zero: DESTROY_CLIENTID and a superseding EXCHANGE_ID
> gate on mark_client_expired_locked(), and the admin "expire" write
> waits in force_expire_client().  The laundromat holds no such
> reference.  A client whose recalled delegation has just timed out -- a
> rebooted client whose new incarnation supersedes the old one, say --
> can therefore run __destroy_client() to completion and free the client
> while revoke_delegation() is still about to dereference clp->cl_lock
> and clp->cl_revoked, a use-after-free.
> 
> Pin the client with cl_rpc_users across the revoke so the teardown
> paths block until it completes and then reap it from cl_revoked
> themselves.  A client already expiring reaps its own delegations, so
> skip it and leave the delegation on del_recall_lru for
> __destroy_client() to handle.
> 
> Fixes: 3bd64a5ba171 ("nfsd4: implement SEQ4_STATUS_RECALLABLE_STATE_REVOKED")
> Signed-off-by: Chuck Lever <cel@kernel.org>
> ---
>  fs/nfsd/netns.h     |  6 ++++--
>  fs/nfsd/nfs4state.c | 23 +++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 03724bef10a7..a7bd7b67fa4f 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -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 */
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e000ed3e96e9..efeb2a2e9c8f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -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);

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

[PATCH v2 3/6] NFSD: Prevent client use-after-free during admin state revocation

[Chuck Lever]

A stateid stores only a bare pointer to its owning nfs4_client; a
reference on the stateid does not keep the client alive.  The client
outlives its stateids only because __destroy_client() drains them
before free_client() runs.

nfsd4_revoke_states() drops nn->client_lock across revoke_one_stid(),
which revokes the delegation, open, lock, or layout stateid and
dereferences its client -- revoke_delegation() and revoke_ol_stid()
both reacquire clp->cl_lock.  The stateid reference it holds does not
pin the client, so a teardown racing the dropped lock can run
__destroy_client() and free the client first.  The subsequent read of
clp->cl_minorversion is exposed the same way.

A DESTROY_CLIENTID or superseding EXCHANGE_ID reaches teardown through
mark_client_expired_locked(), which refuses while cl_rpc_users is
non-zero, so pinning the client under client_lock holds those paths
off.  force_expire_client() does not consult cl_rpc_users: it zeroes
cl_time under client_lock, waits once for cl_rpc_users to reach zero,
then unhashes and frees.  A pin taken after that wait goes unnoticed,
so the walk must also skip a client that is already expiring.

Under client_lock, skip a client whose cl_time is zero and otherwise
pin it with cl_rpc_users before dropping the lock.  cl_time is set
under the same lock, so the walk either sees the expiry and skips, or
pins early enough that force_expire_client() waits for the revoke and
the cl_minorversion update to finish.

Fixes: 1c13bf9f2e3c ("nfsd: allow lock state ids to be revoked and then freed")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 fs/nfsd/nfs4state.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index efeb2a2e9c8f..cdb62b3bf718 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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;
 			}
 		}

-- 
2.54.0

[PATCH v2 4/6] NFSD: Prevent client use-after-free during export state revocation

[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")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 fs/nfsd/nfs4state.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index cdb62b3bf718..c7db2c249441 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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;
 			}
 		}

-- 
2.54.0

[PATCH v2 5/6] NFSD: Prevent client use-after-free during NFSv4.0 revoked-state cleanup

[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.")
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 fs/nfsd/nfs4state.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c7db2c249441..e80d4f97f020 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7447,16 +7447,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);

-- 
2.54.0

[PATCH v2 6/6] NFSD: Consolidate the revocation-path client unpin

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

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 fs/nfsd/nfs4state.c | 69 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 28 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e80d4f97f020..53d123ae7965 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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;
 			}
 		}
@@ -7460,9 +7483,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);
@@ -7535,15 +7556,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);

-- 
2.54.0

Re: [PATCH v2 0/6] NFSD: Fix UAFs in client teardown and state revocation

[NeilBrown]

On Mon, 06 Jul 2026, Chuck Lever wrote:
> A NULL-pointer dereference reported during NFSv4 client teardown
> (patch 1) proved to be one instance of a broader lifetime bug in
> NFSD's state-revocation machinery. This series fixes the reported
> crash and the sibling races found by auditing the same pattern, then
> consolidates the fixes.
> 
> A stateid, and a bare lock owner reachable through the client's owner
> hash, hold only a raw pointer to the owning nfs4_client; a reference
> on the stateid or owner does not keep the client alive. The client
> outlives its state solely because __destroy_client() drains that state
> before free_client() runs. Several paths break that invariant. The
> laundromat unhashes an expired delegation before revoke_delegation()
> re-links it, leaving it momentarily on no client-reachable list
> (patch 2). nfsd4_revoke_states() and its export and NFSv4.0
> admin-revoke siblings drop nn->client_lock and then dereference the
> client again (patches 3-5). __destroy_client() walks the owner hash
> and frees blocked locks with no reference held (patch 1).

Apart from one issue already mentioned in the first patch - which maybe
should be fixed with a separate patch - these all look good.  Thanks,

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

NeilBrown


> 
> ---
> Changes since v1:
> - Add matching UAF fixes in several other paths
> 
> ---
> Chuck Lever (6):
>       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
> 
>  fs/nfsd/netns.h     |   6 ++-
>  fs/nfsd/nfs4state.c | 108 +++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 94 insertions(+), 20 deletions(-)
> ---
> base-commit: ee6ae4a6bf3565b880dfb420017337475dfbc9ea
> change-id: 20260705-cel-61c1c70caa03
> 
> Best regards,
> --  
> Chuck Lever
> 
> 

Re: [PATCH v2 0/6] NFSD: Fix UAFs in client teardown and state revocation

[Jeff Layton]

On Sun, 2026-07-05 at 21:25 -0400, Chuck Lever wrote:
> A NULL-pointer dereference reported during NFSv4 client teardown
> (patch 1) proved to be one instance of a broader lifetime bug in
> NFSD's state-revocation machinery. This series fixes the reported
> crash and the sibling races found by auditing the same pattern, then
> consolidates the fixes.
> 
> A stateid, and a bare lock owner reachable through the client's owner
> hash, hold only a raw pointer to the owning nfs4_client; a reference
> on the stateid or owner does not keep the client alive. The client
> outlives its state solely because __destroy_client() drains that state
> before free_client() runs. Several paths break that invariant. The
> laundromat unhashes an expired delegation before revoke_delegation()
> re-links it, leaving it momentarily on no client-reachable list
> (patch 2). nfsd4_revoke_states() and its export and NFSv4.0
> admin-revoke siblings drop nn->client_lock and then dereference the
> client again (patches 3-5). __destroy_client() walks the owner hash
> and frees blocked locks with no reference held (patch 1).
> 
> ---
> Changes since v1:
> - Add matching UAF fixes in several other paths
> 
> ---
> Chuck Lever (6):
>       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
> 
>  fs/nfsd/netns.h     |   6 ++-
>  fs/nfsd/nfs4state.c | 108 +++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 94 insertions(+), 20 deletions(-)
> ---
> base-commit: ee6ae4a6bf3565b880dfb420017337475dfbc9ea
> change-id: 20260705-cel-61c1c70caa03
> 
> Best regards,
> --  
> Chuck Lever

This all looks pretty good, aside from patch #2 which seems like it
might cause an ABBA deadlock (according to Sashiko).

You can add this to patches 1 and 3-5 though:

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

Re: [PATCH v2 0/6] NFSD: Fix UAFs in client teardown and state revocation

[Chuck Lever]

On Mon, Jul 6, 2026, at 12:42 PM, Jeff Layton wrote:
> On Sun, 2026-07-05 at 21:25 -0400, Chuck Lever wrote:
>> A NULL-pointer dereference reported during NFSv4 client teardown
>> (patch 1) proved to be one instance of a broader lifetime bug in
>> NFSD's state-revocation machinery. This series fixes the reported
>> crash and the sibling races found by auditing the same pattern, then
>> consolidates the fixes.
>> 
>> A stateid, and a bare lock owner reachable through the client's owner
>> hash, hold only a raw pointer to the owning nfs4_client; a reference
>> on the stateid or owner does not keep the client alive. The client
>> outlives its state solely because __destroy_client() drains that state
>> before free_client() runs. Several paths break that invariant. The
>> laundromat unhashes an expired delegation before revoke_delegation()
>> re-links it, leaving it momentarily on no client-reachable list
>> (patch 2). nfsd4_revoke_states() and its export and NFSv4.0
>> admin-revoke siblings drop nn->client_lock and then dereference the
>> client again (patches 3-5). __destroy_client() walks the owner hash
>> and frees blocked locks with no reference held (patch 1).
>> 
>> ---
>> Changes since v1:
>> - Add matching UAF fixes in several other paths
>> 
>> ---
>> Chuck Lever (6):
>>       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
>> 
>>  fs/nfsd/netns.h     |   6 ++-
>>  fs/nfsd/nfs4state.c | 108 +++++++++++++++++++++++++++++++++++++++++++---------
>>  2 files changed, 94 insertions(+), 20 deletions(-)
>> ---
>> base-commit: ee6ae4a6bf3565b880dfb420017337475dfbc9ea
>> change-id: 20260705-cel-61c1c70caa03
>> 
>> Best regards,
>> --  
>> Chuck Lever
>
> This all looks pretty good, aside from patch #2 which seems like it
> might cause an ABBA deadlock (according to Sashiko).

Reconfirmed that Sashiko's finding was a false positive.


> You can add this to patches 1 and 3-5 though:
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Thanks!


-- 
Chuck Lever