Re: [PATCH v6 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:
- [Medium] IB/isert: race window in isert_remove_one() before setting found->removing
--

commit 5f9189f7b427c87e967cf5a96e3a777eb552f8e4
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

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

This commit prevents isert_device_get() from creating or returning an
isert_device for an ib_device that is being removed by tracking device state
via ib_client data and skipping devices marked as removing.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 712d6165150e3..2057bb35af4ca 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;
>  	}
>  
>  	device = kzalloc_obj(struct isert_device);
> @@ -2791,6 +2801,7 @@ isert_remove_one(struct ib_device *ib_device, void *client_data)
>  	LIST_HEAD(removal_list);
>  
>  	mutex_lock(&device_list_mutex);
> +	ib_set_client_data(ib_device, &isert_ib_client, NULL);
>  	list_for_each_entry(device, &device_list, dev_node) {
>  		if (device->ib_device != ib_device)
>  			continue;
> @@ -2805,7 +2816,9 @@ isert_remove_one(struct ib_device *ib_device, void *client_data)
>  		return;
>  
>  	mutex_lock(&isert_conn_mutex);
> +	mutex_lock(&device_list_mutex);
>  	found->removing = true;
> +	mutex_unlock(&device_list_mutex);
>  	list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
>  		if (isert_conn->device != found)
>  			continue;

[Severity: Medium]
In isert_remove_one(), device_list_mutex is unlocked after clearing the
client data and finding the device, before acquiring isert_conn_mutex and
re-locking device_list_mutex to set found->removing = true:

	mutex_lock(&device_list_mutex);
	ib_set_client_data(ib_device, &isert_ib_client, NULL);
	list_for_each_entry(device, &device_list, dev_node) {
		if (device->ib_device != ib_device)
			continue;
		device->refcount++;
		device->removal_comp = &removal_comp;
		found = device;
		break;
	}
	mutex_unlock(&device_list_mutex);

	if (!found)
		return;

	mutex_lock(&isert_conn_mutex);
	mutex_lock(&device_list_mutex);
	found->removing = true;
	mutex_unlock(&device_list_mutex);

The commit message notes that the lookup skips an isert_device being removed
so a device with the same node_guid cannot raise its reference count again.
However, because ib_get_client_data(cma_id->device, &isert_ib_client) already
rejects the same ib_device once client data is cleared, the device->removing
check in isert_device_get() is only reached when cma_id->device is a
different ib_device sharing the same node_guid.

If a connection request on another ib_device with the same node_guid calls
isert_device_get() during that unlocked window (for instance while
isert_remove_one() waits on isert_conn_mutex), won't it observe
device->removing == false and increment found->refcount on the removing
isert_device?

When that happens in isert_connect_request(), isert_alloc_login_buf() DMA-maps
buffers using cma_id->device, isert_create_qp() allocates a CQ on
found->ib_device and fails in rdma_create_qp() with -EINVAL because
id->device != pd->device, and the error path in isert_free_login_buf() then
unmaps those DMA buffers using isert_conn->device->ib_device (found->ib_device)
instead of cma_id->device:

	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;
	}
	...
out_rsp_dma_map:
	isert_free_login_buf(isert_conn);

Additionally, if an in-flight isert_connect_request() on the removing device
already called isert_device_get() before isert_remove_one() started, and it
acquires isert_conn_mutex before isert_remove_one() does, it will also see
device->removing == false and accept the connection.

Should found->removing = true be set inside the first device_list_mutex
critical section in isert_remove_one() when found is located?

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