Re: [PATCH] scsi: iscsi: publish endpoints after transport setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The `iscsi_iser` transport driver was not updated to call `iscsi_register_endpoint()`, completely breaking connection binding.

Pre-existing issues:
- [High] If `device_register()` fails inside `iscsi_create_endpoint()`, a double `idr_remove()` occurs with a dropped lock in between, incorrectly deleting a concurrent allocation.
--

commit d4c9226c25ea1f5ce70c62be37b16435ed05458c
Author: Ruoyu Wang <ruoyuw560@gmail.com>

scsi: iscsi: publish endpoints after transport setup

iscsi_create_endpoint() inserts a new endpoint into iscsi_ep_idr before
transport drivers have initialized their endpoint-private data. The
endpoint handle is returned only after ep_connect() completes, but handles
are allocated from a predictable IDR and iscsi_lookup_endpoint() looks
them up directly.

Reserve the endpoint ID with a NULL IDR entry, add
iscsi_register_endpoint() for the publish step, and call it from the
in-tree transport drivers after private endpoint setup has completed.
Until registration, endpoint lookup keeps returning NULL for the reserved
handle.

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 8aa76f813bcdb..a8c2a32c0f134 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -217,7 +217,7 @@ iscsi_create_endpoint(int dd_size)
>  	 * First endpoint id should be 1 to comply with user space
>  	 * applications (iscsid).
>  	 */
> -	id = idr_alloc(&iscsi_ep_idr, ep, 1, -1, GFP_NOIO);
> +	id = idr_alloc(&iscsi_ep_idr, NULL, 1, -1, GFP_NOIO);
>  	if (id < 0) {

[Severity: High]
This is a pre-existing issue, but does the put_dev error path in
iscsi_create_endpoint() result in a double idr_remove() for the same ID?

If device_register() fails, the code jumps to put_dev:

	put_dev:
		mutex_lock(&iscsi_ep_idr_mutex);
		idr_remove(&iscsi_ep_idr, id);
		mutex_unlock(&iscsi_ep_idr_mutex);
		put_device(&ep->dev);
		return NULL;

Calling put_device() drops the kobject refcount to zero, which triggers the
release callback:

drivers/scsi/scsi_transport_iscsi.c:iscsi_endpoint_release() {
	...
	mutex_lock(&iscsi_ep_idr_mutex);
	idr_remove(&iscsi_ep_idr, ep->id);
	mutex_unlock(&iscsi_ep_idr_mutex);
	...
}

Since the lock is dropped before put_device(), could a concurrent thread
successfully allocate the same ID via iscsi_create_endpoint(), only for the
second idr_remove() to incorrectly delete the newly allocated ID belonging
to the concurrent thread?

[ ... ]
> @@ -257,6 +257,28 @@ iscsi_create_endpoint(int dd_size)
>  }
>  EXPORT_SYMBOL_GPL(iscsi_create_endpoint);
>  
> +int iscsi_register_endpoint(struct iscsi_endpoint *ep)
> +{
> +	void *old;
> +	int err = 0;

[Severity: High]
Does the iscsi_iser transport driver need to be updated to call
iscsi_register_endpoint()?

The iscsi_iser driver in drivers/infiniband/ulp/iser/iscsi_iser.c allocates
the endpoint:

drivers/infiniband/ulp/iser/iscsi_iser.c:iscsi_iser_ep_connect() {
	...
	ep = iscsi_create_endpoint(0);
	if (!ep)
		return ERR_PTR(-ENOMEM);
	...
	return ep;
}

But it doesn't seem to publish it to the IDR. When userspace subsequently
calls iscsi_iser_conn_bind() to bind the connection, the lookup uses
idr_find(), which correctly returns NULL:

drivers/infiniband/ulp/iser/iscsi_iser.c:iscsi_iser_conn_bind() {
	...
	ep = iscsi_lookup_endpoint(transport_eph);
	if (!ep) {
		iser_err("can't bind eph %llx\n",
			 (unsigned long long)transport_eph);
		return -EINVAL;
	}
	...
}

Would this cause the binding lookup to always fail with -EINVAL, preventing
the iSER transport from working?

> +
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	old = idr_find(&iscsi_ep_idr, ep->id);
> +	if (old) {
> +		err = -EBUSY;
> +		goto unlock;
> +	}
> +
> +	old = idr_replace(&iscsi_ep_idr, ep, ep->id);
> +	if (IS_ERR(old))
> +		err = PTR_ERR(old);
> +unlock:
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(iscsi_register_endpoint);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617182135.957230-1-ruoyuw560@gmail.com?part=1