[PATCH] scsi: ch: Remove the minor from ch_index_idr after device_destroy()

[PATCH] scsi: ch: Remove the minor from ch_index_idr after device_destroy()

[Changyul Lee]

ch_remove() removes the minor from ch_index_idr before device_destroy()
removes the "sch<N>" device. A changer probed in between can get the
same minor, and its device_create() fails:

  sysfs: cannot create duplicate filename '/class/scsi_changer/sch0'
  ...
  ch 1:0:0:0: ch0: device_create failed

The new changer is then not attached.

Replace the IDR entry with NULL instead, so that ch_open() still fails
with -ENXIO for the changer being removed, and remove the minor after
device_destroy().

Fixes: da707c54c342 ("[SCSI] ch: fix device minor number management bug")
Signed-off-by: Changyul Lee <lcy8047@gmail.com>
---
Reproduced in QEMU (x86_64) with scsi_debug (ptype=8, add_host=2),
which gives two changers on two hosts. Two threads, one per host,
delete the changer through sysfs and rescan the host in a loop for 20
seconds. Without this patch, device_create() failed 992 to 1151 times
per run in three runs. With it, it never failed in two runs, and
/dev/sch0 can still be opened before and after a delete and rescan.

 drivers/scsi/ch.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index b2c9fb71c9e0..390425671a3a 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -952,11 +952,15 @@ static void ch_remove(struct scsi_device *sd)
 	scsi_changer *ch = dev_get_drvdata(dev);
 
 	spin_lock(&ch_index_lock);
-	idr_remove(&ch_index_idr, ch->minor);
+	idr_replace(&ch_index_idr, NULL, ch->minor);
 	dev_set_drvdata(dev, NULL);
 	spin_unlock(&ch_index_lock);
 
 	device_destroy(&ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR, ch->minor));
+
+	spin_lock(&ch_index_lock);
+	idr_remove(&ch_index_idr, ch->minor);
+	spin_unlock(&ch_index_lock);
 	scsi_device_put(ch->device);
 	kref_put(&ch->ref, ch_destroy);
 }

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0

Re: [PATCH] scsi: ch: Remove the minor from ch_index_idr after device_destroy()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] scsi: ch: use-after-free in ch_probe() [1]
--

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

scsi: ch: Remove the minor from ch_index_idr after device_destroy()

This commit replaces the minor's entry in ch_index_idr with NULL before
calling device_destroy() in ch_remove() and removes the minor from the
IDR only afterward so a concurrent probe cannot reuse the minor while the
sysfs device still exists.

> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index b2c9fb71c9e0a..390425671a3a7 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -952,11 +952,15 @@ static void ch_remove(struct scsi_device *sd)
>  	scsi_changer *ch = dev_get_drvdata(dev);
>  
>  	spin_lock(&ch_index_lock);
> -	idr_remove(&ch_index_idr, ch->minor);
> +	idr_replace(&ch_index_idr, NULL, ch->minor);
>  	dev_set_drvdata(dev, NULL);
>  	spin_unlock(&ch_index_lock);
>  
>  	device_destroy(&ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR, ch->minor));
> +
> +	spin_lock(&ch_index_lock);
> +	idr_remove(&ch_index_idr, ch->minor);
> +	spin_unlock(&ch_index_lock);
>  	scsi_device_put(ch->device);
>  	kref_put(&ch->ref, ch_destroy);
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can ch_probe() trigger
a use-after-free on ch [1] if an error occurs after ch is published
in ch_index_idr?

ch_probe() inserts ch directly into ch_index_idr and initializes ch->ref
before calling device_create() and ch_readconfig(), and its error path
frees ch with kfree(ch) instead of kref_put():

drivers/scsi/ch.c:ch_probe() {
	...
	ret = idr_alloc(&ch_index_idr, ch, 0, CH_MAX_DEVS + 1, GFP_NOWAIT);
	...
	kref_init(&ch->ref);
	...
	ret = ch_readconfig(ch);
	if (ret) {
		mutex_unlock(&ch->lock);
		goto destroy_dev;
	}
	...
destroy_dev:
	device_destroy(&ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR, ch->minor));
put_device:
	scsi_device_put(sd);
remove_idr:
	idr_remove(&ch_index_idr, ch->minor);
free_ch:
	kfree(ch);
	return ret;
}

If ch_readconfig() fails after a concurrent ch_open() looks up ch in
ch_index_idr and takes a reference, ch_open() dereferences freed memory:

CPU0                              CPU1
ch_probe()
  idr_alloc(&ch_index_idr, ch, ...)
  kref_init(&ch->ref)
                                  ch_open()
                                    ch = idr_find(&ch_index_idr, minor)
                                    kref_get(&ch->ref)
  ch_readconfig(ch) /* fails */
  idr_remove(&ch_index_idr, ch->minor)
  kfree(ch)
                                    mutex_lock(&ch->lock)  <-- use-after-free

[1] https://sashiko.dev/#/bug/linux-f4be7a8e-e1b8-4f47-9f47-c6a01d983399

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

Re: [PATCH] scsi: ch: Remove the minor from ch_index_idr after device_destroy()

[Laurence Oberman]

On Mon, 2026-11-09 at 11:06 -0500, Changyul Lee wrote:
> ch_remove() removes the minor from ch_index_idr before
> device_destroy()
> removes the "sch<N>" device. A changer probed in between can get the
> same minor, and its device_create() fails:
> 
>   sysfs: cannot create duplicate filename '/class/scsi_changer/sch0'
>   ...
>   ch 1:0:0:0: ch0: device_create failed
> 
> The new changer is then not attached.
> 
> Replace the IDR entry with NULL instead, so that ch_open() still
> fails
> with -ENXIO for the changer being removed, and remove the minor after
> device_destroy().
> 
> Fixes: da707c54c342 ("[SCSI] ch: fix device minor number management
> bug")
> Signed-off-by: Changyul Lee <lcy8047@gmail.com>
> ---
> Reproduced in QEMU (x86_64) with scsi_debug (ptype=8, add_host=2),
> which gives two changers on two hosts. Two threads, one per host,
> delete the changer through sysfs and rescan the host in a loop for 20
> seconds. Without this patch, device_create() failed 992 to 1151 times
> per run in three runs. With it, it never failed in two runs, and
> /dev/sch0 can still be opened before and after a delete and rescan.
> 
>  drivers/scsi/ch.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index b2c9fb71c9e0..390425671a3a 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -952,11 +952,15 @@ static void ch_remove(struct scsi_device *sd)
>  	scsi_changer *ch = dev_get_drvdata(dev);
>  
>  	spin_lock(&ch_index_lock);
> -	idr_remove(&ch_index_idr, ch->minor);
> +	idr_replace(&ch_index_idr, NULL, ch->minor);
>  	dev_set_drvdata(dev, NULL);
>  	spin_unlock(&ch_index_lock);
>  
>  	device_destroy(&ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR,
> ch->minor));
> +
> +	spin_lock(&ch_index_lock);
> +	idr_remove(&ch_index_idr, ch->minor);
> +	spin_unlock(&ch_index_lock);
>  	scsi_device_put(ch->device);
>  	kref_put(&ch->ref, ch_destroy);
>  }
> 
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4

Looks good to me:
Reviewed-by: Laurence Oberman <loberman@redhat.com>