[PATCH] scsi: bsg: Fix device unregistration

[PATCH] scsi: bsg: Fix device unregistration

[Zenghui Yu]

We use device_initialize() to take refcount for the device but forget to
put_device() on device teardown, which ends up leaking private data of the
driver core, dev_name(), etc. This is reported by kmemleak at boot time if
we compile kernel with DEBUG_TEST_DRIVER_REMOVE.

Note that adding the missing put_device() is _not_ sufficient to fix device
unregistration. As we don't provide the .release() method for device, which
turned out to be typically wrong and will be complained loudly by the
driver core.

Fix both of them.

Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 block/bsg.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 351095193788..c3bb92b9af7e 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -165,13 +165,20 @@ static const struct file_operations bsg_fops = {
 	.llseek		=	default_llseek,
 };
 
+static void bsg_device_release(struct device *dev)
+{
+	struct bsg_device *bd = container_of(dev, struct bsg_device, device);
+
+	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
+	kfree(bd);
+}
+
 void bsg_unregister_queue(struct bsg_device *bd)
 {
 	if (bd->queue->kobj.sd)
 		sysfs_remove_link(&bd->queue->kobj, "bsg");
 	cdev_device_del(&bd->cdev, &bd->device);
-	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
-	kfree(bd);
+	put_device(&bd->device);
 }
 EXPORT_SYMBOL_GPL(bsg_unregister_queue);
 
@@ -198,6 +205,7 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
 	bd->device.devt = MKDEV(bsg_major, ret);
 	bd->device.class = bsg_class;
 	bd->device.parent = parent;
+	bd->device.release = bsg_device_release;
 	dev_set_name(&bd->device, "%s", name);
 	device_initialize(&bd->device);
 
@@ -218,6 +226,7 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
 out_device_del:
 	cdev_device_del(&bd->cdev, &bd->device);
 out_ida_remove:
+	put_device(&bd->device);
 	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
 out_kfree:
 	kfree(bd);
-- 
2.19.1

Re: [PATCH] scsi: bsg: Fix device unregistration

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] scsi: bsg: Fix device unregistration

[Greg KH]

On Thu, Sep 09, 2021 at 11:46:08AM +0800, Zenghui Yu wrote:
> We use device_initialize() to take refcount for the device but forget to
> put_device() on device teardown, which ends up leaking private data of the
> driver core, dev_name(), etc. This is reported by kmemleak at boot time if
> we compile kernel with DEBUG_TEST_DRIVER_REMOVE.
> 
> Note that adding the missing put_device() is _not_ sufficient to fix device
> unregistration. As we don't provide the .release() method for device, which
> turned out to be typically wrong and will be complained loudly by the
> driver core.
> 
> Fix both of them.
> 
> Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  block/bsg.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] scsi: bsg: Fix device unregistration

[Jens Axboe]

On 9/8/21 9:46 PM, Zenghui Yu wrote:
> We use device_initialize() to take refcount for the device but forget to
> put_device() on device teardown, which ends up leaking private data of the
> driver core, dev_name(), etc. This is reported by kmemleak at boot time if
> we compile kernel with DEBUG_TEST_DRIVER_REMOVE.
> 
> Note that adding the missing put_device() is _not_ sufficient to fix device
> unregistration. As we don't provide the .release() method for device, which
> turned out to be typically wrong and will be complained loudly by the
> driver core.
> 
> Fix both of them.

Applied, thanks.

-- 
Jens Axboe

Re: [PATCH] scsi: bsg: Fix device unregistration

[Jens Axboe]

On 9/9/21 2:40 PM, Jens Axboe wrote:
> On 9/8/21 9:46 PM, Zenghui Yu wrote:
>> We use device_initialize() to take refcount for the device but forget to
>> put_device() on device teardown, which ends up leaking private data of the
>> driver core, dev_name(), etc. This is reported by kmemleak at boot time if
>> we compile kernel with DEBUG_TEST_DRIVER_REMOVE.
>>
>> Note that adding the missing put_device() is _not_ sufficient to fix device
>> unregistration. As we don't provide the .release() method for device, which
>> turned out to be typically wrong and will be complained loudly by the
>> driver core.
>>
>> Fix both of them.
> 
> Applied, thanks.

Actually, let's move this through the SCSI tree, as the offending patch
went that way (and my branches are behind that point).

-- 
Jens Axboe

Re: [PATCH] scsi: bsg: Fix device unregistration

[Martin K. Petersen]

Jens,

> Actually, let's move this through the SCSI tree, as the offending
> patch went that way (and my branches are behind that point).

Sure. I queued it up...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: bsg: Fix device unregistration

[Christoph Hellwig]

On Thu, Sep 09, 2021 at 02:42:10PM -0600, Jens Axboe wrote:
> Actually, let's move this through the SCSI tree, as the offending patch
> went that way (and my branches are behind that point).

Btw, should we move bsg.c and bsg-lib.c to drivers/scsi/?  They very much
are SCSI infrastructure now.

Re: [PATCH] scsi: bsg: Fix device unregistration

[Johan Hovold]

On Thu, Sep 09, 2021 at 11:46:08AM +0800, Zenghui Yu wrote:
> We use device_initialize() to take refcount for the device but forget to
> put_device() on device teardown, which ends up leaking private data of the
> driver core, dev_name(), etc. This is reported by kmemleak at boot time if
> we compile kernel with DEBUG_TEST_DRIVER_REMOVE.
> 
> Note that adding the missing put_device() is _not_ sufficient to fix device
> unregistration. As we don't provide the .release() method for device, which
> turned out to be typically wrong and will be complained loudly by the
> driver core.
> 
> Fix both of them.
> 
> Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  block/bsg.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
 
> +static void bsg_device_release(struct device *dev)
> +{
> +	struct bsg_device *bd = container_of(dev, struct bsg_device, device);
> +
> +	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
> +	kfree(bd);
> +}

> @@ -198,6 +205,7 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
>  	bd->device.devt = MKDEV(bsg_major, ret);
>  	bd->device.class = bsg_class;
>  	bd->device.parent = parent;
> +	bd->device.release = bsg_device_release;
>  	dev_set_name(&bd->device, "%s", name);
>  	device_initialize(&bd->device);
>  
> @@ -218,6 +226,7 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
>  out_device_del:
>  	cdev_device_del(&bd->cdev, &bd->device);
>  out_ida_remove:
> +	put_device(&bd->device);
>  	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
>  out_kfree:
>  	kfree(bd);

Ehh, what about the blatant use-after-free and double-free you just
added here?

Martin, can this still be dropped from the scsi tree or does it need to
be fixed incrementally?

Johan

Re: [PATCH] scsi: bsg: Fix device unregistration

[Martin K. Petersen]

Johan,

> Martin, can this still be dropped from the scsi tree or does it need to
> be fixed incrementally?

I dropped it.

Zenghui: Please fix.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: bsg: Fix device unregistration

[Zenghui Yu]

On 2021/9/10 20:45, Johan Hovold wrote:
> On Thu, Sep 09, 2021 at 11:46:08AM +0800, Zenghui Yu wrote:

>> @@ -218,6 +226,7 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
>>  out_device_del:
>>  	cdev_device_del(&bd->cdev, &bd->device);
>>  out_ida_remove:
>> +	put_device(&bd->device);
>>  	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
>>  out_kfree:
>>  	kfree(bd);
> 
> Ehh, what about the blatant use-after-free and double-free you just
> added here?

Yeah, whoops. That's definitely wrong. I'm squash the following fix
in this patch.

Thanks for the heads up!

|diff --git a/block/bsg.c b/block/bsg.c
|index c3bb92b9af7e..882f56bff14f 100644
|--- a/block/bsg.c
|+++ b/block/bsg.c
|@@ -200,7 +200,8 @@ struct bsg_device *bsg_register_queue(struct 
request_queue *q,
|        if (ret < 0) {
|                if (ret == -ENOSPC)
|                        dev_err(parent, "bsg: too many bsg devices\n");
|-               goto out_kfree;
|+               kfree(bd);
|+               return ERR_PTR(ret);
|        }
|        bd->device.devt = MKDEV(bsg_major, ret);
|        bd->device.class = bsg_class;
|@@ -213,7 +214,7 @@ struct bsg_device *bsg_register_queue(struct 
request_queue *q,
|        bd->cdev.owner = THIS_MODULE;
|        ret = cdev_device_add(&bd->cdev, &bd->device);
|        if (ret)
|-               goto out_ida_remove;
|+               goto out_put_device;
|
|        if (q->kobj.sd) {
|                ret = sysfs_create_link(&q->kobj, &bd->device.kobj, "bsg");
|@@ -225,11 +226,8 @@ struct bsg_device *bsg_register_queue(struct 
request_queue *q,
|
| out_device_del:
|        cdev_device_del(&bd->cdev, &bd->device);
|-out_ida_remove:
|+out_put_device:
|        put_device(&bd->device);
|-       ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
|-out_kfree:
|-       kfree(bd);
|        return ERR_PTR(ret);
| }
| EXPORT_SYMBOL_GPL(bsg_register_queue);