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

[Allison Henderson <achender@kernel.org>]

On Thu, 2026-08-27 at 20:55 -0700, Allison Henderson wrote:
> 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
> 
Hi Aohan,

I know this thread is a bit old, but this bug popped up as a pre-existing bug for another set review.  Did you have an
interest in sending a v3?  I am fine to carry it forward if you're on to other things, but I wanted to check with you
first.  I'd preserve all your tags, sob, and reported-by.  Just let me know, I'm fine either way.

Thanks!
Allison