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

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

[Changyul Lee]

When device_register() fails, iscsi_create_endpoint() removes the
endpoint ID from iscsi_ep_idr, drops iscsi_ep_idr_mutex and then calls
put_device(). put_device() runs iscsi_endpoint_release(), which removes
the same ID again. If another endpoint gets that ID in between, its
entry is removed and iscsi_lookup_endpoint() no longer finds it. The
next endpoint then gets the same ID and fails to register:

[    6.689361] sysfs: cannot create duplicate filename '/devices/virtual/iscsi_endpoint/ep-1'

Leave the removal to iscsi_endpoint_release().

Fixes: f014165faa7b ("scsi: iscsi: Fix possible memory leak when device_register() failed")
Signed-off-by: Changyul Lee <lcy8047@gmail.com>
---
Found with a static analysis tool we are developing. Sashiko AI review
also pointed it out as a pre-existing issue.
https://lore.kernel.org/all/20260617204203.91A3B1F000E9@smtp.kernel.org/

Reproduced in QEMU (x86_64) with CONFIG_DEBUG_KOBJECT_RELEASE, which
delays iscsi_endpoint_release() by 1-4 seconds, and a test module. The
module makes device_register() fail with fail-nth (an allocation in
device_add()), creates a second endpoint and looks it up after the
delayed release. Without this patch, the second endpoint gets the same
ID 1 and is not found, and a third endpoint fails with the error above.
With it, the second endpoint gets ID 2 and is found, and the third gets
ID 1 after the release.

 drivers/scsi/scsi_transport_iscsi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index b094af7c997e..6e38fbfd5a9d 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:

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0

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

[sashiko-bot]

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