Re: [PATCH v5 5/5] 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 3 potential issue(s) to consider:
- [High] IB/isert: stale removal marker in isert_device_get() before isert_add_one()
- [Medium] IB/isert: unbounded leak of isert_device markers in device_list
- [Medium] IB/isert: memory leak of removal markers in isert_init() on error
--

commit 812d1184eb1546b67f3290f713769f5e49331bb1
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

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

This patch retains or allocates a removing struct isert_device marker on
device_list during isert_remove_one() so that concurrent isert_device_get()
calls reject devices being removed with -ENODEV. The markers are then freed
when a matching ib_device pointer is seen in isert_add_one() or when the
module unloads in isert_exit().

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 712d6165150e3..4bcd14f5045ac 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -260,10 +260,12 @@ isert_device_put(struct isert_device *device)
>  	isert_info("device %p refcount %d\n", device, device->refcount);
>  	if (!device->refcount) {
>  		isert_free_device_ib_res(device);
> -		list_del(&device->dev_node);
> -		if (device->removal_comp)
> +		if (device->removing) {
>  			complete(device->removal_comp);
> -		kfree(device);
> +		} else {
> +			list_del(&device->dev_node);
> +			kfree(device);
> +		}
>  	}
>  	mutex_unlock(&device_list_mutex);
>  }
> @@ -276,14 +278,21 @@ isert_device_get(struct rdma_cm_id *cma_id)
>  
>  	mutex_lock(&device_list_mutex);
>  	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);
> +		if (device->removing && device->ib_device == cma_id->device) {
>  			mutex_unlock(&device_list_mutex);
> -			return device;
> +			return ERR_PTR(-ENODEV);
>  		}
>  	}

[Severity: High]
Can a stale removal marker in device_list cause isert_device_get() to reject
valid connection requests on a newly enabled or namespace-moved RDMA device
before isert_add_one() has a chance to clear it?

Because ib_isert depends on rdma_cm, cma_client is registered before
isert_ib_client. In enable_device_and_get(), client add callbacks run in
registration order:

drivers/infiniband/core/device.c:enable_device_and_get()
  xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED)
    add_client_context(device, client)

When cma_add_one() runs first, it immediately activates wildcard listeners
(such as an isert_np bound to INADDR_ANY) via cma_listen_on_dev() before
returning:

drivers/infiniband/core/cma.c:cma_add_one()
  list_for_each_entry(id_priv, &listen_any_list, listen_any_item)
    cma_listen_on_dev(id_priv, cma_dev, &to_destroy)

When the same struct ib_device pointer is re-enabled by
rdma_dev_change_netns() (which calls disable_device() followed by
enable_device_and_get() on the same struct ib_device) or when a freed
struct ib_device address is reused by the slab allocator, the marker left by
isert_remove_one() is still on device_list in the window after cma_add_one()
starts listening and before isert_add_one() runs. Any incoming
RDMA_CM_EVENT_CONNECT_REQUEST during that window will match
device->removing && device->ib_device == cma_id->device in the first pass of
isert_device_get() and fail with -ENODEV.

Additionally, if a connection request arrives on a new ib_device after
cma_add_one() starts listening and add_client_context() subsequently fails
before setting CLIENT_DATA_REGISTERED for isert_ib_client, isert_device_get()
will have built a live isert_device, but the error unwind in
disable_device() will skip isert_remove_one(), leaving its IB resources
unreleased.

> +	list_for_each_entry(device, &device_list, dev_node) {
> +		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);
>  	if (!device) {
> @@ -2792,20 +2801,29 @@ isert_remove_one(struct ib_device *ib_device, void *client_data)
>  
>  	mutex_lock(&device_list_mutex);
>  	list_for_each_entry(device, &device_list, dev_node) {
> -		if (device->ib_device != ib_device)
> +		if (device->ib_device != ib_device || device->removing)
>  			continue;
>  		device->refcount++;
>  		device->removal_comp = &removal_comp;
>  		found = device;
>  		break;
>  	}
> -	mutex_unlock(&device_list_mutex);
> -
> -	if (!found)
> +	if (!found) {
> +		device = kzalloc_obj(struct isert_device,
> +				     GFP_KERNEL | __GFP_NOFAIL);
> +		INIT_LIST_HEAD(&device->dev_node);
> +		device->ib_device = ib_device;
> +		device->removing = true;
> +		list_add_tail(&device->dev_node, &device_list);
> +		mutex_unlock(&device_list_mutex);
>  		return;
> +	}
> +	mutex_unlock(&device_list_mutex);
>  
>  	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;
> @@ -2828,8 +2846,38 @@ isert_remove_one(struct ib_device *ib_device, void *client_data)
>  	wait_for_completion(&removal_comp);
>  }
>  
> +static int isert_add_one(struct ib_device *ib_device)
> +{
> +	struct isert_device *device, *tmp;
> +
> +	mutex_lock(&device_list_mutex);
> +	list_for_each_entry_safe(device, tmp, &device_list, dev_node) {
> +		if (device->ib_device != ib_device || !device->removing ||
> +		    device->refcount)
> +			continue;
> +		list_del(&device->dev_node);
> +		kfree(device);
> +	}
> +	mutex_unlock(&device_list_mutex);
> +
> +	return 0;
> +}

[Severity: Medium]
Does this accumulate struct isert_device markers on device_list without
bound across RDMA device unregistrations?

Whenever any IB device is unregistered via __ib_unregister_device() ->
disable_device() -> remove_client_context() -> isert_remove_one() (even a
device never used by ib_isert), isert_remove_one() either keeps the existing
struct isert_device on device_list via isert_device_put() (with dangling
pointers in device->ib_device, device->pd, and the stack-allocated
device->removal_comp) or allocates a new struct isert_device marker with
GFP_KERNEL | __GFP_NOFAIL and appends it to device_list.

Because isert_add_one() only frees a marker if a newly registered device
happens to reuse the exact same struct ib_device pointer address
(device->ib_device == ib_device), any removed ib_device whose slab memory is
not reused by a later ib_device allocation (for example, during repeated
rxe/siw link deletion or VF hotplug cycles) leaves its struct isert_device
marker on device_list until module unload.

> +
> +static void isert_free_markers(void)
> +{
> +	struct isert_device *device, *tmp;
> +
> +	mutex_lock(&device_list_mutex);
> +	list_for_each_entry_safe(device, tmp, &device_list, dev_node) {
> +		list_del(&device->dev_node);
> +		kfree(device);
> +	}
> +	mutex_unlock(&device_list_mutex);
> +}
> +
>  static struct ib_client isert_ib_client = {
>  	.name   = "isert",
> +	.add    = isert_add_one,
>  	.remove = isert_remove_one,
>  };
>  
> @@ -2881,6 +2929,7 @@ static int __init isert_init(void)
>  static void __exit isert_exit(void)
>  {
>  	ib_unregister_client(&isert_ib_client);
> +	isert_free_markers();

[Severity: Medium]
Does isert_init() leak the struct isert_device markers if
ib_register_client(&isert_ib_client) fails partway through?

In ib_register_client(), if add_client_context() succeeds for one or more
devices and then fails on a later device, it rolls back by calling
ib_unregister_client():

drivers/infiniband/core/device.c:ib_register_client()
  xa_for_each_marked (&devices, index, device, DEVICE_REGISTERED) {
    ret = add_client_context(device, client);
    if (ret)
      goto out;
  }
  ...
out:
  ...
  if (need_unreg && ret)
    ib_unregister_client(client);

That rollback invokes remove_client_context() -> isert_remove_one() for each
device where add_client_context() succeeded, which allocates a struct
isert_device marker and adds it to device_list. In isert_init(), the error
path jumps to destroy_release_wq without calling isert_free_markers():

drivers/infiniband/ulp/isert/ib_isert.c:isert_init()
	ret = ib_register_client(&isert_ib_client);
	if (ret) {
		isert_err("Unable to register isert ib client\n");
		goto destroy_release_wq;
	}

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