[PATCH 0/5] SUNRPC: Fix backchannel transport lifetime bugs

[PATCH 0/5] SUNRPC: Fix backchannel transport lifetime bugs

[Chuck Lever]

Wentao Liang posted a fix for a reference leak in the RDMA
backchannel transport:

https://patch.msgid.link/20261104022813.1484267-1-vulab@iscas.ac.cn

Since commit 729580ddc53e ("svcrdma: xpt_bc_xprt is already clear in
__svc_rdma_free()"), xprt_setup_rdma_bc() takes one more reference
than anything releases, so no RDMA backchannel rpc_xprt is ever freed.

Applying Wentao's fix brings the final put back into play. The last
put queues xprt_destroy_cb(), which calls xprt_rdma_bc_put() in
rpcrdma, and nothing holds a module reference across that work item.
An rmmod of rpcrdma can then race the work item and run freed module
text. The module reference added right after Wentao's fix closes that
race, so the two need to land together.

This repost keeps Wentao's code as posted, with a reworded commit
message.

The series also addresses some pre-existing issues in the same area
that turned up while reviewing Wentao's patch: a NULL xprt_class on
the RDMA backchannel transport, an add_xprt sysfs write that rebinds
xpt_bc_xprt, and an xprt_create_transport() error path that leaves
xpt_bc_xprt pointing at a freed transport.

The add_xprt rejection has to come before setting xprt_class.
Currently an add_xprt write on the xprt_switch of an RDMA callback
client oopses on the NULL xprt_class. Setting xprt_class without the
rejection turns the oops into a leak of the previous backchannel
rpc_xprt. With the module reference in place, the leak also keeps
rpcrdma from being unloaded.

The SUNRPC patches change client-side code: the sysfs add_xprt handler
and xprt_create_transport(). Because of the ordering constraints
above, can the whole series, Wentao's fix included, go through the
NFS client tree?

---
Chuck Lever (4):
      SUNRPC: Reject add_xprt on a backchannel xprt_switch
      svcrdma: Hold a module reference for the backchannel transport
      svcrdma: Set xprt_class on the backchannel transport
      SUNRPC: Set up the servername before the transport is created

Wentao Liang (1):
      svcrdma: drop the extra reference on the backchannel transport

 net/sunrpc/sysfs.c                         |  6 ++++++
 net/sunrpc/xprt.c                          | 23 ++++++++++++-----------
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c | 12 ++++++++----
 3 files changed, 26 insertions(+), 15 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261104-liange-1630f2aaf1e9

Best regards,
--  
Chuck Lever

[PATCH 1/5] SUNRPC: Reject add_xprt on a backchannel xprt_switch

[Chuck Lever]

The add_xprt sysfs file clones the main transport of an xprt_switch,
including its bc_xprt. On the xprt_switch of an NFSv4.1 callback
client, the clone runs a backchannel setup routine against the main
transport's svc_xprt. xs_setup_bc_tcp() overwrites xpt_bc_xprt
without releasing the reference the original rpc_xprt held there, so
the original rpc_xprt leaks along with its netns and module
references. On an RDMA callback client the same write oopses
instead, because the RDMA backchannel transport leaves xprt_class
NULL.

A backchannel transport rides on its svc_xprt and has no connection
of its own to clone. Return -EOPNOTSUPP when the main transport has
a bc_xprt, before reading its xprt_class, as
rpc_sysfs_xprt_dstaddr_store() does for transports other than TCP
and RDMA.

Fixes: df210d9b0951 ("sunrpc: Add a sysfs file for adding a new xprt")
Cc: stable@vger.kernel.org
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/sunrpc/sysfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index e638b92b7ad1..90a73ce261d8 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -337,6 +337,12 @@ static ssize_t rpc_sysfs_xprt_switch_add_xprt_store(struct kobject *kobj,
 	if (!xprt)
 		goto out;
 
+	/* Backchannel setup would rebind the svc_xprt's xpt_bc_xprt. */
+	if (xprt->bc_xprt) {
+		count = -EOPNOTSUPP;
+		goto out_put_xprt;
+	}
+
 	xprt_create_args.ident = xprt->xprt_class->ident;
 	xprt_create_args.net = xprt->xprt_net;
 	xprt_create_args.dstaddr = (struct sockaddr *)&xprt->addr;

-- 
2.54.0

Re: [PATCH 1/5] SUNRPC: Reject add_xprt on a backchannel xprt_switch

[sashiko-bot]

> The add_xprt sysfs file clones the main transport of an xprt_switch,
> including its bc_xprt. On the xprt_switch of an NFSv4.1 callback
> client, the clone runs a backchannel setup routine against the main
> transport's svc_xprt. xs_setup_bc_tcp() overwrites xpt_bc_xprt
> without releasing the reference the original rpc_xprt held there, so
> the original rpc_xprt leaks along with its netns and module
> references. On an RDMA callback client the same write oopses
> instead, because the RDMA backchannel transport leaves xprt_class
> NULL.
> 
> A backchannel transport rides on its svc_xprt and has no connection
> of its own to clone. Return -EOPNOTSUPP when the main transport has
> a bc_xprt, before reading its xprt_class, as
> rpc_sysfs_xprt_dstaddr_store() does for transports other than TCP
> and RDMA.
> 
> Fixes: df210d9b0951 ("sunrpc: Add a sysfs file for adding a new xprt")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chuck Lever <cel@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-liange-v1-0-f097e10cea1b@kernel.org?part=1

[PATCH 2/5] svcrdma: drop the extra reference on the backchannel transport

[Chuck Lever]

From: Wentao Liang <vulab@iscas.ac.cn>

Beyond the reference xprt_alloc() returns to the caller,
xprt_setup_rdma_bc() takes two more on the new rpc_xprt: one for the
xpt_bc_xprt link, which svc_xprt_free() drops, and a second whose put
was in __svc_rdma_free(). Commit 729580ddc53e ("svcrdma: xpt_bc_xprt
is already clear in __svc_rdma_free()") removed that put as a
duplicate of the one in svc_xprt_free(), but left the matching
xprt_get() in place. The second reference is therefore never
released: the rpc_xprt outlives the callback client and the
connection, and the net namespace reference taken by xprt_alloc()
leaks along with the rpc_xprt.

Take only the reference that svc_xprt_free() releases, as
xs_setup_bc_tcp() does.

Fixes: 729580ddc53e ("svcrdma: xpt_bc_xprt is already clear in __svc_rdma_free()")
Cc: stable@vger.kernel.org
Assisted-by: DeepSeek-V4.1-Flash and OpenAI Codex
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
Link: https://patch.msgid.link/20261104022813.1484267-1-vulab@iscas.ac.cn
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
index e5a78b761012..6a504806165c 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
@@ -269,9 +269,6 @@ xprt_setup_rdma_bc(struct xprt_create *args)
 	xprt_get(xprt);
 	args->bc_xprt->xpt_bc_xprt = xprt;
 	xprt->bc_xprt = args->bc_xprt;
-
-	/* Final put for backchannel xprt is in __svc_rdma_free */
-	xprt_get(xprt);
 	return xprt;
 }
 

-- 
2.54.0

Re: [PATCH 2/5] svcrdma: drop the extra reference on the backchannel transport

[sashiko-bot]

> From: Wentao Liang <vulab@iscas.ac.cn>
> 
> Beyond the reference xprt_alloc() returns to the caller,
> xprt_setup_rdma_bc() takes two more on the new rpc_xprt: one for the
> xpt_bc_xprt link, which svc_xprt_free() drops, and a second whose put
> was in __svc_rdma_free(). Commit 729580ddc53e ("svcrdma: xpt_bc_xprt
> is already clear in __svc_rdma_free()") removed that put as a
> duplicate of the one in svc_xprt_free(), but left the matching
> xprt_get() in place. The second reference is therefore never
> released: the rpc_xprt outlives the callback client and the
> connection, and the net namespace reference taken by xprt_alloc()
> leaks along with the rpc_xprt.
> 
> Take only the reference that svc_xprt_free() releases, as
> xs_setup_bc_tcp() does.
> 
> Fixes: 729580ddc53e ("svcrdma: xpt_bc_xprt is already clear in __svc_rdma_free()")
> Cc: stable@vger.kernel.org
> Assisted-by: DeepSeek-V4.1-Flash and OpenAI Codex
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> Link: https://patch.msgid.link/20261104022813.1484267-1-vulab@iscas.ac.cn
> Signed-off-by: Chuck Lever <cel@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-liange-v1-0-f097e10cea1b@kernel.org?part=2

[PATCH 3/5] svcrdma: Hold a module reference for the backchannel transport

[Chuck Lever]

The last put on an RDMA backchannel rpc_xprt runs xprt_destroy(),
which queues xprt_destroy_cb() on a workqueue. That work item calls
xprt_rdma_bc_put() through xprt->ops. Nothing pins the rpcrdma
module while the rpc_xprt exists: xprt_create_transport() drops the
class reference as soon as ->setup() returns, and svc_xprt_free()
releases its own module reference right after the final put. An
rmmod of rpcrdma can therefore race the pending work item and run
freed module text. The TCP backchannel in xs_setup_bc_tcp() and the
forward transport in xprt_setup_rdma() both hold a module reference
for the life of the rpc_xprt.

Take a module reference in xprt_setup_rdma_bc() and release it in
xprt_rdma_bc_put(), as xprt_setup_rdma() and xprt_rdma_destroy() do.

Fixes: 5d252f90a800 ("svcrdma: Add class for RDMA backwards direction transport")
Cc: stable@vger.kernel.org
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
index 6a504806165c..6089d4ac85a5 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
@@ -205,6 +205,7 @@ xprt_rdma_bc_put(struct rpc_xprt *xprt)
 {
 	xprt_rdma_free_addresses(xprt);
 	xprt_free(xprt);
+	module_put(THIS_MODULE);
 }
 
 static const struct rpc_xprt_ops xprt_rdma_bc_procs = {
@@ -240,11 +241,16 @@ xprt_setup_rdma_bc(struct xprt_create *args)
 	if (args->addrlen > sizeof(xprt->addr))
 		return ERR_PTR(-EBADF);
 
+	if (!try_module_get(THIS_MODULE))
+		return ERR_PTR(-EIO);
+
 	xprt = xprt_alloc(args->net, sizeof(*new_xprt),
 			  RPCRDMA_MAX_BC_REQUESTS,
 			  RPCRDMA_MAX_BC_REQUESTS);
-	if (!xprt)
+	if (!xprt) {
+		module_put(THIS_MODULE);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	xprt->timeout = &xprt_rdma_bc_timeout;
 	xprt_set_bound(xprt);

-- 
2.54.0

Re: [PATCH 3/5] svcrdma: Hold a module reference for the backchannel transport

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] svcrdma: inaccurate Fixes tag in commit message
--

commit 5aa6827a1ec45b94a0a4813a38c3cc653c0334bf
Author: Chuck Lever <cel@kernel.org>

svcrdma: Hold a module reference for the backchannel transport

This commit takes a module reference in xprt_setup_rdma_bc() and releases
it in xprt_rdma_bc_put() so that the rpcrdma module stays pinned while the
backchannel rpc_xprt exists and its deferred destruction work item runs.

>     svcrdma: Hold a module reference for the backchannel transport
>     
>     The last put on an RDMA backchannel rpc_xprt runs xprt_destroy(),
>     which queues xprt_destroy_cb() on a workqueue. That work item calls
>     xprt_rdma_bc_put() through xprt->ops. Nothing pins the rpcrdma
>     module while the rpc_xprt exists: xprt_create_transport() drops the
>     class reference as soon as ->setup() returns, and svc_xprt_free()
>     releases its own module reference right after the final put. An
>     rmmod of rpcrdma can therefore race the pending work item and run
>     freed module text. The TCP backchannel in xs_setup_bc_tcp() and the
>     forward transport in xprt_setup_rdma() both hold a module reference
>     for the life of the rpc_xprt.
>     
>     Take a module reference in xprt_setup_rdma_bc() and release it in
>     xprt_rdma_bc_put(), as xprt_setup_rdma() and xprt_rdma_destroy() do.
>     
>     Fixes: 5d252f90a800 ("svcrdma: Add class for RDMA backwards direction transport")
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Chuck Lever <cel@kernel.org>

[Severity: Low]
In net/sunrpc/xprtrdma/svc_rdma_backchannel.c, commit 5d252f90a800
("svcrdma: Add class for RDMA backwards direction transport") originally
included both try_module_get(THIS_MODULE) in xprt_setup_rdma_bc() and
module_put(THIS_MODULE) in xprt_rdma_bc_put().

Those calls were later removed by commit 07880fa4968b ("svcrdma: Remove
try_module_get from backchannel") when svcrdma and xprtrdma were merged
into a single module.

Should the Fixes tag reference commit 07880fa4968b instead so stable
backporting targets the commit that removed the module reference?

  Fixes: 07880fa4968b ("svcrdma: Remove try_module_get from backchannel")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-liange-v1-0-f097e10cea1b@kernel.org?part=3

[PATCH 4/5] svcrdma: Set xprt_class on the backchannel transport

[Chuck Lever]

Commit d3abc73987fd ("sunrpc: keep track of the xprt_class in
rpc_xprt structure") added the xprt_class field and filled it in
every xprtsock setup function, including the TCP backchannel one,
but left xprt_setup_rdma_bc() alone. The RDMA backchannel
transport therefore carries a NULL xprt_class.

The SUNRPC sysfs code dereferences the field unconditionally. The
backchannel rpc_clnt is registered in sysfs like any other, so a
root write to the dstaddr attribute of an RDMA backchannel
transport dereferences NULL in rpc_sysfs_xprt_dstaddr_store().

Point xprt_class at xprt_rdma_bc, as xs_setup_bc_tcp() does for
its class. After the change, dstaddr rejects the RDMA backchannel
transport with EOPNOTSUPP, as it already does for TCP.

Fixes: d3abc73987fd ("sunrpc: keep track of the xprt_class in rpc_xprt structure")
Cc: stable@vger.kernel.org
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
index 6089d4ac85a5..6a0ef349a469 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
@@ -261,6 +261,7 @@ xprt_setup_rdma_bc(struct xprt_create *args)
 
 	xprt->prot = XPRT_TRANSPORT_BC_RDMA;
 	xprt->ops = &xprt_rdma_bc_procs;
+	xprt->xprt_class = &xprt_rdma_bc;
 
 	memcpy(&xprt->addr, args->dstaddr, args->addrlen);
 	xprt->addrlen = args->addrlen;

-- 
2.54.0

Re: [PATCH 4/5] svcrdma: Set xprt_class on the backchannel transport

[sashiko-bot]

> Commit d3abc73987fd ("sunrpc: keep track of the xprt_class in
> rpc_xprt structure") added the xprt_class field and filled it in
> every xprtsock setup function, including the TCP backchannel one,
> but left xprt_setup_rdma_bc() alone. The RDMA backchannel
> transport therefore carries a NULL xprt_class.
> 
> The SUNRPC sysfs code dereferences the field unconditionally. The
> backchannel rpc_clnt is registered in sysfs like any other, so a
> root write to the dstaddr attribute of an RDMA backchannel
> transport dereferences NULL in rpc_sysfs_xprt_dstaddr_store().
> 
> Point xprt_class at xprt_rdma_bc, as xs_setup_bc_tcp() does for
> its class. After the change, dstaddr rejects the RDMA backchannel
> transport with EOPNOTSUPP, as it already does for TCP.
> 
> Fixes: d3abc73987fd ("sunrpc: keep track of the xprt_class in rpc_xprt structure")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chuck Lever <cel@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-liange-v1-0-f097e10cea1b@kernel.org?part=4

[PATCH 5/5] SUNRPC: Set up the servername before the transport is created

[Chuck Lever]

xprt_create_transport() checks the servername length and copies the
string only after the class setup() routine has returned. A
backchannel setup() routine publishes the new transport in the
svc_xprt's xpt_bc_xprt field before it returns, and svc_xprt_free()
puts that pointer when the svc_xprt goes away. When the length
check or the kstrdup() then fails, xprt_create_transport() calls
xprt_destroy(), which ignores the reference count and frees the
transport. xpt_bc_xprt is left pointing at freed memory, and
svc_xprt_free() later puts a dangling reference.

The setup() routines never read the servername, so validate and
duplicate it before the transport is created. After the change, no
failure remains between setup() and the return, and the
xprt_destroy() calls on the error path go away with it.

Fixes: 4e0038b6b246 ("SUNRPC: Move clnt->cl_server into struct rpc_xprt")
Cc: stable@vger.kernel.org
Signed-off-by: Chuck Lever <cel@kernel.org>
---
 net/sunrpc/xprt.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 48a3618cbb29..a50fdd84daef 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -2076,18 +2076,29 @@ struct rpc_xprt *xprt_create_transport(struct xprt_create *args)
 {
 	struct rpc_xprt	*xprt;
 	const struct xprt_class *t;
+	char *servername;
+
+	if (strlen(args->servername) > RPC_MAXNETNAMELEN)
+		return ERR_PTR(-EINVAL);
+	servername = kstrdup(args->servername, GFP_KERNEL);
+	if (!servername)
+		return ERR_PTR(-ENOMEM);
 
 	t = xprt_class_find_by_ident(args->ident);
 	if (!t) {
 		dprintk("RPC: transport (%d) not supported\n", args->ident);
+		kfree(servername);
 		return ERR_PTR(-EIO);
 	}
 
 	xprt = t->setup(args);
 	xprt_class_release(t);
 
-	if (IS_ERR(xprt))
+	if (IS_ERR(xprt)) {
+		kfree(servername);
 		goto out;
+	}
+	xprt->servername = servername;
 	if (args->flags & XPRT_CREATE_NO_IDLE_TIMEOUT)
 		xprt->idle_timeout = 0;
 	INIT_WORK(&xprt->task_cleanup, xprt_autoclose);
@@ -2096,16 +2107,6 @@ struct rpc_xprt *xprt_create_transport(struct xprt_create *args)
 	else
 		timer_setup(&xprt->timer, NULL, 0);
 
-	if (strlen(args->servername) > RPC_MAXNETNAMELEN) {
-		xprt_destroy(xprt);
-		return ERR_PTR(-EINVAL);
-	}
-	xprt->servername = kstrdup(args->servername, GFP_KERNEL);
-	if (xprt->servername == NULL) {
-		xprt_destroy(xprt);
-		return ERR_PTR(-ENOMEM);
-	}
-
 	rpc_xprt_debugfs_register(xprt);
 
 	trace_xprt_create(xprt);

-- 
2.54.0

Re: [PATCH 5/5] SUNRPC: Set up the servername before the transport is created

[sashiko-bot]

> xprt_create_transport() checks the servername length and copies the
> string only after the class setup() routine has returned. A
> backchannel setup() routine publishes the new transport in the
> svc_xprt's xpt_bc_xprt field before it returns, and svc_xprt_free()
> puts that pointer when the svc_xprt goes away. When the length
> check or the kstrdup() then fails, xprt_create_transport() calls
> xprt_destroy(), which ignores the reference count and frees the
> transport. xpt_bc_xprt is left pointing at freed memory, and
> svc_xprt_free() later puts a dangling reference.
> 
> The setup() routines never read the servername, so validate and
> duplicate it before the transport is created. After the change, no
> failure remains between setup() and the return, and the
> xprt_destroy() calls on the error path go away with it.
> 
> Fixes: 4e0038b6b246 ("SUNRPC: Move clnt->cl_server into struct rpc_xprt")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chuck Lever <cel@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-liange-v1-0-f097e10cea1b@kernel.org?part=5