[PATCH net 0/2] net/rds: fix init failures leaving built-in transports on a dead core

[PATCH net 0/2] net/rds: fix init failures leaving built-in transports on a dead core

[Allison Henderson]

The kernel test robot hit a slab-use-after-free on rds_wq at boot with
CONFIG_SYSCTL=n and RDS, RDS_RDMA and RDS_TCP built in [1].  Two
separate bugs are involved.

With CONFIG_SYSCTL=n, register_net_sysctl() is a stub that returns
NULL, and RDS treats that as -ENOMEM, so rds_init() fails even though
every tunable has a usable default.  Patch 1 only treats NULL as a
failure when sysctl support is built in.

Second, when RDS is built in, a failed rds_init() does not stop the
rds_rdma and rds_tcp initcalls.  They run against a core that has
already been torn down and queue onto or flush the freed rds_wq.  The
error path of rds_ib_init() did this in the robot's report.  The
rds_tcp listen socket can also do it when an incoming connection
arrives.  Patch 2 makes both transports bail out with -ENODEV unless
rds_init() succeeded.  Modular builds can't hit this, because the
transports need symbols from rds.ko.

Thanks to Oleg Nesterov and Joel Granados for the analysis in [2].

Testing: x86_64 under virtme-ng, built in RDS/RDS_RDMA/RDS_TCP,
CONFIG_SYSCTL=n, KASAN, booted with initcall_debug:
  - net/main: rds_init() returns -ENOMEM, then rds_rdma_init() hits
    KASAN slab-use-after-free in __flush_workqueue() from rds_ib_init(),
    reproducing [1].
  - patch 2 only: rds_init() still returns -ENOMEM; rds_rdma_init() and
    rds_tcp_init() return -ENODEV and the boot completes cleanly.
  - both patches: rds_init(), rds_rdma_init() and rds_tcp_init() all
    return 0, no KASAN reports.

[1] https://lore.kernel.org/oe-lkp/202610190310.9737b2e3-lkp@intel.com
[2] https://lore.kernel.org/all/atdoGiW_sHlWgOwt@redhat.com/


Allison Henderson (2):
  net/rds: do not fail init when CONFIG_SYSCTL=n
  net/rds: do not initialize transports after rds_init() failed

 net/rds/af_rds.c         | 14 ++++++++++++++
 net/rds/ib_sysctl.c      |  2 +-
 net/rds/rdma_transport.c |  3 +++
 net/rds/rds.h            |  1 +
 net/rds/sysctl.c         |  2 +-
 net/rds/tcp.c            |  5 ++++-
 6 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.50.1

[PATCH net 1/2] net/rds: do not fail init when CONFIG_SYSCTL=n

[Allison Henderson]

With CONFIG_SYSCTL=n, register_net_sysctl() and register_net_sysctl_sz()
are inline stubs that return NULL.  rds_sysctl_init(),
rds_ib_sysctl_init() and rds_tcp_init_net() all treat that NULL as
-ENOMEM, so RDS refuses to initialize at all even though every tunable
has a sane compiled-in default.

When RDS is built in, that is worse than a missing feature: the failed
rds_init() destroys rds_wq, but the rds_rdma initcall still runs, and
when rds_ib_sysctl_init() fails the same way, the error path in
rds_ib_init() flushes the already freed workqueue:

  BUG: KASAN: slab-use-after-free in touch_wq_lockdep_map
  ...
   __flush_workqueue
   rds_ib_init
   rds_rdma_init
  ...
  Allocated by task 1:
   alloc_workqueue_noprof
   rds_threads_init
   rds_init
  ...
  Freed by task 0:
   kfree
   rcu_do_batch

Only treat the NULL return as a failure when sysctl support is actually
compiled in.  The exit paths need no change: unregister_net_sysctl_table()
is an empty stub with CONFIG_SYSCTL=n and already tolerates a NULL header
otherwise.

The underlying problem of built-in transports initializing after
rds_init() has failed is independent of sysctl and is addressed
separately.

Reported-by: kernel test robot <yi1.lai@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202610190310.9737b2e3-lkp@intel.com
Fixes: 3e5048495c85 ("RDS: sysctls")
Fixes: e6babe4cc4ce ("RDS/IB: Stats and sysctls")
Fixes: c6a58ffed536 ("RDS: TCP: Add sysctl tunables for sndbuf/rcvbuf on rds-tcp socket")
Suggested-by: Joel Granados <joel.granados@kernel.org>
Assisted-by: LLM
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/ib_sysctl.c | 2 +-
 net/rds/sysctl.c    | 2 +-
 net/rds/tcp.c       | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/rds/ib_sysctl.c b/net/rds/ib_sysctl.c
index 2af678e71e3c..bc97a3eecad6 100644
--- a/net/rds/ib_sysctl.c
+++ b/net/rds/ib_sysctl.c
@@ -114,7 +114,7 @@ void rds_ib_sysctl_exit(void)
 int rds_ib_sysctl_init(void)
 {
 	rds_ib_sysctl_hdr = register_net_sysctl(&init_net, "net/rds/ib", rds_ib_sysctl_table);
-	if (!rds_ib_sysctl_hdr)
+	if (!rds_ib_sysctl_hdr && IS_ENABLED(CONFIG_SYSCTL))
 		return -ENOMEM;
 	return 0;
 }
diff --git a/net/rds/sysctl.c b/net/rds/sysctl.c
index 025f518a4349..968d6279bbd3 100644
--- a/net/rds/sysctl.c
+++ b/net/rds/sysctl.c
@@ -103,7 +103,7 @@ int rds_sysctl_init(void)
 
 	rds_sysctl_reg_table =
 		register_net_sysctl(&init_net, "net/rds", rds_sysctl_rds_table);
-	if (!rds_sysctl_reg_table)
+	if (!rds_sysctl_reg_table && IS_ENABLED(CONFIG_SYSCTL))
 		return -ENOMEM;
 	return 0;
 }
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 774a71f88d37..34815b4f658b 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -637,7 +637,7 @@ static __net_init int rds_tcp_init_net(struct net *net)
 	tbl[RDS_TCP_RCVBUF].data = &rtn->rcvbuf_size;
 	rtn->rds_tcp_sysctl = register_net_sysctl_sz(net, "net/rds/tcp", tbl,
 						     ARRAY_SIZE(rds_tcp_sysctl_table));
-	if (!rtn->rds_tcp_sysctl) {
+	if (!rtn->rds_tcp_sysctl && IS_ENABLED(CONFIG_SYSCTL)) {
 		pr_warn("could not register sysctl\n");
 		err = -ENOMEM;
 		goto fail;
-- 
2.50.1

[PATCH net 2/2] net/rds: do not initialize transports after rds_init() failed

[Allison Henderson]

rds, rds_rdma and rds_tcp each have their own module_init().  When they
are built in, those run in link order, so rds_init() goes first, but
do_initcalls() does not stop when it fails.  The transports then
initialize against a core that has already been torn down: rds_wq has
been destroyed, yet rds_ib_dev_put(), rds_ib_unregister_client() and
rds_tcp_accept_work() still queue onto or flush it, and
rds_trans_register() hands the transports to a core whose socket
family may never have been registered.

This was seen with CONFIG_SYSCTL=n, where rds_sysctl_init() failed and
rds_ib_init()'s error path then flushed the freed rds_wq [1], but any
rds_init() failure after rds_threads_init() has the same effect.

Record whether rds_init() succeeded and have rds_rdma_init() and
rds_tcp_init() bail out with -ENODEV otherwise.  Modular builds are
unaffected: the transports need symbols exported by rds.ko, so they
cannot be loaded unless rds_init() succeeded.

[1] https://lore.kernel.org/oe-lkp/202610190310.9737b2e3-lkp@intel.com
Fixes: 55b7ed0b582f ("RDS: Common RDMA transport code")
Fixes: 70041088e3b9 ("RDS: Add TCP transport to RDS")
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Assisted-by: LLM
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/af_rds.c         | 14 ++++++++++++++
 net/rds/rdma_transport.c |  3 +++
 net/rds/rds.h            |  1 +
 net/rds/tcp.c            |  3 +++
 4 files changed, 21 insertions(+)

diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index d5defe9172e3..840587c05178 100644
--- a/net/rds/af_rds.c
+++ b/net/rds/af_rds.c
@@ -922,8 +922,11 @@ static void rds6_sock_info(struct socket *sock, unsigned int len,
 }
 #endif
 
+static bool rds_initialized;
+
 static void rds_exit(void)
 {
+	rds_initialized = false;
 	sock_unregister(rds_family_ops.family);
 	proto_unregister(&rds_proto);
 	rds_conn_exit();
@@ -944,6 +947,16 @@ module_exit(rds_exit);
 
 u32 rds_gen_num;
 
+/* When RDS is built in, a failed rds_init() does not stop the rds_rdma
+ * and rds_tcp initcalls from running against a core that has already
+ * been torn down (rds_wq in particular).  Let them bail out early.
+ */
+bool rds_is_initialized(void)
+{
+	return rds_initialized;
+}
+EXPORT_SYMBOL_GPL(rds_is_initialized);
+
 static int __init rds_init(void)
 {
 	int ret;
@@ -980,6 +993,7 @@ static int __init rds_init(void)
 	rds_info_register_func(RDS6_INFO_SOCKETS, rds6_sock_info);
 	rds_info_register_func(RDS6_INFO_RECV_MESSAGES, rds6_sock_inc_info);
 #endif
+	rds_initialized = true;
 
 	goto out;
 
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index 7be38be1228e..cc6a7d29791a 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -325,6 +325,9 @@ static int __init rds_rdma_init(void)
 {
 	int ret;
 
+	if (!rds_is_initialized())
+		return -ENODEV;
+
 	ret = rds_ib_init();
 	if (ret)
 		goto out;
diff --git a/net/rds/rds.h b/net/rds/rds.h
index 2db49573dacd..b7ef7c8b1e03 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -1040,6 +1040,7 @@ extern unsigned int  rds_sysctl_trace_level;
 int rds_threads_init(void);
 void rds_threads_exit(void);
 extern struct workqueue_struct *rds_wq;
+bool rds_is_initialized(void);
 void rds_queue_reconnect(struct rds_conn_path *cp);
 void rds_connect_worker(struct work_struct *);
 void rds_shutdown_worker(struct work_struct *);
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 34815b4f658b..c1ad37505417 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -815,6 +815,9 @@ static int __init rds_tcp_init(void)
 {
 	int ret;
 
+	if (!rds_is_initialized())
+		return -ENODEV;
+
 	rds_tcp_conn_slab = KMEM_CACHE(rds_tcp_connection, 0);
 	if (!rds_tcp_conn_slab) {
 		ret = -ENOMEM;
-- 
2.50.1

Re: [PATCH net 1/2] net/rds: do not fail init when CONFIG_SYSCTL=n

[sashiko-bot]

> With CONFIG_SYSCTL=n, register_net_sysctl() and register_net_sysctl_sz()
> are inline stubs that return NULL.  rds_sysctl_init(),
> rds_ib_sysctl_init() and rds_tcp_init_net() all treat that NULL as
> -ENOMEM, so RDS refuses to initialize at all even though every tunable
> has a sane compiled-in default.
> 
> When RDS is built in, that is worse than a missing feature: the failed
> rds_init() destroys rds_wq, but the rds_rdma initcall still runs, and
> when rds_ib_sysctl_init() fails the same way, the error path in
> rds_ib_init() flushes the already freed workqueue:
> 
>   BUG: KASAN: slab-use-after-free in touch_wq_lockdep_map
>   ...
>    __flush_workqueue
>    rds_ib_init
> [ ... ]
> Fixes: e6babe4cc4ce ("RDS/IB: Stats and sysctls")
> Fixes: c6a58ffed536 ("RDS: TCP: Add sysctl tunables for sndbuf/rcvbuf on rds-tcp socket")
> Suggested-by: Joel Granados <joel.granados@kernel.org>
> Assisted-by: LLM
> Signed-off-by: Allison Henderson <achender@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021195837.142802-1-achender@kernel.org?part=1

Re: [PATCH net 2/2] net/rds: do not initialize transports after rds_init() failed

[sashiko-bot]

> rds, rds_rdma and rds_tcp each have their own module_init().  When they
> are built in, those run in link order, so rds_init() goes first, but
> do_initcalls() does not stop when it fails.  The transports then
> initialize against a core that has already been torn down: rds_wq has
> been destroyed, yet rds_ib_dev_put(), rds_ib_unregister_client() and
> rds_tcp_accept_work() still queue onto or flush it, and
> rds_trans_register() hands the transports to a core whose socket
> family may never have been registered.
> 
> This was seen with CONFIG_SYSCTL=n, where rds_sysctl_init() failed and
> rds_ib_init()'s error path then flushed the freed rds_wq [1], but any
> rds_init() failure after rds_threads_init() has the same effect.
> 
> Record whether rds_init() succeeded and have rds_rdma_init() and
> rds_tcp_init() bail out with -ENODEV otherwise.  Modular builds are
> unaffected: the transports need symbols exported by rds.ko, so they
> cannot be loaded unless rds_init() succeeded.
> 
> [1] https://lore.kernel.org/oe-lkp/202610190310.9737b2e3-lkp@intel.com
> Fixes: 55b7ed0b582f ("RDS: Common RDMA transport code")
> Fixes: 70041088e3b9 ("RDS: Add TCP transport to RDS")
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Assisted-by: LLM
> Signed-off-by: Allison Henderson <achender@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021195837.142802-1-achender@kernel.org?part=2