[PATCH 02/10] nfsd: drain callbacks and clear cl_cb_session

[Jeff Layton]

From: Chris Mason <clm@xxxxxxxx>

After a DESTROY_SESSION the per-session teardown path can free a
session while rpciod still holds an inflight callback rpc_task that
dereferences clp->cl_cb_session.  nfsd4_probe_callback_sync() flushes
cl_callback_wq, but once nfsd4_run_cb_work() has called
rpc_call_async() the rpc_task lives on rpciod; flushing the workqueue
does not wait for it.  After the flush returns,
nfsd4_destroy_session() proceeds through nfsd4_put_session_locked()
and free_session() kfree()s the slab while rpciod's
nfsd4_cb_sequence_done(), grab_slot(), and nfsd41_cb_release_slot()
are still dereferencing cb->cb_clp->cl_cb_session.

    destroy path                       rpciod
    ------------                       ------
    unhash_session(ses)
    nfsd4_probe_callback_sync(clp)
      flush_workqueue(cl_callback_wq)
      /* returns; rpc_task still live */
    nfsd4_put_session_locked(ses)
    free_session(ses) -> kfree(ses)
                                       nfsd4_cb_sequence_done()
                                         reads cb_clp->cl_cb_session
                                         /* freed slab */

A second window exists in nfsd4_process_cb_update().  When
__nfsd4_find_backchannel() returns NULL because unhash_session() has
already removed the destroyed session from cl_sessions,
setup_callback_client() takes the v4.1 early return

    if (!conn->cb_xprt || !ses)
            return -EINVAL;

so clp->cl_cb_session = ses never fires and the field retains a
pointer to the about-to-be-freed session.  Symmetrically, if a later
probe finds a different session's backchannel conn and that
setup_callback_client() call fails, the error tail must still scrub
any previously published cl_cb_session.

Fix by mirroring the two-stage drain that nfsd4_shutdown_callback()
already performs: call nfsd41_cb_inflight_wait_complete() in
nfsd4_probe_callback_sync() after flush_workqueue() so rpciod-side
nfsd41_cb_inflight_end() decrements are observed before the caller
releases the final session reference.  The two direct callers,
nfsd4_destroy_session() and nfsd4_init_conn() (itself invoked from
nfsd4_create_session() and nfsd4_bind_conn_to_session()), run in
sleepable process context and tolerate the wait_var_event() sleep:

    nfsd4_destroy_session() (fs/nfsd/nfs4state.c):
      unhash_session(ses);
      spin_unlock(&nn->client_lock);   /* spinlock dropped */
      nfsd4_probe_callback_sync(ses->se_client);

    nfsd4_init_conn() (fs/nfsd/nfs4state.c):
      acquires no locks in its body; calls nfsd4_hash_conn(),
      nfsd4_register_conn(), then nfsd4_probe_callback_sync() --
      entirely in sleepable process context.

Also clear clp->cl_cb_session unconditionally on the
nfsd4_process_cb_update() error return so every
setup_callback_client() failure -- whether c is NULL or points at a
different session whose probe failed -- leaves the field NULL rather
than pointing at a session that may subsequently be freed.

Fixes: dcbeaa68dbbd ("nfsd4: allow backchannel recovery")
Assisted-by: kres:claude-opus-4-7
Signed-off-by: Chris Mason <clm@xxxxxxxx>
---
 fs/nfsd/nfs4callback.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 1964a213f80e..1cf6b6100357 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1205,9 +1205,8 @@ static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *c
 	} else {
 		if (!conn->cb_xprt || !ses)
 			return -EINVAL;
-		clp->cl_cb_session = ses;
 		args.bc_xprt = conn->cb_xprt;
-		args.prognumber = clp->cl_cb_session->se_cb_prog;
+		args.prognumber = ses->se_cb_prog;
 		args.protocol = conn->cb_xprt->xpt_class->xcl_ident |
 				XPRT_TRANSPORT_BC;
 		args.authflavor = ses->se_cb_sec.flavor;
@@ -1225,8 +1224,10 @@ static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *c
 		return -ENOMEM;
 	}
 
-	if (clp->cl_minorversion != 0)
+	if (clp->cl_minorversion != 0) {
 		clp->cl_cb_conn.cb_xprt = conn->cb_xprt;
+		clp->cl_cb_session = ses;
+	}
 	clp->cl_cb_client = client;
 	clp->cl_cb_cred = cred;
 	rcu_read_lock();
@@ -1299,6 +1300,7 @@ void nfsd4_probe_callback_sync(struct nfs4_client *clp)
 {
 	nfsd4_probe_callback(clp);
 	flush_workqueue(clp->cl_callback_wq);
+	nfsd41_cb_inflight_wait_complete(clp);
 }
 
 void nfsd4_change_callback(struct nfs4_client *clp, struct nfs4_cb_conn *conn)
@@ -1679,7 +1681,17 @@ static struct nfsd4_conn * __nfsd4_find_backchannel(struct nfs4_client *clp)
  * Note there isn't a lot of locking in this code; instead we depend on
  * the fact that it is run from clp->cl_callback_wq, which won't run two
  * work items at once.  So, for example, clp->cl_callback_wq handles all
- * access of cl_cb_client and all calls to rpc_create or rpc_shutdown_client.
+ * access of cl_cb_client and cl_cb_session, and all calls to rpc_create
+ * or rpc_shutdown_client.
+ *
+ * rpciod-side readers of cl_cb_session (encode_cb_sequence4args(),
+ * nfsd4_cb_sequence_done(), the cb-slot helpers, and the cb_sequence
+ * tracepoints) run outside cl_callback_wq.  The
+ * nfsd41_cb_inflight_wait_complete() drain in nfsd4_probe_callback_sync()
+ * waits until cl_cb_inflight reaches zero before the caller proceeds with
+ * session teardown; any rpc_task that reads cl_cb_session must hold an
+ * inflight pin (via nfsd41_cb_inflight_begin) for this fence to be
+ * effective.
  */
 static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 {
@@ -1731,6 +1743,7 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 		nfsd4_mark_cb_down(clp);
 		if (c)
 			svc_xprt_put(c->cn_xprt);
+		clp->cl_cb_session = NULL;
 		return;
 	}
 }

-- 
2.54.0