[PATCH v2] nfsd: drain pNFS fence work during state teardown

[PATCH v2] nfsd: drain pNFS fence work during state teardown

[Jinpyo Lee]

The pNFS layout-recall timeout path holds a layout stateid reference
while a delayed fence worker runs and retries. A stateid reference does
not retain sc_client, so client expiry can free the nfs4_client while
the worker still uses client-owned state and eventually releases the
stateid through the stale client pointer.

Keeping only the client alive is not sufficient. The worker also uses
clp->net, and neither the stateid nor a client reference prevents NFSD
per-net state or module code from being torn down while a retry is
pending.

Prevent new fence work from being scheduled once layout teardown starts,
and synchronously cancel or drain an existing worker before releasing
the layout's client association. Check the stop state under ls_lock at
both the initial scheduling point and the retry point so cancellation
cannot miss a newly queued retry. Serialize teardown callers so only one
of them disposes of the worker-owned stateid reference.

The worker clears ls_fence_inflight before its final nfs4_put_stid(), so
that flag alone does not prove the worker has finished. Always synchronize
with the delayed work during teardown, even when the flag is already
clear. Release the worker-owned stateid reference in the teardown path
only when pending work was actually canceled.

nfsd4_return_all_client_layouts() is reached from __destroy_client(), so
the drain covers administrator expiry, laundromat expiry, client
replacement during CREATE_SESSION, and nfs4_state_shutdown_net().
Stopping revoked layout stateids separately also prevents a worker from
escaping the client list before shutdown. Module unload reaches the same
per-net state shutdown path before the pNFS caches and module text are
released.

The reproducer uses a pNFS SCSI export and a failed storage fence to keep
the retry pending. A source reproducer and the complete KASAN log are
available privately on request.

The original KASAN use-after-free was reproduced in nfs4_put_stid() on
the unpatched nfsd-testing tree. The patched x86_64 kernel was built
and boot-tested with Generic KASAN, lockdep, modular NFSD, and pNFS block
and SCSI layout support. Targeted runs covered administrator expiry,
natural lease expiry followed by courtesy-client shrinker reclamation,
CREATE_SESSION replacement, per-net shutdown, module unload, retry, and
successful storage fencing. No KASAN, lockdep, or refcount report occurred.

Named-netns removal followed by explicit NFSD shutdown and final
namespace exit also produced no KASAN, lockdep, or refcount report.
A test-only run confirmed teardown waited for the worker's final
stateid release.

Basic NFSv4.2 and NFSv3 read/write/unmount smoke tests passed. The full
kernel and modules were built with GCC 13.3 without new compiler warnings.

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

Fixes: f52792f484ba ("NFSD: Enforce timeout on layout recall and integrate lease manager fencing")
Assisted-by: LLM
Signed-off-by: Jinpyo Lee <bint4b13@gmail.com>
---
Changes in v2:
- Replace client pinning with synchronized fence-work shutdown that blocks
  initial scheduling and retries and waits for the final stateid release.
- Handle revoked layout stateids and client, per-net, and module teardown.
- Describe targeted runtime validation and its limits.

Validation details for reviewers:

- NFSD holds a network namespace reference until nfs4_state_destroy_net(),
  after client teardown has drained fence work. Removing a namespace name
  during a pending retry did not destroy it; explicit rpc.nfsd 0 drained
  the worker before final namespace exit. This does not test namespace
  destruction while the worker runs.
- Test-only instrumentation widened the interval between clearing
  ls_fence_inflight and the final nfs4_put_stid(). During expiry,
  cancel_delayed_work_sync() waited for worker completion (2.027 seconds).
  The test-only changes are not in this patch.
- Matching unpatched runs also logged GETDEVICEINFO nfserrno() warnings
  (values 2 and 917504). This patch does not modify that path.

 fs/nfsd/nfs4layouts.c | 106 +++++++++++++++++++++++++++++++++---------
 fs/nfsd/nfs4state.c   |   2 +
 fs/nfsd/pnfs.h        |   5 ++
 fs/nfsd/state.h       |   2 +
 4 files changed, 94 insertions(+), 21 deletions(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 12acb68cb..7ac379fd5 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -246,6 +246,7 @@ nfsd4_alloc_layout_stateid(struct nfsd4_compound_state *cstate,
 	spin_lock_init(&ls->ls_lock);
 	INIT_LIST_HEAD(&ls->ls_layouts);
 	mutex_init(&ls->ls_mutex);
+	mutex_init(&ls->ls_fence_mutex);
 	ls->ls_layout_type = layout_type;
 	nfsd4_init_cb(&ls->ls_recall, clp, &nfsd4_cb_layout_ops,
 			NFSPROC4_CLNT_CB_LAYOUT);
@@ -265,6 +266,7 @@ nfsd4_alloc_layout_stateid(struct nfsd4_compound_state *cstate,
 
 	ls->ls_fenced = false;
 	ls->ls_fence_inflight = false;
+	ls->ls_fence_stopped = false;
 	ls->ls_fence_delay = 0;
 	INIT_DELAYED_WORK(&ls->ls_fence_work, nfsd4_layout_fence_worker);
 
@@ -602,13 +604,35 @@ nfsd4_return_all_layouts(struct nfs4_layout_stateid *ls,
 void
 nfsd4_return_all_client_layouts(struct nfs4_client *clp)
 {
-	struct nfs4_layout_stateid *ls, *n;
+	struct nfs4_layout_stateid *ls;
 	LIST_HEAD(reaplist);
 
-	spin_lock(&clp->cl_lock);
-	list_for_each_entry_safe(ls, n, &clp->cl_lo_states, ls_perclnt)
+	/*
+	 * A fence worker dereferences sc_client and clp->net.  Drain every
+	 * worker before client or per-net state can be released.  Take a
+	 * temporary stateid reference because stopping a worker can sleep.
+	 */
+	for (;;) {
+		spin_lock(&clp->cl_lock);
+		ls = list_first_entry_or_null(&clp->cl_lo_states,
+					      struct nfs4_layout_stateid,
+					      ls_perclnt);
+		if (!ls) {
+			spin_unlock(&clp->cl_lock);
+			break;
+		}
+		if (!refcount_inc_not_zero(&ls->ls_stid.sc_count)) {
+			spin_unlock(&clp->cl_lock);
+			cond_resched();
+			continue;
+		}
+		list_del_init(&ls->ls_perclnt);
+		spin_unlock(&clp->cl_lock);
+
+		nfsd4_stop_layout_fence(ls);
 		nfsd4_return_all_layouts(ls, &reaplist);
-	spin_unlock(&clp->cl_lock);
+		nfs4_put_stid(&ls->ls_stid);
+	}
 
 	nfsd4_free_layouts(&reaplist);
 }
@@ -792,8 +816,47 @@ nfsd4_layout_lm_open_conflict(struct file *filp, int arg)
 	return 0;
 }
 
-static void
-nfsd4_layout_fence_worker(struct work_struct *work)
+static void nfsd4_layout_fence_done(struct nfs4_layout_stateid *ls)
+{
+	/* Unlock the lease so that tasks waiting on it can proceed. */
+	nfsd4_close_layout(ls);
+
+	spin_lock(&ls->ls_lock);
+	ls->ls_fenced = true;
+	ls->ls_fence_inflight = false;
+	spin_unlock(&ls->ls_lock);
+	nfs4_put_stid(&ls->ls_stid);
+}
+
+void nfsd4_stop_layout_fence(struct nfs4_layout_stateid *ls)
+{
+	/* Serialize teardown callers which can arrive through different paths. */
+	mutex_lock(&ls->ls_fence_mutex);
+	spin_lock(&ls->ls_lock);
+	if (ls->ls_fence_stopped) {
+		spin_unlock(&ls->ls_lock);
+		mutex_unlock(&ls->ls_fence_mutex);
+		return;
+	}
+	ls->ls_fence_stopped = true;
+	spin_unlock(&ls->ls_lock);
+
+	/*
+	 * The worker clears ls_fence_inflight before its final nfs4_put_stid().
+	 * Always wait for it, even if that flag is already clear.  New work
+	 * cannot be queued after ls_fence_stopped is set under ls_lock.
+	 * If pending work was canceled, release its stateid reference here.
+	 */
+	if (cancel_delayed_work_sync(&ls->ls_fence_work))
+		nfsd4_layout_fence_done(ls);
+
+	spin_lock(&ls->ls_lock);
+	WARN_ON_ONCE(ls->ls_fence_inflight);
+	spin_unlock(&ls->ls_lock);
+	mutex_unlock(&ls->ls_fence_mutex);
+}
+
+static void nfsd4_layout_fence_worker(struct work_struct *work)
 {
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct nfs4_layout_stateid *ls = container_of(dwork,
@@ -804,18 +867,10 @@ nfsd4_layout_fence_worker(struct work_struct *work)
 	struct nfsd_net *nn;
 
 	spin_lock(&ls->ls_lock);
-	if (list_empty(&ls->ls_layouts)) {
+	if (ls->ls_fence_stopped || list_empty(&ls->ls_layouts)) {
 		spin_unlock(&ls->ls_lock);
 dispose:
-		cancel_delayed_work(&ls->ls_fence_work);
-		/* unlock the lease so that tasks waiting on it can proceed */
-		nfsd4_close_layout(ls);
-
-		ls->ls_fenced = true;
-		spin_lock(&ls->ls_lock);
-		ls->ls_fence_inflight = false;
-		spin_unlock(&ls->ls_lock);
-		nfs4_put_stid(&ls->ls_stid);
+		nfsd4_layout_fence_done(ls);
 		return;
 	}
 	spin_unlock(&ls->ls_lock);
@@ -862,12 +917,19 @@ dispose:
 	 *    clid: is the unique client identifier displayed in
 	 *          the warning message above.
 	 */
+	spin_lock(&ls->ls_lock);
+	if (ls->ls_fence_stopped || list_empty(&ls->ls_layouts)) {
+		spin_unlock(&ls->ls_lock);
+		goto dispose;
+	}
 	if (!ls->ls_fence_delay)
 		ls->ls_fence_delay = HZ;
 	else
 		ls->ls_fence_delay = min(ls->ls_fence_delay << 1,
 					 MAX_FENCE_DELAY);
-	mod_delayed_work(system_dfl_wq, &ls->ls_fence_work, ls->ls_fence_delay);
+	mod_delayed_work(system_dfl_wq, &ls->ls_fence_work,
+			 ls->ls_fence_delay);
+	spin_unlock(&ls->ls_lock);
 }
 
 /**
@@ -897,8 +959,7 @@ nfsd4_layout_lm_breaker_timedout(struct file_lease *fl)
 {
 	struct nfs4_layout_stateid *ls = fl->c.flc_owner;
 
-	if ((!nfsd4_layout_ops[ls->ls_layout_type]->fence_client) ||
-			ls->ls_fenced)
+	if (!nfsd4_layout_ops[ls->ls_layout_type]->fence_client)
 		return true;
 	/*
 	 * Make sure layout has not been returned yet before
@@ -910,6 +971,10 @@ nfsd4_layout_lm_breaker_timedout(struct file_lease *fl)
 	 * fresh schedule that takes an extra unmatched reference.
 	 */
 	spin_lock(&ls->ls_lock);
+	if (ls->ls_fenced || ls->ls_fence_stopped) {
+		spin_unlock(&ls->ls_lock);
+		return true;
+	}
 	if (ls->ls_fence_inflight) {
 		spin_unlock(&ls->ls_lock);
 		return false;
@@ -920,9 +985,8 @@ nfsd4_layout_lm_breaker_timedout(struct file_lease *fl)
 		return true;
 	}
 	ls->ls_fence_inflight = true;
-	spin_unlock(&ls->ls_lock);
-
 	mod_delayed_work(system_dfl_wq, &ls->ls_fence_work, 0);
+	spin_unlock(&ls->ls_lock);
 	return false;
 }
 
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0f9340eb2..4075db57e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2082,6 +2082,7 @@ static void revoke_one_stid(struct nfsd_net *nn, struct nfs4_client *clp,
 			atomic_inc(&clp->cl_admin_revoked);
 		}
 		spin_unlock(&clp->cl_lock);
+		nfsd4_stop_layout_fence(layoutstateid(stid));
 		nfsd4_close_layout(layoutstateid(stid));
 		drop_stid_export(clp, stid);
 		break;
@@ -5861,6 +5862,7 @@ static void nfsd4_drop_revoked_stid(struct nfs4_stid *s)
 		ls = layoutstateid(s);
 		list_del_init(&ls->ls_perclnt);
 		spin_unlock(&cl->cl_lock);
+		nfsd4_stop_layout_fence(ls);
 		nfs4_put_stid(s);
 		break;
 	default:
diff --git a/fs/nfsd/pnfs.h b/fs/nfsd/pnfs.h
index f7bee4dc5..559df0e5b 100644
--- a/fs/nfsd/pnfs.h
+++ b/fs/nfsd/pnfs.h
@@ -78,6 +78,7 @@ void nfsd4_return_all_client_layouts(struct nfs4_client *);
 void nfsd4_return_all_file_layouts(struct nfs4_client *clp,
 		struct nfs4_file *fp);
 void nfsd4_close_layout(struct nfs4_layout_stateid *ls);
+void nfsd4_stop_layout_fence(struct nfs4_layout_stateid *ls);
 int nfsd4_init_pnfs(void);
 void nfsd4_exit_pnfs(void);
 #else
@@ -99,6 +100,10 @@ static inline void nfsd4_return_all_file_layouts(struct nfs4_client *clp,
 static inline void nfsd4_close_layout(struct nfs4_layout_stateid *ls)
 {
 }
+
+static inline void nfsd4_stop_layout_fence(struct nfs4_layout_stateid *ls)
+{
+}
 static inline void nfsd4_exit_pnfs(void)
 {
 }
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index cd9294f02..209ea43a2 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -861,9 +861,11 @@ struct nfs4_layout_stateid {
 	struct mutex			ls_mutex;
 
 	struct delayed_work		ls_fence_work;
+	struct mutex			ls_fence_mutex; /* serializes fence shutdown */
 	unsigned int			ls_fence_delay;
 	bool				ls_fenced;
 	bool				ls_fence_inflight;
+	bool				ls_fence_stopped;
 };
 
 static inline struct nfs4_layout_stateid *layoutstateid(struct nfs4_stid *s)

base-commit: cab95e6be3ba82bcf4c8be27c2eb20e55238aa41
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2] nfsd: drain pNFS fence work during state teardown

[Chuck Lever]

I'm passing along a sashiko review finding here. It was confirmed
with Claude Fable 5.

----

commit ba79c2d330cd7f4c60c83de92ba0d54184b97c08
Author: Jinpyo Lee <bint4b13@gmail.com>

nfsd: drain pNFS fence work during state teardown

Stop the layout fence worker from being scheduled once teardown starts,
and synchronously cancel or drain it before the layout's client
association is released, so the worker cannot use a freed nfs4_client.

> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f487ef0916d9..3d5792b6e997 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c

> @@ -5870,6 +5871,7 @@ static void nfsd4_drop_revoked_stid(struct nfs4_stid *s)
>  		ls = layoutstateid(s);
>  		list_del_init(&ls->ls_perclnt);
>  		spin_unlock(&cl->cl_lock);
> +		nfsd4_stop_layout_fence(ls);
>  		nfs4_put_stid(s);
>  		break;

Which reference keeps ls alive across nfsd4_stop_layout_fence() here?
nfsd4_free_stateid() reaches this case through find_stateid_locked(),
which takes no reference of its own, and the only status it tests first
is SC_STATUS_CLOSED, which the layout case never sets. So two
FREE_STATEID operations for the same admin-revoked layout stateid can
both arrive here, and the nfs4_put_stid() below drops the same hash
reference in each. nfsd40_drop_revoked_stid() has the same shape.

Before this patch that race is a double put. With ls_fence_mutex in
front of the put, the second caller sleeps in mutex_lock() until the
first caller's nfs4_put_stid() frees the stateid, then wakes on the
freed ls_fence_mutex and takes the freed ls_lock in
nfsd4_stop_layout_fence(). Can that be reached from two slots of the
same session?

The SC_TYPE_DELEG case of revoke_one_stid() takes an extra reference to
guard against a concurrent FREE_STATEID, and the delegation recall path
sets SC_STATUS_CLOSED under cl_lock for the same reason. Would setting
SC_STATUS_CLOSED under cl_lock in this case, before the unlock, close
the window? Since the race predates this patch, that guard probably
wants its own patch ahead of this one, with its own Fixes: tag.

-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)