Skip to contentSkip to conversation

NFSD: Fix UAFs in client teardown and state revocation

PATCH In review Chuck Lever 4 participants 18 messages Jul 9 – Jul 23

Subsystems (3)
lore
Share & tools
lore

Chuck Lever

author maintainer
#0 lore raw

v4 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 v4:
- Fix svc_export double put in nfs4_put_stid() (sashiko)
- Link to v3: https://patch.msgid.link/[email protected]

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]

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

---
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 | 149 ++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 131 insertions(+), 24 deletions(-)

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

Best regards,

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

PATCH v4 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.

---
 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
#2 lore raw

Re: [PATCH v4 1/9] NFSD: Prevent lock owner use-after-free during client teardown

Quoted 11 lines · Chuck Lever

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

[...]

Fixes 68ef3bc31664 ("nfsd: remove blocked locks on client teardown")

What's the plan to merge this change? From the outside it looks like a fix for a user reported regression from this merge window, as Wolfgang afaics blamed commit 516403d4d85607 ("Revert "NFSD: Defer sub-object cleanup in export put callbacks""), which also made it to various stable trees.

So shouldn't this change ideally be on it's way to Linus by now? Ideally with a stable tag to ensure quick backporting? Or is it not a good idea to have this in without the follow-up patches from this series?

Or did I get something totally wrong (which is easily possible from my external point)?

Ciao, Thorsten

Quoted 40 lines
---
 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);
author maintainer
#3 lore raw

Re: [PATCH v4 1/9] NFSD: Prevent lock owner use-after-free during client teardown

Quoted 19 lines · Thorsten Leemhuis
Quoted 11 lines · Chuck Lever

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

[...]

Fixes 68ef3bc31664 ("nfsd: remove blocked locks on client teardown")

What's the plan to merge this change? From the outside it looks like a fix for a user reported regression from this merge window, as Wolfgang afaics blamed commit 516403d4d85607 ("Revert "NFSD: Defer sub-object cleanup in export put callbacks""), which also made it to various stable trees.

Quoted 6 lines

So shouldn't this change ideally be on it's way to Linus by now? Ideally with a stable tag to ensure quick backporting? Or is it not a good idea to have this in without the follow-up patches from this series?

Or did I get something totally wrong (which is easily possible from my external point)?

Thorsten, thanks for your attention to this issue.

Here is the plan of record:

This commit is currently in nfsd-testing:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-testing&id=3055ac7c996369876342aa22a33bcc68cd4abca1

It will matriculate to nfsd-next in a week or so and then be sent to Linus in the initial nfsd-7.3 pull request.

A stable tag is on this commit, so it will get picked up by stable automation as soon as Linus merges it. That will include 7.2.y, which will still be open at that point.

Notes:

The Fixes tag refers to a commit in v4.16. That makes it technically a fix for a bug that was introduced not in the most recent merge window.

My general policy is to subject fixes to the same review and CI that is mandated for any other change regardless of urgency, since historically getting such fixes in quickly has been the source of defects in released kernels.

Both CI and review latency are subject to forces outside of my control so I leave a lengthy window for both.

Signature · 1 line
Chuck Lever
#4 lore raw

Re: [PATCH v4 1/9] NFSD: Prevent lock owner use-after-free during client teardown

Quoted 20 lines · Chuck Lever
Quoted 19 lines · Thorsten Leemhuis
Quoted 11 lines · Chuck Lever

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

[...]

Fixes 68ef3bc31664 ("nfsd: remove blocked locks on client teardown")

What's the plan to merge this change? From the outside it looks like a fix for a user reported regression from this merge window, as Wolfgang afaics blamed commit 516403d4d85607 ("Revert "NFSD: Defer sub-object cleanup in export put callbacks""), which also made it to various stable trees.

Quoted 8 lines
Quoted 6 lines

So shouldn't this change ideally be on it's way to Linus by now? Ideally with a stable tag to ensure quick backporting? Or is it not a good idea to have this in without the follow-up patches from this series?

Or did I get something totally wrong (which is easily possible from my external point)?

Thorsten, thanks for your attention to this issue.

Thank you for the answer!

Quoted 5 lines

Here is the plan of record:

This commit is currently in nfsd-testing:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-testing&id=3055ac7c996369876342aa22a33bcc68cd4abca1

Ahh, I see, the change gained a stable tag somewhere along the way, great!

Quoted 2 lines

It will matriculate to nfsd-next in a week or so and then be sent to Linus in the initial nfsd-7.3 pull request.

Hmmm. Just reread your reply to Wolfgang's report where you explain things in more detail:
https://lore.kernel.org/stable/[email protected]/

I now understand this better and it still feels to me like a situation where Linus would want the fix to be included in mainline quickly[1] -- among others to not have another release with the bug[2] and because it's something a user reported[3]. Yes, the root of the problem might be in an older commit the Fixes: tag quoted above refers to -- but apparently something (maybe the change Wolfgang and you suspected) seems to have exposed the problem or made it happen more often at least in 6.18.y.

Anyway, if Wolfgang is the only one that seems to hit this and is happy with the state of things I guess it's not worth arguing further about.

Thanks again for taking the time to answer my mail!

Ciao, Thorsten

[1] and Greg likely as well, as situations like that are the reason why stable kernels released during or shortly after the merge window have many hundreds of changes that have seen little testing (and we are currently at the point where it could get some testing before hitting a new release)

[2]
https://lore.kernel.org/all/CAHk-=wis_qQy4oDNynNKi5b7Qhosmxtoj1jxo5wmB6SRUwQUBQ@mail.gmail.com/

[3]
https://www.kernel.org/doc/html/latest/process/handling-regressions.html#on-how-quickly-regressions-should-be-fixed

#5 lore raw

Re: [PATCH v4 1/9] NFSD: Prevent lock owner use-after-free during client teardown

Hello Thorsten!

Am 2026-07-22 16:33, schrieb Thorsten Leemhuis:

Quoted 55 lines · Chuck Lever
Quoted 20 lines · Thorsten Leemhuis
Quoted 19 lines · Chuck Lever
Quoted 11 lines

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

[...]

Fixes 68ef3bc31664 ("nfsd: remove blocked locks on client teardown")

What's the plan to merge this change? From the outside it looks like a fix for a user reported regression from this merge window, as Wolfgang afaics blamed commit 516403d4d85607 ("Revert "NFSD: Defer sub-object cleanup in export put callbacks""), which also made it to various stable trees.

Quoted 8 lines
Quoted 6 lines

So shouldn't this change ideally be on it's way to Linus by now? Ideally with a stable tag to ensure quick backporting? Or is it not a good idea to have this in without the follow-up patches from this series?

Or did I get something totally wrong (which is easily possible from my external point)?

Thorsten, thanks for your attention to this issue.

Thank you for the answer!

Quoted 5 lines

Here is the plan of record:

This commit is currently in nfsd-testing:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-testing&id=3055ac7c996369876342aa22a33bcc68cd4abca1

Ahh, I see, the change gained a stable tag somewhere along the way, great!

Quoted 2 lines

It will matriculate to nfsd-next in a week or so and then be sent to Linus in the initial nfsd-7.3 pull request.

Hmmm. Just reread your reply to Wolfgang's report where you explain things in more detail:
https://lore.kernel.org/stable/[email protected]/

I now understand this better and it still feels to me like a situation where Linus would want the fix to be included in mainline quickly[1] -- among others to not have another release with the bug[2] and because it's something a user reported[3]. Yes, the root of the problem might be in an older commit the Fixes: tag quoted above refers to -- but apparently something (maybe the change Wolfgang and you suspected) seems to have exposed the problem or made it happen more often at least in 6.18.y.

Quoted 3 lines

Anyway, if Wolfgang is the only one that seems to hit this and is happy with the state of things I guess it's not worth arguing further about.

I'm very happy having this patch and especially that I got it so fast. If I build a new version of a stable kernel I just rebase this patch on top of it. Git makes carrying such patches very easy.

So I'm pefectly fine if this goes into stable/lts a bit later.

Quoted 10 lines

Thanks again for taking the time to answer my mail!

Ciao, Thorsten

[1] and Greg likely as well, as situations like that are the reason why stable kernels released during or shortly after the merge window have many hundreds of changes that have seen little testing (and we are currently at the point where it could get some testing before hitting a new release)

Quoted 5 lines

[2]
https://lore.kernel.org/all/CAHk-=wis_qQy4oDNynNKi5b7Qhosmxtoj1jxo5wmB6SRUwQUBQ@mail.gmail.com/

[3]
https://www.kernel.org/doc/html/latest/process/handling-regressions.html#on-how-quickly-regressions-should-be-fixed

Ciao,

Signature · 3 lines
Wolfgang Walter
Studierendenwerk München Oberbayern
Anstalt des öffentlichen Rechts
author maintainer
#6 lore raw

PATCH v4 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")
---
 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
#7 lore raw

PATCH v4 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")
---
 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
#8 lore raw

PATCH v4 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")
---
 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
#9 lore raw

PATCH v4 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.")
---
 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
#10 lore raw

PATCH v4 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.

---
 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
#11 lore raw

PATCH v4 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")
---
 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
maintainer Reviewed-by on v4 7/9
#12 lore raw

Re: [PATCH v4 7/9] NFSD: Prevent client use-after-free during blocked-lock reaping

Quoted 68 lines · 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")
---
 fs/nfsd/nfs4state.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)
fs/nfsd/nfs4state.c +18 -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);
 

fs/nfsd/nfs4state.c nfs4_laundromat(struct nfsd_net *nn) · line 7629

Quoted 10 lines
fs/nfsd/nfs4state.c +2 -0
 	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 */
author maintainer
#13 lore raw

PATCH v4 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")
---
 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
maintainer Reviewed-by on v4 8/9
#14 lore raw

Re: [PATCH v4 8/9] NFSD: Prevent client use-after-free during close_lru reaping

Quoted 31 lines · 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")
---
 fs/nfsd/nfs4state.c | 5 +++++
 1 file changed, 5 insertions(+)
fs/nfsd/nfs4state.c +3 -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;

fs/nfsd/nfs4state.c nfs4_laundromat(struct nfsd_net *nn) · line 7596

Quoted 10 lines
fs/nfsd/nfs4state.c +2 -0
 		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);
 
author maintainer
#15 lore raw

PATCH v4 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.

nfs4_put_stid() itself read sc_export before acquiring cl_lock. drop_stid_export() clears that field and releases the reference under cl_lock, so a concurrent revocation could drop the export in the window between the read and the final put, releasing the same reference twice. Read sc_export while cl_lock is held so the two paths serialize and the reference is released exactly once.

Fixes ba0cde5dc81d ("NFSD: Track svc_export in nfs4_stid")
---
 fs/nfsd/nfs4state.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
fs/nfsd/nfs4state.c +7 -1
index 20556b8f186a..e988dfebf75e 100644
@@ -1272,9 +1272,9 @@ alloc_init_dir_deleg(struct nfs4_client *clp, struct nfs4_file *fp)
 void
 nfs4_put_stid(struct nfs4_stid *s)
 {
-	struct svc_export *exp = s->sc_export;
 	struct nfs4_file *fp = s->sc_file;
 	struct nfs4_client *clp = s->sc_client;
+	struct svc_export *exp;
 
 	might_lock(&clp->cl_lock);
 
@@ -1285,6 +1285,8 @@ nfs4_put_stid(struct nfs4_stid *s)
 	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
 	if (s->sc_status & SC_STATUS_ADMIN_REVOKED)
 		atomic_dec(&s->sc_client->cl_admin_revoked);
+	/* Read under cl_lock to serialize with drop_stid_export(). */
+	exp = s->sc_export;
 	nfs4_free_cpntf_statelist(clp->net, s);
 	spin_unlock(&clp->cl_lock);
 	s->sc_free(s);
@@ -1744,6 +1746,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 +1756,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
maintainer Reviewed-by on v4 9/9
#16 lore raw

Re: [PATCH v4 9/9] NFSD: Release the export reference when reaping open stateids

Quoted 66 lines · 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.

nfs4_put_stid() itself read sc_export before acquiring cl_lock. drop_stid_export() clears that field and releases the reference under cl_lock, so a concurrent revocation could drop the export in the window between the read and the final put, releasing the same reference twice. Read sc_export while cl_lock is held so the two paths serialize and the reference is released exactly once.

Fixes ba0cde5dc81d ("NFSD: Track svc_export in nfs4_stid")
---
 fs/nfsd/nfs4state.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
fs/nfsd/nfs4state.c +4 -1
index 20556b8f186a..e988dfebf75e 100644
@@ -1272,9 +1272,9 @@ alloc_init_dir_deleg(struct nfs4_client *clp, struct nfs4_file *fp)
 void
 nfs4_put_stid(struct nfs4_stid *s)
 {
-	struct svc_export *exp = s->sc_export;
 	struct nfs4_file *fp = s->sc_file;
 	struct nfs4_client *clp = s->sc_client;
+	struct svc_export *exp;
 
 	might_lock(&clp->cl_lock);
 
@@ -1285,6 +1285,8 @@ nfs4_put_stid(struct nfs4_stid *s)
 	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
 	if (s->sc_status & SC_STATUS_ADMIN_REVOKED)
 		atomic_dec(&s->sc_client->cl_admin_revoked);
+	/* Read under cl_lock to serialize with drop_stid_export(). */
+	exp = s->sc_export;
 	nfs4_free_cpntf_statelist(clp->net, s);
 	spin_unlock(&clp->cl_lock);
 	s->sc_free(s);
@@ -1744,6 +1746,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();

fs/nfsd/nfs4state.c free_ol_stateid_reaplist(struct list_head *reaplist) · line 1764

Quoted 9 lines
fs/nfsd/nfs4state.c +3 -0
@@ -1753,9 +1756,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);

nit: nfs4_put_stid() does this in the reverse order. It doesn't actually matter, but it looks a bit weird if you want to fix it up before merging.

fs/nfsd/nfs4state.c free_ol_stateid_reaplist(struct list_head *reaplist) · line 1767

Quoted 3 lines
fs/nfsd/nfs4state.c +0 -0
 	}
 }
 
author maintainer
#17 lore raw

Re: [PATCH v4 9/9] NFSD: Release the export reference when reaping open stateids

Quoted 54 lines · Jeff Layton
Quoted 39 lines · 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.

nfs4_put_stid() itself read sc_export before acquiring cl_lock. drop_stid_export() clears that field and releases the reference under cl_lock, so a concurrent revocation could drop the export in the window between the read and the final put, releasing the same reference twice. Read sc_export while cl_lock is held so the two paths serialize and the reference is released exactly once.

Fixes ba0cde5dc81d ("NFSD: Track svc_export in nfs4_stid")
---
 fs/nfsd/nfs4state.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
fs/nfsd/nfs4state.c +0 -0
index 20556b8f186a..e988dfebf75e 100644

fs/nfsd/nfs4state.c free_ol_stateid_reaplist(struct list_head *reaplist) · line 1764

Quoted 9 lines
fs/nfsd/nfs4state.c +3 -0
@@ -1753,9 +1756,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);

nit: nfs4_put_stid() does this in the reverse order. It doesn't actually matter, but it looks a bit weird if you want to fix it up before merging.

Quoted 5 lines

fs/nfsd/nfs4state.c free_ol_stateid_reaplist(struct list_head *reaplist) · line 1767

Quoted 3 lines
fs/nfsd/nfs4state.c +0 -0
 	}
 }
 

Swap applied, series pushed to nfsd-testing. Thanks for your review!

Signature · 1 line
Chuck Lever