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 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