[PATCH net v4 0/2] net/rds: RDMA-CM event handler fixes for non-IB devices

[PATCH net v4 0/2] net/rds: RDMA-CM event handler fixes for non-IB devices

[Allison Henderson]

Hi all,

This is v4 of Aohan Mei's fix for the uninitialized transport pointer
in the RDMA-CM event handler (v1 at [1], v2 at [2], v3 at [3]), now a
two-patch set.

  Patch 1 is the fix itself.  v3 only rejected a connect request
  arriving on a non-IB device; the active side can bind an id to one
  as well, and resolving a route on an iWARP id leaves
  cm_id->route.path_rec unset, which the ROUTE_RESOLVED case
  dereferences.  Such a connection is now dropped at ADDR_RESOLVED
  instead of resolving a route.

  Patch 2 fixes a neighbouring problem in the same case: a synchronous
  rdma_resolve_route() failure was handed back to the rdma_cm, which
  then destroyed an id RDS still owns as ic->i_cm_id and would later
  disconnect and destroy again from the connection's shutdown.

On net-next the rdma_cm ids RDS creates are restricted to IB devices
(commit c7fca8aae6fe), which makes the non-IB cases impossible there;
these are the fixes stable kernels without that API need.

Changes since v3 [3]:
 - Patch 1 also drops a connection whose address resolved to a non-IB
   device, before a route is resolved on it (review of v3).
 - New patch 2 for the rdma_resolve_route() failure return.
 - Rebased onto current net.

Changes since v2 [2]:
 - Carried forward; the rejection is limited to
   RDMA_CM_EVENT_CONNECT_REQUEST so that rdma_cm does not destroy
   connection ids RDS still owns.

[1] https://lore.kernel.org/netdev/20260824111701.2979194-1-ljp1205831794@gmail.com/
[2] https://lore.kernel.org/netdev/20260825021223.3483044-1-ljp1205831794@gmail.com/
[3] https://lore.kernel.org/netdev/20260928044507.335883-1-achender@kernel.org/

Thank you,
Allison


Allison Henderson (1):
  net/rds: don't let the rdma_cm destroy an id RDS still owns on route
    failure

Aohan Mei (1):
  net: rds: fix uninitialized trans dereference in CM event handler

 net/rds/rdma_transport.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH net v4 1/2] net: rds: fix uninitialized trans dereference in CM event handler

[Allison Henderson]

From: Aohan Mei <henrymei@tencent.com>

rds_rdma_cm_event_handler_cmn() assigns trans only when the RDMA
device is an InfiniBand CA (RDMA_NODE_IB_CA).  On any other device
type, e.g. an iWARP RNIC such as siw, trans stays uninitialized, but
the event switch dereferences it: unconditionally in the
RDMA_CM_EVENT_CONNECT_REQUEST case via trans->cm_handle_connect(),
and (with a connection context) in the ROUTE_RESOLVED and
ESTABLISHED cases.

An RDS listener on an iWARP device therefore crashes the kernel as
soon as a connect request arrives: with CONFIG_INIT_STACK_ALL_ZERO
the wild load becomes a NULL dereference at offset 0xa0
(&trans->cm_handle_connect) in the iw_cm_wq workqueue.

GCC masks the bug in default builds by folding the uninitialized
load into &rds_ib_transport; Clang-built kernels take the real
uninitialized path and oops.

The active side can land on a non-IB device too: the id that
rds_ib_conn_path_connect() creates is not restricted to a node type,
so rdma_resolve_addr() binds it to whichever device serves the local
address, and an iWARP RNIC on the same netdev qualifies.  The crash
there is different: resolving a route on an iWARP id never fills in
cm_id->route.path_rec, and the ROUTE_RESOLVED case writes
path_rec[0].sl before it calls into the transport.

The iWARP transport was dropped long ago and IB is the only
transport left, so make that explicit: initialize trans to
&rds_ib_transport at declaration, drop the conditional assignment,
reject a connect request that arrives on a non-IB device, and drop a
connection whose address resolved to one instead of resolving a
route on it.  The rejection is limited to RDMA_CM_EVENT_CONNECT_REQUEST
on purpose: a non-zero return from the handler makes rdma_cm destroy
the id the event was delivered on, which is right for the request's
freshly created id but would free a connection id that RDS still owns
for any other event.  On the active side the id stays ic->i_cm_id and
the connection's shutdown destroys it, so that case returns 0.

Fixes: dcdede0406d3 ("RDS: Drop stale iWARP RDMA transport")
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Link: https://lore.kernel.org/netdev/20260824111701.2979194-1-ljp1205831794@gmail.com/
Link: https://lore.kernel.org/netdev/20260825021223.3483044-1-ljp1205831794@gmail.com/
Cc: stable@vger.kernel.org
Assisted-by: CodeBuddy:Kimi-K3
Signed-off-by: Aohan Mei <henrymei@tencent.com>
[achender: reject only on RDMA_CM_EVENT_CONNECT_REQUEST instead of
 bailing out for every event on a non-IB device, so that rdma_cm does
 not destroy connection ids RDS still tracks; drop a connection whose
 address resolved to a non-IB device before a route is resolved on it;
 changelog adjusted]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/rdma_transport.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index b15cf316b23a..09cc2ba23570 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -52,7 +52,7 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
 {
 	/* this can be null in the listening path */
 	struct rds_connection *conn = cm_id->context;
-	struct rds_transport *trans;
+	struct rds_transport *trans = &rds_ib_transport;
 	int ret = 0;
 	int *err;
 	u8 len;
@@ -60,9 +60,6 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
 	rdsdebug("conn %p id %p handling event %u (%s)\n", conn, cm_id,
 		 event->event, rdma_event_msg(event->event));
 
-	if (cm_id->device->node_type == RDMA_NODE_IB_CA)
-		trans = &rds_ib_transport;
-
 	/* Prevent shutdown from tearing down the connection
 	 * while we're executing. */
 	if (conn) {
@@ -82,11 +79,32 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
 
 	switch (event->event) {
 	case RDMA_CM_EVENT_CONNECT_REQUEST:
+		/* Only the IB transport is supported, but RDS listens on
+		 * every RDMA device: reject a request that arrived on any
+		 * other kind.  A non-zero return has rdma_cm destroy the
+		 * request's id, which is what we want here and only here.
+		 */
+		if (cm_id->device->node_type != RDMA_NODE_IB_CA) {
+			ret = 1;
+			break;
+		}
 		ret = trans->cm_handle_connect(cm_id, event, isv6);
 		break;
 
 	case RDMA_CM_EVENT_ADDR_RESOLVED:
 		if (conn) {
+			/* The address resolved to a device RDS has no
+			 * transport for.  Do not go on to resolve a route:
+			 * an iWARP route leaves cm_id->route.path_rec
+			 * unset, which the ROUTE_RESOLVED case below
+			 * dereferences.  Drop the connection instead; the
+			 * id is still ic->i_cm_id, so return 0 and let the
+			 * shutdown destroy it.
+			 */
+			if (cm_id->device->node_type != RDMA_NODE_IB_CA) {
+				rds_conn_drop(conn);
+				break;
+			}
 			rdma_set_service_type(cm_id, conn->c_tos);
 			rdma_set_min_rnr_timer(cm_id, IB_RNR_TIMER_000_32);
 			/* XXX do we need to clean up if this fails? */
-- 
2.25.1

Re: [PATCH net v4 1/2] net: rds: fix uninitialized trans dereference in CM event handler

[sashiko-bot]

> From: Aohan Mei <henrymei@tencent.com>
> 
> rds_rdma_cm_event_handler_cmn() assigns trans only when the RDMA
> device is an InfiniBand CA (RDMA_NODE_IB_CA).  On any other device
> type, e.g. an iWARP RNIC such as siw, trans stays uninitialized, but
> the event switch dereferences it: unconditionally in the
> RDMA_CM_EVENT_CONNECT_REQUEST case via trans->cm_handle_connect(),
> and (with a connection context) in the ROUTE_RESOLVED and
> ESTABLISHED cases.
> 
> An RDS listener on an iWARP device therefore crashes the kernel as
> soon as a connect request arrives: with CONFIG_INIT_STACK_ALL_ZERO
> the wild load becomes a NULL dereference at offset 0xa0
> (&trans->cm_handle_connect) in the iw_cm_wq workqueue.
> 
> [ ... ]
>  not destroy connection ids RDS still tracks; drop a connection whose
>  address resolved to a non-IB device before a route is resolved on it;
>  changelog adjusted]
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Allison Henderson <achender@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH net v4 2/2] net/rds: don't let the rdma_cm destroy an id RDS still owns on route failure

[Allison Henderson]

rds_rdma_cm_event_handler_cmn() hands the return value of
rdma_resolve_route() straight back to the rdma_cm on
RDMA_CM_EVENT_ADDR_RESOLVED.  A synchronous failure there - -ENOMEM
from the route work allocation, an SA query that cannot be set up, a
RoCE route with no usable device - makes addr_handler() destroy the
id the event was delivered on.  That id is ic->i_cm_id, and nothing
clears the pointer: the connection sits in RDS_CONN_CONNECTING with a
freed id until its shutdown calls rdma_disconnect() and
rdma_destroy_id() on it.

The ROUTE_RESOLVED case already avoids this: rds_ib_cm_initiate_connect()
forces a zero return while ic->i_cm_id == cm_id, as the comment there
explains.  Do the same here - drop the connection and return 0 - so
the id stays RDS's to destroy from the shutdown, and the reconnect
gets a fresh one.

Fixes: 55b7ed0b582f ("RDS: Common RDMA transport code")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/rdma_transport.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index 09cc2ba23570..7be38be1228e 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -107,9 +107,19 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
 			}
 			rdma_set_service_type(cm_id, conn->c_tos);
 			rdma_set_min_rnr_timer(cm_id, IB_RNR_TIMER_000_32);
-			/* XXX do we need to clean up if this fails? */
 			ret = rdma_resolve_route(cm_id,
 						 RDS_RDMA_RESOLVE_TIMEOUT_MS);
+			if (ret) {
+				/* A non-zero return has the rdma_cm destroy
+				 * the id, but it is still ic->i_cm_id, which
+				 * the connection's shutdown would then
+				 * disconnect and destroy again.  Drop the
+				 * connection and keep the id for that
+				 * shutdown.
+				 */
+				rds_conn_drop(conn);
+				ret = 0;
+			}
 		}
 		break;
 
-- 
2.25.1

Re: [PATCH net v4 2/2] net/rds: don't let the rdma_cm destroy an id RDS still owns on route failure

[sashiko-bot]

> rds_rdma_cm_event_handler_cmn() hands the return value of
> rdma_resolve_route() straight back to the rdma_cm on
> RDMA_CM_EVENT_ADDR_RESOLVED.  A synchronous failure there - -ENOMEM
> from the route work allocation, an SA query that cannot be set up, a
> RoCE route with no usable device - makes addr_handler() destroy the
> id the event was delivered on.  That id is ic->i_cm_id, and nothing
> clears the pointer: the connection sits in RDS_CONN_CONNECTING with a
> freed id until its shutdown calls rdma_disconnect() and
> rdma_destroy_id() on it.
> 
> The ROUTE_RESOLVED case already avoids this: rds_ib_cm_initiate_connect()
> forces a zero return while ic->i_cm_id == cm_id, as the comment there
> explains.  Do the same here - drop the connection and return 0 - so
> the id stays RDS's to destroy from the shutdown, and the reconnect
> gets a fresh one.
> 
> Fixes: 55b7ed0b582f ("RDS: Common RDMA transport code")
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Allison Henderson <achender@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

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