Re: [PATCH v7 5/6] IB/isert: do not build a new isert_device for a device being removed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] IB/isert: Cross-device aliasing via node_guid lookup in isert_device_get()
--

commit 01d8f2361924f11ef1e502a2b19ce2e53509658d
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

IB/isert: do not build a new isert_device for a device being removed

This patch uses ib_client data to track whether an ib_device is active and
skips devices marked as removing in isert_device_get() so a concurrent
CONNECT_REQUEST cannot create or grab an isert_device during device removal.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 712d6165150e3..1b388075cc633 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c

[ ... ]

> @@ -275,14 +280,19 @@ isert_device_get(struct rdma_cm_id *cma_id)
>  	int ret;
>  
>  	mutex_lock(&device_list_mutex);
> +	if (!ib_get_client_data(cma_id->device, &isert_ib_client)) {
> +		mutex_unlock(&device_list_mutex);
> +		return ERR_PTR(-ENODEV);
> +	}
>  	list_for_each_entry(device, &device_list, dev_node) {
> -		if (device->ib_device->node_guid == cma_id->device->node_guid) {
> -			device->refcount++;
> -			isert_info("Found iser device %p refcount %d\n",
> -				   device, device->refcount);
> -			mutex_unlock(&device_list_mutex);
> -			return device;
> -		}
> +		if (device->removing ||
> +		    device->ib_device->node_guid != cma_id->device->node_guid)
> +			continue;
> +		device->refcount++;
> +		isert_info("Found iser device %p refcount %d\n",
> +			   device, device->refcount);
> +		mutex_unlock(&device_list_mutex);
> +		return device;
>  	}

[Severity: High]
As reported by Sashiko on v6 [1], when two ib_device instances share the same
node_guid (for example, software RDMA devices over netdevs sharing a MAC
address, which the commit message notes), can matching by node_guid in
isert_device_get() bind a connection on one ib_device to another ib_device's
isert_device?

Each isert_device is bound to a specific ib_device pointer
(device->ib_device = cma_id->device), allocates its protection domain on that
device in isert_create_device_ib_res(), and is looked up by pointer equality
in isert_remove_one():

drivers/infiniband/ulp/isert/ib_isert.c:isert_remove_one() {
	...
	list_for_each_entry(device, &device_list, dev_node) {
		if (device->ib_device != ib_device)
			continue;
		device->refcount++;
		device->removal_comp = &removal_comp;
		device->removing = true;
		found = device;
		break;
	}
}

In addition, while the commit message mentions that taking device_list_mutex
under isert_conn_mutex prevents a sibling device from raising the reference
count of one being removed, isert_device_get() only acquires
device_list_mutex and does not acquire isert_conn_mutex.

If ib_dev1 has an active isert_device in device_list and a CONNECT_REQUEST
arrives on ib_dev2 (which shares node_guid with ib_dev1), isert_device_get()
passes ib_get_client_data(cma_id->device, &isert_ib_client) for ib_dev2,
matches ib_dev1's isert_device by node_guid, increments its refcount, and
returns it.

In isert_connect_request(), isert_alloc_login_buf() then maps DMA buffers on
cma_id->device (ib_dev2), while isert_create_qp() allocates a CQ on
device->ib_device (ib_dev1) and calls rdma_create_qp(cma_id, device->pd,
&attr), which fails with -EINVAL because id->device != pd->device:

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_request() {
	...
	ret = isert_alloc_login_buf(isert_conn, cma_id->device);
	if (ret)
		goto out_conn_dev;

	isert_set_nego_params(isert_conn, &event->param.conn);

	isert_conn->qp = isert_create_qp(isert_conn, cma_id);
	if (IS_ERR(isert_conn->qp)) {
		ret = PTR_ERR(isert_conn->qp);
		goto out_rsp_dma_map;
	}
}

On the out_rsp_dma_map error path, isert_free_login_buf() unmaps
login_rsp_dma and login_desc->dma_addr using isert_conn->device->ib_device
(ib_dev1) instead of cma_id->device (ib_dev2), unmapping on the wrong device
and leaking the DMA mappings on ib_dev2:

drivers/infiniband/ulp/isert/ib_isert.c:isert_free_login_buf() {
	struct ib_device *ib_dev = isert_conn->device->ib_device;

	ib_dma_unmap_single(ib_dev, isert_conn->login_rsp_dma,
			    ISER_RX_PAYLOAD_SIZE, DMA_TO_DEVICE);
	...
}

Conversely, if ib_dev2 already has an active isert_device in device_list and
ib_dev1 has none when a CONNECT_REQUEST arrives on ib_dev1 just before
isert_remove_one(ib_dev1) runs, isert_device_get() matches ib_dev2's
isert_device. When isert_remove_one(ib_dev1) then searches device_list for
device->ib_device == ib_dev1, found is NULL and it returns immediately
without waiting while isert_connect_request() is still mapping DMA buffers on
ib_dev1.

Should isert_device_get() match by device->ib_device != cma_id->device
instead of node_guid so that it is consistent with isert_remove_one()?

[1] https://lore.kernel.org/r/20261103223802.1697607-6-yhlee@isslab.korea.ac.kr

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104012219.1759523-1-yhlee@isslab.korea.ac.kr?part=5