[PATCH v2 0/5] nfsd: don't allow concurrent queueing of workqueue jobs

[PATCH v2 0/5] nfsd: don't allow concurrent queueing of workqueue jobs

[Jeff Layton]

While looking at the problem that Li Lingfeng reported [1] around
callback queueing failures, I noticed that there were potential
scenarios where the callback workqueue jobs could run concurrently with
an rpc_task. Since they touch some of the same fields, this is incorrect
at best and potentially dangerous.

This patchset adds a new mechanism for ensuring that the same
nfsd4_callback can't run concurrently with itself, regardless of where
it is in its execution. This also gives us a more sure mechanism for
handling the places where we need to take and hold a reference on an
object while the callback is running.

This should also fix the problem that Li Lingfeng reported, since
queueing the work from nfsd4_cb_release() should never fail. Note that
their earlier patch (fdf5c9413ea) should be dropped from nfsd-testing
before this will apply cleanly.

[1]: https://lore.kernel.org/linux-nfs/20250218135423.1487309-1-lilingfeng3@huawei.com/

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- added patche to handle rpc_call_async() errors
- rename NFSD4_CALLBACK_RESTART to NFSD4_CALLBACK_REQUEUE
- add patch to replace CB_GETATTR_BUSY with NFSD4_CALLBACK_REQUEUE
- Link to v1: https://lore.kernel.org/r/20250218-nfsd-callback-v1-0-14f966967dd8@kernel.org

---
Jeff Layton (5):
      nfsd: prevent callback tasks running concurrently
      nfsd: eliminate cl_ra_cblist and NFSD4_CLIENT_CB_RECALL_ANY
      nfsd: replace CB_GETATTR_BUSY with NFSD4_CALLBACK_RUNNING
      nfsd: move cb_need_restart flag into cb_flags
      nfsd: handle errors from rpc_call_async()

 fs/nfsd/nfs4callback.c | 26 +++++++++++++++++---------
 fs/nfsd/nfs4layouts.c  |  7 ++++---
 fs/nfsd/nfs4proc.c     |  2 +-
 fs/nfsd/nfs4state.c    | 31 ++++++++++++++-----------------
 fs/nfsd/state.h        | 18 +++++++++++-------
 fs/nfsd/trace.h        |  2 +-
 6 files changed, 48 insertions(+), 38 deletions(-)
---
base-commit: b7e85fd7c8964e31f8fa1cf7333b12f442b642f1
change-id: 20250218-nfsd-callback-f723b8498c78

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v2 1/5] nfsd: prevent callback tasks running concurrently

[Jeff Layton]

The nfsd4_callback workqueue jobs exist to queue backchannel RPCs to
rpciod. Because they run in different workqueue contexts, the rpc_task
can run concurrently with the workqueue job itself, should it become
requeued. This is problematic as there is no locking when accessing the
fields in the nfsd4_callback.

Add a new unsigned long to nfsd4_callback and declare a new
NFSD4_CALLBACK_RUNNING flag to be set in it. When attempting to run a
workqueue job, do a test_and_set_bit() on that flag first, and don't
queue the workqueue job if it returns true. Clear NFSD4_CALLBACK_RUNNING
in nfsd41_destroy_cb().

This also gives us a more reliable mechanism for handling queueing
failures in codepaths where we have to take references under spinlocks.
We can now do the test_and_set_bit on NFSD4_CALLBACK_RUNNING first, and
only take references to the objects if that returns false.

Most of the nfsd4_run_cb() callers are converted to use this new flag or
the nfsd4_try_run_cb() wrapper. The main exception is the callback
channel probe, which has its own synchronization.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4callback.c |  2 ++
 fs/nfsd/nfs4layouts.c  |  7 ++++---
 fs/nfsd/nfs4proc.c     |  2 +-
 fs/nfsd/nfs4state.c    | 14 ++++++++++----
 fs/nfsd/state.h        |  9 +++++++++
 5 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index ae4b7b6df47ff054db197bd2f6083905e4149bee..1f26c811e5f73c2e745ee68d0b6e668d1dd7c704 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1312,6 +1312,7 @@ static void nfsd41_destroy_cb(struct nfsd4_callback *cb)
 
 	trace_nfsd_cb_destroy(clp, cb);
 	nfsd41_cb_release_slot(cb);
+	clear_bit(NFSD4_CALLBACK_RUNNING, &cb->cb_flags);
 	if (cb->cb_ops && cb->cb_ops->release)
 		cb->cb_ops->release(cb);
 	nfsd41_cb_inflight_end(clp);
@@ -1632,6 +1633,7 @@ void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
 	cb->cb_msg.rpc_proc = &nfs4_cb_procedures[op];
 	cb->cb_msg.rpc_argp = cb;
 	cb->cb_msg.rpc_resp = cb;
+	cb->cb_flags = 0;
 	cb->cb_ops = ops;
 	INIT_WORK(&cb->cb_work, nfsd4_run_cb_work);
 	cb->cb_status = 0;
diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index fbfddd3c4c943c34aa3991328eacb5759e311cc4..290271ac424540e4405a5fd0eacc8db9f47603cd 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -344,9 +344,10 @@ nfsd4_recall_file_layout(struct nfs4_layout_stateid *ls)
 	atomic_inc(&ls->ls_stid.sc_file->fi_lo_recalls);
 	trace_nfsd_layout_recall(&ls->ls_stid.sc_stateid);
 
-	refcount_inc(&ls->ls_stid.sc_count);
-	nfsd4_run_cb(&ls->ls_recall);
-
+	if (!test_and_set_bit(NFSD4_CALLBACK_RUNNING, &ls->ls_recall.cb_flags)) {
+		refcount_inc(&ls->ls_stid.sc_count);
+		nfsd4_run_cb(&ls->ls_recall);
+	}
 out_unlock:
 	spin_unlock(&ls->ls_lock);
 }
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index f6e06c779d09dacdcea81fb3b4135bf600f6cc63..b397246dae7b7e8c2a0ba436bb3813213cfe4fa2 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1847,7 +1847,7 @@ static void nfsd4_send_cb_offload(struct nfsd4_copy *copy)
 		      NFSPROC4_CLNT_CB_OFFLOAD);
 	trace_nfsd_cb_offload(copy->cp_clp, &cbo->co_res.cb_stateid,
 			      &cbo->co_fh, copy->cp_count, copy->nfserr);
-	nfsd4_run_cb(&cbo->co_cb);
+	nfsd4_try_run_cb(&cbo->co_cb);
 }
 
 /**
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e806fd97cca972559d1929d37c1a24e760d9d6d8..fabcd979c40695ebcc795cfd2d8a035b7d589a37 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3232,8 +3232,10 @@ static void nfs4_cb_getattr(struct nfs4_cb_fattr *ncf)
 	/* set to proper status when nfsd4_cb_getattr_done runs */
 	ncf->ncf_cb_status = NFS4ERR_IO;
 
-	refcount_inc(&dp->dl_stid.sc_count);
-	nfsd4_run_cb(&ncf->ncf_getattr);
+	if (!test_and_set_bit(NFSD4_CALLBACK_RUNNING, &ncf->ncf_getattr.cb_flags)) {
+		refcount_inc(&dp->dl_stid.sc_count);
+		nfsd4_run_cb(&ncf->ncf_getattr);
+	}
 }
 
 static struct nfs4_client *create_client(struct xdr_netobj name,
@@ -5422,6 +5424,10 @@ static const struct nfsd4_callback_ops nfsd4_cb_recall_ops = {
 static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
 {
 	bool queued;
+
+	if (test_and_set_bit(NFSD4_CALLBACK_RUNNING, &dp->dl_recall.cb_flags))
+		return;
+
 	/*
 	 * We're assuming the state code never drops its reference
 	 * without first removing the lease.  Since we're in this lease
@@ -6910,7 +6916,7 @@ deleg_reaper(struct nfsd_net *nn)
 		clp->cl_ra->ra_bmval[0] = BIT(RCA4_TYPE_MASK_RDATA_DLG) |
 						BIT(RCA4_TYPE_MASK_WDATA_DLG);
 		trace_nfsd_cb_recall_any(clp->cl_ra);
-		nfsd4_run_cb(&clp->cl_ra->ra_cb);
+		nfsd4_try_run_cb(&clp->cl_ra->ra_cb);
 	}
 }
 
@@ -7839,7 +7845,7 @@ nfsd4_lm_notify(struct file_lock *fl)
 
 	if (queue) {
 		trace_nfsd_cb_notify_lock(lo, nbl);
-		nfsd4_run_cb(&nbl->nbl_cb);
+		nfsd4_try_run_cb(&nbl->nbl_cb);
 	}
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 74d2d7b42676d907bec9159b927aeed223d668c3..e6af969a03f26b1feb6768af5baa4887bf5fa5e9 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -67,6 +67,8 @@ typedef struct {
 struct nfsd4_callback {
 	struct nfs4_client *cb_clp;
 	struct rpc_message cb_msg;
+#define NFSD4_CALLBACK_RUNNING	BIT(0)	// Callback is running
+	unsigned long cb_flags;
 	const struct nfsd4_callback_ops *cb_ops;
 	struct work_struct cb_work;
 	int cb_seq_status;
@@ -780,6 +782,13 @@ extern void nfsd4_change_callback(struct nfs4_client *clp, struct nfs4_cb_conn *
 extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
 		const struct nfsd4_callback_ops *ops, enum nfsd4_cb_op op);
 extern bool nfsd4_run_cb(struct nfsd4_callback *cb);
+
+static inline void nfsd4_try_run_cb(struct nfsd4_callback *cb)
+{
+	if (!test_and_set_bit(NFSD4_CALLBACK_RUNNING, &cb->cb_flags))
+		WARN_ON_ONCE(!nfsd4_run_cb(cb));
+}
+
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
 extern void nfsd4_shutdown_copy(struct nfs4_client *clp);
 void nfsd4_async_copy_reaper(struct nfsd_net *nn);

-- 
2.48.1

[PATCH v2 2/5] nfsd: eliminate cl_ra_cblist and NFSD4_CLIENT_CB_RECALL_ANY

[Jeff Layton]

deleg_reaper() will walk the client_lru list and put any suitable
entries onto "cblist" using the cl_ra_cblist pointer. It then walks the
objects outside the spinlock and queues callbacks for them.

None of the operations that deleg_reaper() does outside the
nn->client_lock are blocking operations. Just queue their workqueue jobs
under the nn->client_lock instead.

Also, the NFSD4_CLIENT_CB_RECALL_ANY and NFSD4_CALLBACK_RUNNING flags
serve an identical purpose now. Drop the NFSD4_CLIENT_CB_RECALL_ANY flag
and just use the one in the callback.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 16 +++-------------
 fs/nfsd/state.h     |  2 --
 2 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fabcd979c40695ebcc795cfd2d8a035b7d589a37..422439a46ffd03926524b8463cfdabfb866281b3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3175,7 +3175,6 @@ nfsd4_cb_recall_any_release(struct nfsd4_callback *cb)
 {
 	struct nfs4_client *clp = cb->cb_clp;
 
-	clear_bit(NFSD4_CLIENT_CB_RECALL_ANY, &clp->cl_flags);
 	drop_client(clp);
 }
 
@@ -6881,7 +6880,6 @@ deleg_reaper(struct nfsd_net *nn)
 {
 	struct list_head *pos, *next;
 	struct nfs4_client *clp;
-	LIST_HEAD(cblist);
 
 	spin_lock(&nn->client_lock);
 	list_for_each_safe(pos, next, &nn->client_lru) {
@@ -6893,31 +6891,23 @@ deleg_reaper(struct nfsd_net *nn)
 			continue;
 		if (atomic_read(&clp->cl_delegs_in_recall))
 			continue;
-		if (test_bit(NFSD4_CLIENT_CB_RECALL_ANY, &clp->cl_flags))
+		if (test_and_set_bit(NFSD4_CALLBACK_RUNNING, &clp->cl_ra->ra_cb.cb_flags))
 			continue;
 		if (ktime_get_boottime_seconds() - clp->cl_ra_time < 5)
 			continue;
 		if (clp->cl_cb_state != NFSD4_CB_UP)
 			continue;
-		list_add(&clp->cl_ra_cblist, &cblist);
 
 		/* release in nfsd4_cb_recall_any_release */
 		kref_get(&clp->cl_nfsdfs.cl_ref);
-		set_bit(NFSD4_CLIENT_CB_RECALL_ANY, &clp->cl_flags);
 		clp->cl_ra_time = ktime_get_boottime_seconds();
-	}
-	spin_unlock(&nn->client_lock);
-
-	while (!list_empty(&cblist)) {
-		clp = list_first_entry(&cblist, struct nfs4_client,
-					cl_ra_cblist);
-		list_del_init(&clp->cl_ra_cblist);
 		clp->cl_ra->ra_keep = 0;
 		clp->cl_ra->ra_bmval[0] = BIT(RCA4_TYPE_MASK_RDATA_DLG) |
 						BIT(RCA4_TYPE_MASK_WDATA_DLG);
 		trace_nfsd_cb_recall_any(clp->cl_ra);
-		nfsd4_try_run_cb(&clp->cl_ra->ra_cb);
+		nfsd4_run_cb(&clp->cl_ra->ra_cb);
 	}
+	spin_unlock(&nn->client_lock);
 }
 
 static void
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e6af969a03f26b1feb6768af5baa4887bf5fa5e9..6967925f4b438b24c2abd5fcd4f3bd6664a81862 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -454,7 +454,6 @@ struct nfs4_client {
 #define NFSD4_CLIENT_UPCALL_LOCK	(5)	/* upcall serialization */
 #define NFSD4_CLIENT_CB_FLAG_MASK	(1 << NFSD4_CLIENT_CB_UPDATE | \
 					 1 << NFSD4_CLIENT_CB_KILL)
-#define NFSD4_CLIENT_CB_RECALL_ANY	(6)
 	unsigned long		cl_flags;
 
 	struct workqueue_struct *cl_callback_wq;
@@ -500,7 +499,6 @@ struct nfs4_client {
 
 	struct nfsd4_cb_recall_any	*cl_ra;
 	time64_t		cl_ra_time;
-	struct list_head	cl_ra_cblist;
 };
 
 /* struct nfs4_client_reset

-- 
2.48.1

[PATCH v2 3/5] nfsd: replace CB_GETATTR_BUSY with NFSD4_CALLBACK_RUNNING

[Jeff Layton]

These flags serve essentially the same purpose and get set and cleared
at the same time. Drop CB_GETATTR_BUSY and just use
NFSD4_CALLBACK_RUNNING instead.

For this to work, we must use clear_and_wake_up_bit(), but doing that on
for other types of callbacks is wasteful. Declare a new NFSD4_CALLBACK_WAKE
flag in cb_flags to indicate that wake_up is needed, and only set that
for CB_GETATTRs.

Also, make the wait use a TASK_UNINTERRUPTIBLE sleep. This is done in
the context of an nfsd thread, and it should never need to deal with
signals.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4callback.c |  6 +++++-
 fs/nfsd/nfs4state.c    | 17 +++++++++--------
 fs/nfsd/state.h        |  5 +----
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 1f26c811e5f73c2e745ee68d0b6e668d1dd7c704..bb2cb0d1b7885bf5f6be39221cb3dd95518326e5 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1312,7 +1312,11 @@ static void nfsd41_destroy_cb(struct nfsd4_callback *cb)
 
 	trace_nfsd_cb_destroy(clp, cb);
 	nfsd41_cb_release_slot(cb);
-	clear_bit(NFSD4_CALLBACK_RUNNING, &cb->cb_flags);
+	if (test_bit(NFSD4_CALLBACK_WAKE, &cb->cb_flags))
+		clear_and_wake_up_bit(NFSD4_CALLBACK_RUNNING, &cb->cb_flags);
+	else
+		clear_bit(NFSD4_CALLBACK_RUNNING, &cb->cb_flags);
+
 	if (cb->cb_ops && cb->cb_ops->release)
 		cb->cb_ops->release(cb);
 	nfsd41_cb_inflight_end(clp);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 422439a46ffd03926524b8463cfdabfb866281b3..b68bc3f12a8c29904e6bdd2b5a1e4ed6418104e3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3205,7 +3205,6 @@ nfsd4_cb_getattr_release(struct nfsd4_callback *cb)
 	struct nfs4_delegation *dp =
 			container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
 
-	clear_and_wake_up_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags);
 	nfs4_put_stid(&dp->dl_stid);
 }
 
@@ -3226,15 +3225,17 @@ static void nfs4_cb_getattr(struct nfs4_cb_fattr *ncf)
 	struct nfs4_delegation *dp =
 			container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
 
-	if (test_and_set_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags))
+	if (test_and_set_bit(NFSD4_CALLBACK_RUNNING, &ncf->ncf_getattr.cb_flags))
 		return;
+
 	/* set to proper status when nfsd4_cb_getattr_done runs */
 	ncf->ncf_cb_status = NFS4ERR_IO;
 
-	if (!test_and_set_bit(NFSD4_CALLBACK_RUNNING, &ncf->ncf_getattr.cb_flags)) {
-		refcount_inc(&dp->dl_stid.sc_count);
-		nfsd4_run_cb(&ncf->ncf_getattr);
-	}
+	/* ensure that wake_bit is done when RUNNING is cleared */
+	set_bit(NFSD4_CALLBACK_WAKE, &ncf->ncf_getattr.cb_flags);
+
+	refcount_inc(&dp->dl_stid.sc_count);
+	nfsd4_run_cb(&ncf->ncf_getattr);
 }
 
 static struct nfs4_client *create_client(struct xdr_netobj name,
@@ -9210,8 +9211,8 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 	nfs4_cb_getattr(&dp->dl_cb_fattr);
 	spin_unlock(&ctx->flc_lock);
 
-	wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
-			    TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
+	wait_on_bit_timeout(&ncf->ncf_getattr.cb_flags, NFSD4_CALLBACK_RUNNING,
+			    TASK_UNINTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
 	if (ncf->ncf_cb_status) {
 		/* Recall delegation only if client didn't respond */
 		status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 6967925f4b438b24c2abd5fcd4f3bd6664a81862..b7d6be45ad022194e90cdf8d19c2879be08143ff 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -68,6 +68,7 @@ struct nfsd4_callback {
 	struct nfs4_client *cb_clp;
 	struct rpc_message cb_msg;
 #define NFSD4_CALLBACK_RUNNING	BIT(0)	// Callback is running
+#define NFSD4_CALLBACK_WAKE	BIT(1)	// must wake_bit when clearing RUNNING
 	unsigned long cb_flags;
 	const struct nfsd4_callback_ops *cb_ops;
 	struct work_struct cb_work;
@@ -164,15 +165,11 @@ struct nfs4_cb_fattr {
 	struct timespec64 ncf_cb_mtime;
 	struct timespec64 ncf_cb_atime;
 
-	unsigned long ncf_cb_flags;
 	bool ncf_file_modified;
 	u64 ncf_initial_cinfo;
 	u64 ncf_cur_fsize;
 };
 
-/* bits for ncf_cb_flags */
-#define	CB_GETATTR_BUSY		0
-
 /*
  * Represents a delegation stateid. The nfs4_client holds references to these
  * and they are put when it is being destroyed or when the delegation is

-- 
2.48.1

[PATCH v2 4/5] nfsd: move cb_need_restart flag into cb_flags

[Jeff Layton]

Since there is now a cb_flags word, use a new NFSD4_CALLBACK_REQUEUE
flag in that instead of a separate boolean.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4callback.c | 10 ++++------
 fs/nfsd/state.h        |  2 +-
 fs/nfsd/trace.h        |  2 +-
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index bb2cb0d1b7885bf5f6be39221cb3dd95518326e5..018533bb83a3ca6fda46e072eebca6d2583c393a 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1071,7 +1071,7 @@ static void nfsd4_requeue_cb(struct rpc_task *task, struct nfsd4_callback *cb)
 	if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) {
 		trace_nfsd_cb_restart(clp, cb);
 		task->tk_status = 0;
-		cb->cb_need_restart = true;
+		set_bit(NFSD4_CALLBACK_REQUEUE, &cb->cb_flags);
 	}
 }
 
@@ -1479,7 +1479,7 @@ static void nfsd4_cb_release(void *calldata)
 
 	trace_nfsd_cb_rpc_release(cb->cb_clp);
 
-	if (cb->cb_need_restart)
+	if (test_bit(NFSD4_CALLBACK_REQUEUE, &cb->cb_flags))
 		nfsd4_queue_cb(cb);
 	else
 		nfsd41_destroy_cb(cb);
@@ -1618,12 +1618,11 @@ nfsd4_run_cb_work(struct work_struct *work)
 		return;
 	}
 
-	if (cb->cb_need_restart) {
-		cb->cb_need_restart = false;
-	} else {
+	if (!test_and_clear_bit(NFSD4_CALLBACK_REQUEUE, &cb->cb_flags)) {
 		if (cb->cb_ops && cb->cb_ops->prepare)
 			cb->cb_ops->prepare(cb);
 	}
+
 	cb->cb_msg.rpc_cred = clp->cl_cb_cred;
 	flags = clp->cl_minorversion ? RPC_TASK_NOCONNECT : RPC_TASK_SOFTCONN;
 	rpc_call_async(clnt, &cb->cb_msg, RPC_TASK_SOFT | flags,
@@ -1641,7 +1640,6 @@ void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
 	cb->cb_ops = ops;
 	INIT_WORK(&cb->cb_work, nfsd4_run_cb_work);
 	cb->cb_status = 0;
-	cb->cb_need_restart = false;
 	cb->cb_held_slot = -1;
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index b7d6be45ad022194e90cdf8d19c2879be08143ff..bdcbee1d328a2f68da2e7c733f50f8eee5775be7 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -69,13 +69,13 @@ struct nfsd4_callback {
 	struct rpc_message cb_msg;
 #define NFSD4_CALLBACK_RUNNING	BIT(0)	// Callback is running
 #define NFSD4_CALLBACK_WAKE	BIT(1)	// must wake_bit when clearing RUNNING
+#define NFSD4_CALLBACK_REQUEUE	BIT(2)	// requeue callback instead of destroying
 	unsigned long cb_flags;
 	const struct nfsd4_callback_ops *cb_ops;
 	struct work_struct cb_work;
 	int cb_seq_status;
 	int cb_status;
 	int cb_held_slot;
-	bool cb_need_restart;
 };
 
 struct nfsd4_callback_ops {
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 49bbd26ffcdb36173047569b8d4b41efdec4880b..f26a42a37f452486e9683d7778016adf923f2995 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -1613,7 +1613,7 @@ DECLARE_EVENT_CLASS(nfsd_cb_lifetime_class,
 		__entry->cl_id = clp->cl_clientid.cl_id;
 		__entry->cb = cb;
 		__entry->opcode = cb->cb_ops ? cb->cb_ops->opcode : _CB_NULL;
-		__entry->need_restart = cb->cb_need_restart;
+		__entry->need_restart = test_bit(NFSD4_CALLBACK_REQUEUE, &cb->cb_flags);
 		__assign_sockaddr(addr, &clp->cl_cb_conn.cb_addr,
 				  clp->cl_cb_conn.cb_addrlen)
 	),

-- 
2.48.1

[PATCH v2 5/5] nfsd: handle errors from rpc_call_async()

[Jeff Layton]

It's possible for rpc_call_async() to fail (mainly due to memory
allocation failure). If it does, there isn't much recourse other than to
requeue the callback and try again later.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4callback.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 018533bb83a3ca6fda46e072eebca6d2583c393a..3f21de06154e3f80818c85a92d6044058339697f 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1592,7 +1592,7 @@ nfsd4_run_cb_work(struct work_struct *work)
 		container_of(work, struct nfsd4_callback, cb_work);
 	struct nfs4_client *clp = cb->cb_clp;
 	struct rpc_clnt *clnt;
-	int flags;
+	int flags, ret;
 
 	trace_nfsd_cb_start(clp);
 
@@ -1625,8 +1625,12 @@ nfsd4_run_cb_work(struct work_struct *work)
 
 	cb->cb_msg.rpc_cred = clp->cl_cb_cred;
 	flags = clp->cl_minorversion ? RPC_TASK_NOCONNECT : RPC_TASK_SOFTCONN;
-	rpc_call_async(clnt, &cb->cb_msg, RPC_TASK_SOFT | flags,
-			cb->cb_ops ? &nfsd4_cb_ops : &nfsd4_cb_probe_ops, cb);
+	ret = rpc_call_async(clnt, &cb->cb_msg, RPC_TASK_SOFT | flags,
+			     cb->cb_ops ? &nfsd4_cb_ops : &nfsd4_cb_probe_ops, cb);
+	if (ret != 0) {
+		set_bit(NFSD4_CALLBACK_REQUEUE, &cb->cb_flags);
+		nfsd4_queue_cb(cb);
+	}
 }
 
 void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,

-- 
2.48.1

Re: [PATCH v2 0/5] nfsd: don't allow concurrent queueing of workqueue jobs

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

On Thu, 20 Feb 2025 11:47:12 -0500, Jeff Layton wrote:
> While looking at the problem that Li Lingfeng reported [1] around
> callback queueing failures, I noticed that there were potential
> scenarios where the callback workqueue jobs could run concurrently with
> an rpc_task. Since they touch some of the same fields, this is incorrect
> at best and potentially dangerous.
> 
> This patchset adds a new mechanism for ensuring that the same
> nfsd4_callback can't run concurrently with itself, regardless of where
> it is in its execution. This also gives us a more sure mechanism for
> handling the places where we need to take and hold a reference on an
> object while the callback is running.
> 
> [...]

Applied to nfsd-testing, thanks! This series replaces:

https://lore.kernel.org/linux-nfs/20250218135423.1487309-1-lilingfeng3@huawei.com/

Review is still open.

[1/5] nfsd: prevent callback tasks running concurrently
      commit: 9a03a9d82410bdb758a6b342689e0c235bba94f1
[2/5] nfsd: eliminate cl_ra_cblist and NFSD4_CLIENT_CB_RECALL_ANY
      commit: 743fda103062626c828dbac774716e718a74f81b
[3/5] nfsd: replace CB_GETATTR_BUSY with NFSD4_CALLBACK_RUNNING
      commit: d2d94554567f486eba111e953e75745eca09bee3
[4/5] nfsd: move cb_need_restart flag into cb_flags
      commit: 355f1ec5ce21ab324d9b3978d2d5abe6d0c84024
[5/5] nfsd: handle errors from rpc_call_async()
      commit: d0f1ba5ed270fbda06248ef8af822a9e14708ee1

--
Chuck Lever

Re: [PATCH v2 1/5] nfsd: prevent callback tasks running concurrently

[ChenXiaoSong]

在 2025/2/21 00:47, Jeff Layton 写道:
> Most of the nfsd4_run_cb() callers are converted to use this new flag or
> the nfsd4_try_run_cb() wrapper. The main exception is the callback
> channel probe, which has its own synchronization.
> 

Hi Jeff:

We had a null-ptr-deref in nfsd4_probe_callback():

[24225.738349] Unable to handle kernel NULL pointer dereference at 
virtual address 0000000000000000
...
[24225.803480] Call trace:
[24225.804639]  __queue_work+0xb4/0x558
[24225.805949]  queue_work_on+0x88/0x90
[24225.807306]  nfsd4_probe_callback+0x4c/0x58 [nfsd]
[24225.808896]  nfsd4_probe_callback_sync+0x20/0x38 [nfsd]
[24225.808909]  nfsd4_init_conn.isra.57+0x8c/0xa8 [nfsd]
[24225.815204]  nfsd4_create_session+0x5b8/0x718 [nfsd]
[24225.817711]  nfsd4_proc_compound+0x4c0/0x710 [nfsd]
[24225.819329]  nfsd_dispatch+0x104/0x248 [nfsd]
[24225.820742]  svc_process_common+0x348/0x808 [sunrpc]
[24225.822294]  svc_process+0xb0/0xc8 [sunrpc]
[24225.823760]  nfsd+0xf0/0x160 [nfsd]
[24225.825006]  kthread+0x134/0x138
[24225.826336]  ret_from_fork+0x10/0x18

Is this patch or patchset can fix this issue? And I'm having trouble 
understanding the commit message "callback channel probe has its own 
synchronization", I'd appreciate it if you could explain in more detail.

Thanks,
ChenXiaoSong.

Re: [PATCH v2 1/5] nfsd: prevent callback tasks running concurrently

[Jeff Layton]

On Tue, 2025-06-10 at 16:49 +0800, ChenXiaoSong wrote:
> 在 2025/2/21 00:47, Jeff Layton 写道:
> > Most of the nfsd4_run_cb() callers are converted to use this new flag or
> > the nfsd4_try_run_cb() wrapper. The main exception is the callback
> > channel probe, which has its own synchronization.
> > 
> 
> Hi Jeff:
> 
> We had a null-ptr-deref in nfsd4_probe_callback():
> 
> [24225.738349] Unable to handle kernel NULL pointer dereference at 
> virtual address 0000000000000000
> ...
> [24225.803480] Call trace:
> [24225.804639]  __queue_work+0xb4/0x558
> [24225.805949]  queue_work_on+0x88/0x90
> [24225.807306]  nfsd4_probe_callback+0x4c/0x58 [nfsd]
> [24225.808896]  nfsd4_probe_callback_sync+0x20/0x38 [nfsd]
> [24225.808909]  nfsd4_init_conn.isra.57+0x8c/0xa8 [nfsd]
> [24225.815204]  nfsd4_create_session+0x5b8/0x718 [nfsd]
> [24225.817711]  nfsd4_proc_compound+0x4c0/0x710 [nfsd]
> [24225.819329]  nfsd_dispatch+0x104/0x248 [nfsd]
> [24225.820742]  svc_process_common+0x348/0x808 [sunrpc]
> [24225.822294]  svc_process+0xb0/0xc8 [sunrpc]
> [24225.823760]  nfsd+0xf0/0x160 [nfsd]
> [24225.825006]  kthread+0x134/0x138
> [24225.826336]  ret_from_fork+0x10/0x18
> 
> Is this patch or patchset can fix this issue? And I'm having trouble 
> understanding the commit message "callback channel probe has its own 
> synchronization", I'd appreciate it if you could explain in more detail.
> 

Synchronization was probably too strong a word. I remember looking over
this code and convincing myself that the probe callback wasn't subject
to the same races as the others, but I think that was mostly because
the outcome of those races was not harmful. Note that the probe itself
can actually be run at the start of a completely unrelated callback to
the same client.

So you hit a NULL pointer in __queue_work()? The work_struct is
embedded in the nfs4_client so that would probably imply that that the
nfs4_client struct was corrupt?

You may want to get a vmcore and analyze it if you can reproduce this.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 1/5] nfsd: prevent callback tasks running concurrently

[ChenXiaoSong]

在 2025/6/10 19:09, Jeff Layton 写道:
> 
> Synchronization was probably too strong a word. I remember looking over
> this code and convincing myself that the probe callback wasn't subject
> to the same races as the others, but I think that was mostly because
> the outcome of those races was not harmful. Note that the probe itself
> can actually be run at the start of a completely unrelated callback to
> the same client.
> 
> So you hit a NULL pointer in __queue_work()? The work_struct is
> embedded in the nfs4_client so that would probably imply that that the
> nfs4_client struct was corrupt?
> 
> You may want to get a vmcore and analyze it if you can reproduce this.

Thanks for your reply.

I have already got a vmcore. Here is the link to the vmcore analysis:

https://chenxiaosong.com/en/nfs/en-null-ptr-deref-in-nfsd4_probe_callback.html

Please let me know if you need any more detailed information.

Thanks,
ChenXiaoSong.

Re: [PATCH v2 1/5] nfsd: prevent callback tasks running concurrently

[ChenXiaoSong]

Hi Jeff:

Do you have any suggestions for this null-ptr-deref issue?

Here is the link to the vmcore analysis:

https://chenxiaosong.com/en/nfs/en-null-ptr-deref-in-nfsd4_probe_callback.html

Thanks,
ChenXiaoSong.

在 2025/6/11 15:12, ChenXiaoSong 写道:
> 在 2025/6/10 19:09, Jeff Layton 写道:
>>
>> Synchronization was probably too strong a word. I remember looking over
>> this code and convincing myself that the probe callback wasn't subject
>> to the same races as the others, but I think that was mostly because
>> the outcome of those races was not harmful. Note that the probe itself
>> can actually be run at the start of a completely unrelated callback to
>> the same client.
>>
>> So you hit a NULL pointer in __queue_work()? The work_struct is
>> embedded in the nfs4_client so that would probably imply that that the
>> nfs4_client struct was corrupt?
>>
>> You may want to get a vmcore and analyze it if you can reproduce this.
> 
> Thanks for your reply.
> 
> I have already got a vmcore. Here is the link to the vmcore analysis:
> 
> https://chenxiaosong.com/en/nfs/en-null-ptr-deref-in- 
> nfsd4_probe_callback.html
> 
> Please let me know if you need any more detailed information.
> 
> Thanks,
> ChenXiaoSong.

Re: [PATCH v2 1/5] nfsd: prevent callback tasks running concurrently

[Jeff Layton]

On Mon, 2025-06-16 at 15:49 +0800, ChenXiaoSong wrote:
> Hi Jeff:
> 
> Do you have any suggestions for this null-ptr-deref issue?
> 
> Here is the link to the vmcore analysis:
> 
> https://chenxiaosong.com/en/nfs/en-null-ptr-deref-in-nfsd4_probe_callback.html
> 
> Thanks,
> ChenXiaoSong.
> 

Not right offhand. My first guess would be some sort of UAF. Maybe a
nfs4_client refcounting issue? If this is reproducible then you may
want to turn up KASAN which might give you more info.

That said, 4.19.90 is more than 6 years old at this point. You may want
to consider moving to something more recent.


> 在 2025/6/11 15:12, ChenXiaoSong 写道:
> > 在 2025/6/10 19:09, Jeff Layton 写道:
> > > 
> > > Synchronization was probably too strong a word. I remember looking over
> > > this code and convincing myself that the probe callback wasn't subject
> > > to the same races as the others, but I think that was mostly because
> > > the outcome of those races was not harmful. Note that the probe itself
> > > can actually be run at the start of a completely unrelated callback to
> > > the same client.
> > > 
> > > So you hit a NULL pointer in __queue_work()? The work_struct is
> > > embedded in the nfs4_client so that would probably imply that that the
> > > nfs4_client struct was corrupt?
> > > 
> > > You may want to get a vmcore and analyze it if you can reproduce this.
> > 
> > Thanks for your reply.
> > 
> > I have already got a vmcore. Here is the link to the vmcore analysis:
> > 
> > https://chenxiaosong.com/en/nfs/en-null-ptr-deref-in- 
> > nfsd4_probe_callback.html
> > 
> > Please let me know if you need any more detailed information.
> > 
> > Thanks,
> > ChenXiaoSong.
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 1/5] nfsd: prevent callback tasks running concurrently

[ChenXiaoSong]

OK, I'll try to reproduce it. Thanks for your reply.

在 2025/6/16 20:09, Jeff Layton 写道:
> Not right offhand. My first guess would be some sort of UAF. Maybe a
> nfs4_client refcounting issue? If this is reproducible then you may
> want to turn up KASAN which might give you more info.
> 
> That said, 4.19.90 is more than 6 years old at this point. You may want
> to consider moving to something more recent.