[PATCH net v2] net: rds: fix uninitialized trans dereference in CM event handler

[PATCH net v2] net: rds: fix uninitialized trans dereference in CM event handler

[Aohan Mei]

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 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,
and reject events from non-IB devices before the event switch.

Fixes: dcdede0406d3 ("RDS: Drop stale iWARP RDMA transport")
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Cc: stable@vger.kernel.org
Assisted-by: CodeBuddy:Kimi-K3
Signed-off-by: Aohan Mei <henrymei@tencent.com>
---
v2:
- Enforce the IB transport directly instead of NULL-guarding trans,
  as suggested by Allison Henderson.
v1: https://lore.kernel.org/netdev/20260824111701.2979194-1-ljp1205831794@gmail.com
 net/rds/rdma_transport.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index b15cf316b23a..2c5481f1fcb4 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) {
@@ -80,6 +77,12 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
 		}
 	}
 
+	/* Only the IB transport is supported. */
+	if (cm_id->device->node_type != RDMA_NODE_IB_CA) {
+		ret = 1;
+		goto out;
+	}
+
 	switch (event->event) {
 	case RDMA_CM_EVENT_CONNECT_REQUEST:
 		ret = trans->cm_handle_connect(cm_id, event, isv6);
-- 
2.43.7

Re: [PATCH net v2] net: rds: fix uninitialized trans dereference in CM event handler

[Allison Henderson]

On Tue, 2026-08-25 at 10:12 +0800, Aohan Mei wrote:
> 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 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,
> and reject events from non-IB devices before the event switch.
> 
> Fixes: dcdede0406d3 ("RDS: Drop stale iWARP RDMA transport")
> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
> Cc: stable@vger.kernel.org
> Assisted-by: CodeBuddy:Kimi-K3
> Signed-off-by: Aohan Mei <henrymei@tencent.com>

This looks good to me.  Thanks for the quick response.

Reviewed-by: Allison Henderson <achender@kernel.org>

> ---
> v2:
> - Enforce the IB transport directly instead of NULL-guarding trans,
>   as suggested by Allison Henderson.
> v1: https://lore.kernel.org/netdev/20260824111701.2979194-1-ljp1205831794@gmail.com
>  net/rds/rdma_transport.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index b15cf316b23a..2c5481f1fcb4 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) {
> @@ -80,6 +77,12 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
>  		}
>  	}
>  
> +	/* Only the IB transport is supported. */
> +	if (cm_id->device->node_type != RDMA_NODE_IB_CA) {
> +		ret = 1;
> +		goto out;
> +	}
> +
>  	switch (event->event) {
>  	case RDMA_CM_EVENT_CONNECT_REQUEST:
>  		ret = trans->cm_handle_connect(cm_id, event, isv6);

Re: [PATCH net v2] net: rds: fix uninitialized trans dereference in CM event handler

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: rds: fix uninitialized trans dereference in CM event handler

This initializes trans to &rds_ib_transport at declaration in
rds_rdma_cm_event_handler_cmn(), drops the conditional assignment based on
cm_id->device->node_type, and adds an early bail-out that returns 1 for any
event arriving on a device that is not RDMA_NODE_IB_CA.

> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index b15cf316b23a2..2c5481f1fcb4f 100644
> --- a/net/rds/rdma_transport.c
> +++ b/net/rds/rdma_transport.c

[ ... ]

> @@ -80,6 +77,12 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
>  		}
>  	}
>  
> +	/* Only the IB transport is supported. */
> +	if (cm_id->device->node_type != RDMA_NODE_IB_CA) {
> +		ret = 1;
> +		goto out;
> +	}
> +

[Severity: High]
Should this non-zero return be limited to RDMA_CM_EVENT_CONNECT_REQUEST?
As written it applies to every event type.

include/rdma/rdma_cm.h documents what a non-zero return means:

/**
 * rdma_cm_event_handler - Callback used to report user events.
 *
 * Notes: Users may not call rdma_destroy_id from this callback to destroy
 *   the passed in id, or a corresponding listen id.  Returning a
 *   non-zero value from the callback will destroy the passed in id.
 */

The cma dispatchers implement exactly that, for example addr_handler() and
cma_work_handler() in drivers/infiniband/core/cma.c:

	if (cma_cm_event_handler(id_priv, &event)) {
		destroy_id_handler_unlock(id_priv);
		return;
	}

But RDS keeps ownership of connection cm_ids.  rds_ib_conn_path_connect()
stores it:

	ic->i_cm_id = rdma_create_id(&init_net, handler, conn,
				     RDMA_PS_TCP, IB_QPT_RC);

and rds_ib_conn_path_shutdown() later dereferences and destroys the same
pointer, clearing it only afterwards:

	if (ic->i_cm_id) {
		rdsdebug("disconnecting cm %p\n", ic->i_cm_id);
		err = rdma_disconnect(ic->i_cm_id);
	...
		rdma_destroy_id(ic->i_cm_id);
	...
		ic->i_cm_id = NULL;

So if any event other than CONNECT_REQUEST is delivered with conn != NULL on
a cm_id whose device node_type is not RDMA_NODE_IB_CA, does the core free the
cm_id while ic->i_cm_id still points at it?  Can the following conn drop,
socket close or module unload then hit freed memory in
rds_ib_conn_path_shutdown(), including a second rdma_destroy_id() on the same
id?

A reachable path without any race: rds_ib_laddr_check() short-circuits via
rds_ib_get_device(), which only walks the ipaddr_list of RDS's own IB
devices:

net/rds/ib_rdma.c:rds_ib_get_device() {
	list_for_each_entry_rcu(rds_ibdev, &rds_ib_devices, list) {
		list_for_each_entry_rcu(i_ipaddr, &rds_ibdev->ipaddr_list, list) {
			if (i_ipaddr->ipaddr == ipaddr) {
	...
}

while the device actually bound for the outgoing connection is picked later
by cma_acquire_dev_by_src_ip(), which walks the global cma dev_list and can
match a siw device on the same address.  siw_device_create() sets:

	base_dev->node_type = RDMA_NODE_RNIC;

In that configuration, rdma_resolve_addr() from rds_ib_conn_path_connect()
binds a RDMA_NODE_RNIC device, and the resulting RDMA_CM_EVENT_ADDR_RESOLVED
with conn != NULL now returns 1, so addr_handler() destroys the id while RDS
still tracks it.

The same function a few lines above already documents the opposite
convention:

		/* If the connection is being shut down, bail out
		 * right away. We return 0 so cm_id doesn't get
		 * destroyed prematurely */
		if (rds_conn_state(conn) == RDS_CONN_DISCONNECTING) {
			/* Reject incoming connections while we're tearing
			 * down an existing one. */
			if (event->event == RDMA_CM_EVENT_CONNECT_REQUEST)
				ret = 1;
			goto out;
		}

Separately, does the unconditional goto out also skip the rds_conn_drop()
that the ADDR_ERROR, ROUTE_ERROR, CONNECT_ERROR, UNREACHABLE,
DEVICE_REMOVAL, ADDR_CHANGE, DISCONNECTED and TIMEWAIT_EXIT cases perform,
leaving the RDS connection stuck in CONNECTING or UP with no error
propagated?

Would something along these lines match the existing convention, while still
dropping the conn for the remaining events:

	ret = (event->event == RDMA_CM_EVENT_CONNECT_REQUEST) ? 1 : 0;

or moving the node_type test into the RDMA_CM_EVENT_CONNECT_REQUEST case?

Re: [PATCH net v2] net: rds: fix uninitialized trans dereference in CM event handler

[Allison Henderson]

On Thu, 2026-08-27 at 09:18 -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: rds: fix uninitialized trans dereference in CM event handler
> 
> This initializes trans to &rds_ib_transport at declaration in
> rds_rdma_cm_event_handler_cmn(), drops the conditional assignment based on
> cm_id->device->node_type, and adds an early bail-out that returns 1 for any
> event arriving on a device that is not RDMA_NODE_IB_CA.
> 
> > diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> > index b15cf316b23a2..2c5481f1fcb4f 100644
> > --- a/net/rds/rdma_transport.c
> > +++ b/net/rds/rdma_transport.c
> 
> [ ... ]
> 
> > @@ -80,6 +77,12 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
> >  		}
> >  	}
> >  
> > +	/* Only the IB transport is supported. */
> > +	if (cm_id->device->node_type != RDMA_NODE_IB_CA) {
> > +		ret = 1;
> > +		goto out;
> > +	}
> > +
> 
> [Severity: High]
> Should this non-zero return be limited to RDMA_CM_EVENT_CONNECT_REQUEST?
> As written it applies to every event type.
> 
> include/rdma/rdma_cm.h documents what a non-zero return means:
> 
> /**
>  * rdma_cm_event_handler - Callback used to report user events.
>  *
>  * Notes: Users may not call rdma_destroy_id from this callback to destroy
>  *   the passed in id, or a corresponding listen id.  Returning a
>  *   non-zero value from the callback will destroy the passed in id.
>  */
> 
> The cma dispatchers implement exactly that, for example addr_handler() and
> cma_work_handler() in drivers/infiniband/core/cma.c:
> 
> 	if (cma_cm_event_handler(id_priv, &event)) {
> 		destroy_id_handler_unlock(id_priv);
> 		return;
> 	}
> 
> But RDS keeps ownership of connection cm_ids.  rds_ib_conn_path_connect()
> stores it:
> 
> 	ic->i_cm_id = rdma_create_id(&init_net, handler, conn,
> 				     RDMA_PS_TCP, IB_QPT_RC);
> 
> and rds_ib_conn_path_shutdown() later dereferences and destroys the same
> pointer, clearing it only afterwards:
> 
> 	if (ic->i_cm_id) {
> 		rdsdebug("disconnecting cm %p\n", ic->i_cm_id);
> 		err = rdma_disconnect(ic->i_cm_id);
> 	...
> 		rdma_destroy_id(ic->i_cm_id);
> 	...
> 		ic->i_cm_id = NULL;
> 
> So if any event other than CONNECT_REQUEST is delivered with conn != NULL on
> a cm_id whose device node_type is not RDMA_NODE_IB_CA, does the core free the
> cm_id while ic->i_cm_id still points at it?  Can the following conn drop,
> socket close or module unload then hit freed memory in
> rds_ib_conn_path_shutdown(), including a second rdma_destroy_id() on the same
> id?
> 
> A reachable path without any race: rds_ib_laddr_check() short-circuits via
> rds_ib_get_device(), which only walks the ipaddr_list of RDS's own IB
> devices:
> 
> net/rds/ib_rdma.c:rds_ib_get_device() {
> 	list_for_each_entry_rcu(rds_ibdev, &rds_ib_devices, list) {
> 		list_for_each_entry_rcu(i_ipaddr, &rds_ibdev->ipaddr_list, list) {
> 			if (i_ipaddr->ipaddr == ipaddr) {
> 	...
> }
> 
> while the device actually bound for the outgoing connection is picked later
> by cma_acquire_dev_by_src_ip(), which walks the global cma dev_list and can
> match a siw device on the same address.  siw_device_create() sets:
> 
> 	base_dev->node_type = RDMA_NODE_RNIC;
> 
> In that configuration, rdma_resolve_addr() from rds_ib_conn_path_connect()
> binds a RDMA_NODE_RNIC device, and the resulting RDMA_CM_EVENT_ADDR_RESOLVED
> with conn != NULL now returns 1, so addr_handler() destroys the id while RDS
> still tracks it.
> 
> The same function a few lines above already documents the opposite
> convention:
> 
> 		/* If the connection is being shut down, bail out
> 		 * right away. We return 0 so cm_id doesn't get
> 		 * destroyed prematurely */
> 		if (rds_conn_state(conn) == RDS_CONN_DISCONNECTING) {
> 			/* Reject incoming connections while we're tearing
> 			 * down an existing one. */
> 			if (event->event == RDMA_CM_EVENT_CONNECT_REQUEST)
> 				ret = 1;
> 			goto out;
> 		}
> 
> Separately, does the unconditional goto out also skip the rds_conn_drop()
> that the ADDR_ERROR, ROUTE_ERROR, CONNECT_ERROR, UNREACHABLE,
> DEVICE_REMOVAL, ADDR_CHANGE, DISCONNECTED and TIMEWAIT_EXIT cases perform,
> leaving the RDS connection stuck in CONNECTING or UP with no error
> propagated?
> 
> Would something along these lines match the existing convention, while still
> dropping the conn for the remaining events:
> 
> 	ret = (event->event == RDMA_CM_EVENT_CONNECT_REQUEST) ? 1 : 0;
> 
> or moving the node_type test into the RDMA_CM_EVENT_CONNECT_REQUEST case?

I think the cleaner solution here would be to hoist the check into the calling function.  So in
rds_rdma_cm_event_handler_cmn():

        case RDMA_CM_EVENT_CONNECT_REQUEST:
		/* Only the IB transport is supported. */
                if (cm_id->device->node_type == RDMA_NODE_IB_CA)
                        ret = trans->cm_handle_connect(cm_id, event, isv6);
                else
                	ret = 1;
                break;

Thanks for working on this!
Allison