* [PATCH 1/2] SUNRPC: release lower rpc_clnt if killed waiting for XPRT_LOCKED
2026-05-04 10:28 [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Chuck Lever
@ 2026-05-04 10:28 ` Chuck Lever
2026-05-04 10:28 ` [PATCH 2/2] SUNRPC: pin upper rpc_clnt across the TLS connect_worker Chuck Lever
2026-05-04 22:09 ` [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Michael Nemanov
2 siblings, 0 replies; 4+ messages in thread
From: Chuck Lever @ 2026-05-04 10:28 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman
Cc: linux-nfs, netdev, Chuck Lever
From: Chuck Lever <chuck.lever@oracle.com>
xs_tcp_tls_setup_socket() creates a temporary "lower" rpc_clnt with
rpc_create() to drive the inner TLS handshake, then waits for
XPRT_LOCKED on its xprt with TASK_KILLABLE so a stuck handshake can
be aborted by signal. When the wait is interrupted, the function
jumps to out_unlock without releasing lower_clnt. The success path
and the out_close error path both call
rpc_shutdown_client(lower_clnt); only the killed-wait path skips it,
leaking the clnt and its underlying xprt.
Call rpc_shutdown_client() on this path before joining out_unlock.
xprt_release_write() is not needed here because XPRT_LOCKED was
never acquired.
Fixes: 26e8bfa30dac ("SUNRPC/TLS: Lock the lower_xprt during the tls handshake")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
net/sunrpc/xprtsock.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 2e1fe6013361..3eccd4923e6c 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2734,8 +2734,11 @@ static void xs_tcp_tls_setup_socket(struct work_struct *work)
lower_xprt = rcu_dereference(lower_clnt->cl_xprt);
rcu_read_unlock();
- if (wait_on_bit_lock(&lower_xprt->state, XPRT_LOCKED, TASK_KILLABLE))
+ if (wait_on_bit_lock(&lower_xprt->state, XPRT_LOCKED, TASK_KILLABLE)) {
+ /* XPRT_LOCKED was never acquired. */
+ rpc_shutdown_client(lower_clnt);
goto out_unlock;
+ }
status = xs_tls_handshake_sync(lower_xprt, &upper_xprt->xprtsec);
if (status) {
--
2.53.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] SUNRPC: pin upper rpc_clnt across the TLS connect_worker
2026-05-04 10:28 [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Chuck Lever
2026-05-04 10:28 ` [PATCH 1/2] SUNRPC: release lower rpc_clnt if killed waiting for XPRT_LOCKED Chuck Lever
@ 2026-05-04 10:28 ` Chuck Lever
2026-05-04 22:09 ` [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Michael Nemanov
2 siblings, 0 replies; 4+ messages in thread
From: Chuck Lever @ 2026-05-04 10:28 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman
Cc: linux-nfs, netdev, Chuck Lever, Michael Nemanov
From: Chuck Lever <chuck.lever@oracle.com>
The TLS connect path has a use-after-free: nothing pins the
upper rpc_clnt across the delayed connect_worker. xs_connect()
stores task->tk_client in sock_xprt::clnt as a raw pointer
and queues the worker; for TLS-secured transports that worker
is xs_tcp_tls_setup_socket(), which reads several fields out
of the saved pointer (cl_timeout, cl_program, cl_prog,
cl_vers, cl_cred, cl_stats) to construct the args for the
inner handshake rpc_clnt.
The xprt does not reference the rpc_clnt; the rpc_clnt
references the xprt. xs_destroy() does cancel the
connect_worker, but it runs only when the xprt's refcount
drops to zero, which cannot happen until the rpc_clnt
releases its cl_xprt reference in rpc_free_client_work().
When a TLS handshake fails fatally (for example, an mTLS
mount whose client cert does not match the server), the
connecting task is woken with -EACCES and exits, the mount
caller invokes rpc_shutdown_client(), and the upper rpc_clnt
is freed before the queued connect_worker fires.
xs_tcp_tls_setup_socket() then dereferences the freed clnt,
producing the refcount_t underflow Michael Nemanov reported.
Take a reference on the upper rpc_clnt in xs_connect() for
TLS transports via a new rpc_hold_client() helper, and drop
it in the connect_worker's exit path with rpc_release_client().
The xprt_lock_connect() / xprt_unlock_connect() pairing
already serialises xs_connect() with xs_tcp_tls_setup_socket(),
so the take and release are balanced one-for-one.
The non-TLS connect worker (xs_tcp_setup_socket) never reads
sock_xprt::clnt, so leave that path alone and avoid the
clnt-holds-xprt-holds-clnt cycle that would otherwise prevent
xprt destruction.
Reported-by: Michael Nemanov <michael.nemanov@vastdata.com>
Closes: https://lore.kernel.org/linux-nfs/40e3d522-dfcf-4fc1-9c55-b5e81f1536d5@vastdata.com/
Fixes: 75eb6af7acdf ("SUNRPC: Add a TCP-with-TLS RPC transport class")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
include/linux/sunrpc/clnt.h | 1 +
net/sunrpc/clnt.c | 19 +++++++++++++++++--
net/sunrpc/xprtsock.c | 11 ++++++++++-
3 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index f8b406b0a1af..3c2b8c355ab3 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -190,6 +190,7 @@ int rpc_switch_client_transport(struct rpc_clnt *,
const struct rpc_timeout *);
void rpc_shutdown_client(struct rpc_clnt *);
+void rpc_hold_client(struct rpc_clnt *);
void rpc_release_client(struct rpc_clnt *);
void rpc_task_release_transport(struct rpc_task *);
void rpc_task_release_client(struct rpc_task *);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index bc8ca470718b..efa26899bc7d 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1026,8 +1026,23 @@ rpc_free_auth(struct rpc_clnt *clnt)
return NULL;
}
-/*
- * Release reference to the RPC client
+/**
+ * rpc_hold_client - acquire a reference on an rpc_clnt
+ * @clnt: rpc_clnt to pin
+ *
+ * Pairs with rpc_release_client().
+ */
+void rpc_hold_client(struct rpc_clnt *clnt)
+{
+ refcount_inc(&clnt->cl_count);
+}
+
+/**
+ * rpc_release_client - release a reference on an rpc_clnt
+ * @clnt: rpc_clnt to release
+ *
+ * Pairs with rpc_hold_client(). The rpc_clnt's resources are
+ * freed once its reference count drops to zero.
*/
void
rpc_release_client(struct rpc_clnt *clnt)
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 3eccd4923e6c..359407aae03e 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2761,6 +2761,7 @@ static void xs_tcp_tls_setup_socket(struct work_struct *work)
out_unlock:
current_restore_flags(pflags, PF_MEMALLOC);
upper_transport->clnt = NULL;
+ rpc_release_client(upper_clnt);
xprt_unlock_connect(upper_xprt, upper_transport);
return;
@@ -2808,7 +2809,15 @@ static void xs_connect(struct rpc_xprt *xprt, struct rpc_task *task)
} else
dprintk("RPC: xs_connect scheduled xprt %p\n", xprt);
- transport->clnt = task->tk_client;
+ /*
+ * Only the TLS connect_worker reads transport->clnt; pinning
+ * the upper rpc_clnt unconditionally would form a cycle with
+ * cl_xprt and prevent xprt destruction.
+ */
+ if (xprt->xprtsec.policy != RPC_XPRTSEC_NONE) {
+ rpc_hold_client(task->tk_client);
+ transport->clnt = task->tk_client;
+ }
queue_delayed_work(xprtiod_workqueue,
&transport->connect_worker,
delay);
--
2.53.0
^ permalink raw reply related [flat|nested] 4+ messages in thread