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

[Jakub Kicinski <kuba@kernel.org>]

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?