Re: [PATCH] scsi: iscsi: Remove the endpoint ID only once when device_register() fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] scsi: iscsi: failed ep left in iscsi_ep_idr in
  iscsi_create_endpoint()
--

commit bcf1b2eaf3554e78f33bb71b87b333ff7a9501a1
Author: Changyul Lee <lcy8047@gmail.com>

scsi: iscsi: Remove the endpoint ID only once when device_register() fails

This commit drops the explicit idr_remove() from the device_register()
error path in iscsi_create_endpoint() so that iscsi_endpoint_release()
removes the endpoint ID once.

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index b094af7c997e2..6e38fbfd5a9de 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -246,9 +246,6 @@ iscsi_create_endpoint(int dd_size)
>  	return NULL;
>  
>  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;
>  free_ep:

[Severity: Medium]
Does leaving ep in iscsi_ep_idr here allow iscsi_lookup_endpoint() to
find the failed endpoint after put_device() drops its reference count to
zero?

iscsi_create_endpoint() publishes ep in iscsi_ep_idr before calling
device_register(&ep->dev) and before setting ep->dd_data:

CPU0                                  CPU1
iscsi_create_endpoint()
  idr_alloc(&iscsi_ep_idr, ep, ...)
  device_register(&ep->dev) -> fails
  put_device(&ep->dev)
    kref_put(&kobj->kref, ...)
      /* kref reaches 0 */
                                      iscsi_lookup_endpoint()
                                        ep = idr_find(&iscsi_ep_idr, ...)
                                        get_device(&ep->dev)  <-- use-after-free
      iscsi_endpoint_release()
        mutex_lock(&iscsi_ep_idr_mutex)
        idr_remove(&iscsi_ep_idr, ep->id)
        kfree(ep)

In addition to the refcount addition on zero and use-after-free on ep,
ep->dd_data is still NULL because the error path skipped its
initialization, so callers of iscsi_lookup_endpoint() such as
iscsi_if_ep_disconnect() -> transport->ep_disconnect() will dereference a
NULL pointer.

Replacing the pointer with NULL via idr_replace(&iscsi_ep_idr, NULL, id)
under iscsi_ep_idr_mutex before put_device(&ep->dev) would hide the
failed endpoint from lookups while keeping the ID reserved until
iscsi_endpoint_release() removes it.

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