[PATCH v2] ubi: block: Fix cleanup handling

[PATCH v2] ubi: block: Fix cleanup handling

[Vincent Whitchurch]

ubiblock's remove handling has a couple of problems:

 - It uses the gendisk after put_disk(), resulting in a use-after-free.

 - There is a circular locking dependency between disk->open_mutex (used
   from del_gendisk() and blkdev_open()) and dev->dev_mutex (used from
   ubiblock_open() and ubiblock_remove()).

Fix these by implementing ->free_disk() and moving the final cleanup
there.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
Changes in v2:
- Combine and rework patches to implement and use ->free_disk().
- Link to v1: https://lore.kernel.org/r/20230523-ubiblock-remove-v1-0-240bed75849b@axis.com
---
 drivers/mtd/ubi/block.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 3711d7f74600..570e660673ad 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -293,11 +293,23 @@ static int ubiblock_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return 0;
 }
 
+static void ubiblock_free_disk(struct gendisk *disk)
+{
+	struct ubiblock *dev = disk->private_data;
+
+	mutex_lock(&devices_mutex);
+	idr_remove(&ubiblock_minor_idr, disk->first_minor);
+	mutex_unlock(&devices_mutex);
+
+	kfree(dev);
+}
+
 static const struct block_device_operations ubiblock_ops = {
 	.owner = THIS_MODULE,
 	.open = ubiblock_open,
 	.release = ubiblock_release,
 	.getgeo	= ubiblock_getgeo,
+	.free_disk = ubiblock_free_disk,
 };
 
 static blk_status_t ubiblock_queue_rq(struct blk_mq_hw_ctx *hctx,
@@ -452,9 +464,8 @@ static void ubiblock_cleanup(struct ubiblock *dev)
 	del_gendisk(dev->gd);
 	/* Finally destroy the blk queue */
 	dev_info(disk_to_dev(dev->gd), "released");
-	put_disk(dev->gd);
 	blk_mq_free_tag_set(&dev->tag_set);
-	idr_remove(&ubiblock_minor_idr, dev->gd->first_minor);
+	put_disk(dev->gd);
 }
 
 int ubiblock_remove(struct ubi_volume_info *vi)
@@ -478,11 +489,11 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 
 	/* Remove from device list */
 	list_del(&dev->list);
-	ubiblock_cleanup(dev);
 	mutex_unlock(&dev->dev_mutex);
 	mutex_unlock(&devices_mutex);
 
-	kfree(dev);
+	ubiblock_cleanup(dev);
+
 	return 0;
 
 out_unlock_dev:
@@ -623,17 +634,19 @@ static void ubiblock_remove_all(void)
 {
 	struct ubiblock *next;
 	struct ubiblock *dev;
+	LIST_HEAD(list);
 
 	mutex_lock(&devices_mutex);
-	list_for_each_entry_safe(dev, next, &ubiblock_devices, list) {
+	list_splice_init(&ubiblock_devices, &list);
+	mutex_unlock(&devices_mutex);
+
+	list_for_each_entry_safe(dev, next, &list, list) {
 		/* The module is being forcefully removed */
 		WARN_ON(dev->desc);
 		/* Remove from device list */
 		list_del(&dev->list);
 		ubiblock_cleanup(dev);
-		kfree(dev);
 	}
-	mutex_unlock(&devices_mutex);
 }
 
 int __init ubiblock_init(void)

---
base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
change-id: 20230523-ubiblock-remove-eab61cf683f0

Best regards,
-- 
Vincent Whitchurch <vincent.whitchurch@axis.com>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2] ubi: block: Fix cleanup handling

[Zhihao Cheng]

在 2023/6/7 17:25, Vincent Whitchurch 写道:
> ubiblock's remove handling has a couple of problems:
> 
>   - It uses the gendisk after put_disk(), resulting in a use-after-free.
> 
>   - There is a circular locking dependency between disk->open_mutex (used
>     from del_gendisk() and blkdev_open()) and dev->dev_mutex (used from
>     ubiblock_open() and ubiblock_remove()).
> 
> Fix these by implementing ->free_disk() and moving the final cleanup
> there.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> Changes in v2:
> - Combine and rework patches to implement and use ->free_disk().
> - Link to v1: https://lore.kernel.org/r/20230523-ubiblock-remove-v1-0-240bed75849b@axis.com
> ---
>   drivers/mtd/ubi/block.c | 27 ++++++++++++++++++++-------
>   1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
> index 3711d7f74600..570e660673ad 100644
> --- a/drivers/mtd/ubi/block.c
> +++ b/drivers/mtd/ubi/block.c
> @@ -293,11 +293,23 @@ static int ubiblock_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>   	return 0;
>   }
>   
> +static void ubiblock_free_disk(struct gendisk *disk)
> +{
> +	struct ubiblock *dev = disk->private_data;
> +
> +	mutex_lock(&devices_mutex);
> +	idr_remove(&ubiblock_minor_idr, disk->first_minor);
> +	mutex_unlock(&devices_mutex);
> +
> +	kfree(dev);
> +}
> +
>   static const struct block_device_operations ubiblock_ops = {
>   	.owner = THIS_MODULE,
>   	.open = ubiblock_open,
>   	.release = ubiblock_release,
>   	.getgeo	= ubiblock_getgeo,
> +	.free_disk = ubiblock_free_disk,
>   };
>   
>   static blk_status_t ubiblock_queue_rq(struct blk_mq_hw_ctx *hctx,
> @@ -452,9 +464,8 @@ static void ubiblock_cleanup(struct ubiblock *dev)
>   	del_gendisk(dev->gd);
>   	/* Finally destroy the blk queue */
>   	dev_info(disk_to_dev(dev->gd), "released");
> -	put_disk(dev->gd);
>   	blk_mq_free_tag_set(&dev->tag_set);
> -	idr_remove(&ubiblock_minor_idr, dev->gd->first_minor);
> +	put_disk(dev->gd);

I thought it's better to do put_disk() first then do 
blk_mq_free_tag_set(), likes nbd, loop does. Will 
put_disk->disk_release->blk_mq_exit_queue->blk_mq_exit_hw_queues access 
tag_set which has been freed by blk_mq_free_tag_set()?

>   }
>   
>   int ubiblock_remove(struct ubi_volume_info *vi)
> @@ -478,11 +489,11 @@ int ubiblock_remove(struct ubi_volume_info *vi)
>   
>   	/* Remove from device list */
>   	list_del(&dev->list);
> -	ubiblock_cleanup(dev);
>   	mutex_unlock(&dev->dev_mutex);
>   	mutex_unlock(&devices_mutex);
>   
> -	kfree(dev);
> +	ubiblock_cleanup(dev);
> +
>   	return 0;
>   
>   out_unlock_dev:
> @@ -623,17 +634,19 @@ static void ubiblock_remove_all(void)
>   {
>   	struct ubiblock *next;
>   	struct ubiblock *dev;
> +	LIST_HEAD(list);
>   
>   	mutex_lock(&devices_mutex);
> -	list_for_each_entry_safe(dev, next, &ubiblock_devices, list) {
> +	list_splice_init(&ubiblock_devices, &list);
> +	mutex_unlock(&devices_mutex);
> +
> +	list_for_each_entry_safe(dev, next, &list, list) {
>   		/* The module is being forcefully removed */
>   		WARN_ON(dev->desc);
>   		/* Remove from device list */
>   		list_del(&dev->list);
>   		ubiblock_cleanup(dev);
> -		kfree(dev);
>   	}
> -	mutex_unlock(&devices_mutex);
>   }
>   
>   int __init ubiblock_init(void)
> 
> ---
> base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
> change-id: 20230523-ubiblock-remove-eab61cf683f0
> 
> Best regards,
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2] ubi: block: Fix cleanup handling

[Christoph Hellwig]

Looks good:

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2] ubi: block: Fix cleanup handling

[Zhihao Cheng]

在 2023/6/9 11:16, Zhihao Cheng 写道:
> 在 2023/6/7 17:25, Vincent Whitchurch 写道:
>> ubiblock's remove handling has a couple of problems:
>>
>>   - It uses the gendisk after put_disk(), resulting in a use-after-free.
>>
>>   - There is a circular locking dependency between disk->open_mutex (used
>>     from del_gendisk() and blkdev_open()) and dev->dev_mutex (used from
>>     ubiblock_open() and ubiblock_remove()).
>>
>> Fix these by implementing ->free_disk() and moving the final cleanup
>> there.
>>
>> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
>> ---
>> Changes in v2:
>> - Combine and rework patches to implement and use ->free_disk().
>> - Link to v1: 
>> https://lore.kernel.org/r/20230523-ubiblock-remove-v1-0-240bed75849b@axis.com 
>>
>> ---
>>   drivers/mtd/ubi/block.c | 27 ++++++++++++++++++++-------
>>   1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
>> index 3711d7f74600..570e660673ad 100644
>> --- a/drivers/mtd/ubi/block.c
>> +++ b/drivers/mtd/ubi/block.c
>> @@ -293,11 +293,23 @@ static int ubiblock_getgeo(struct block_device 
>> *bdev, struct hd_geometry *geo)
>>       return 0;
>>   }
>> +static void ubiblock_free_disk(struct gendisk *disk)
>> +{
>> +    struct ubiblock *dev = disk->private_data;
>> +
>> +    mutex_lock(&devices_mutex);
>> +    idr_remove(&ubiblock_minor_idr, disk->first_minor);
>> +    mutex_unlock(&devices_mutex);
>> +
>> +    kfree(dev);
>> +}
>> +
>>   static const struct block_device_operations ubiblock_ops = {
>>       .owner = THIS_MODULE,
>>       .open = ubiblock_open,
>>       .release = ubiblock_release,
>>       .getgeo    = ubiblock_getgeo,
>> +    .free_disk = ubiblock_free_disk,
>>   };
>>   static blk_status_t ubiblock_queue_rq(struct blk_mq_hw_ctx *hctx,
>> @@ -452,9 +464,8 @@ static void ubiblock_cleanup(struct ubiblock *dev)
>>       del_gendisk(dev->gd);
>>       /* Finally destroy the blk queue */
>>       dev_info(disk_to_dev(dev->gd), "released");
>> -    put_disk(dev->gd);
>>       blk_mq_free_tag_set(&dev->tag_set);
>> -    idr_remove(&ubiblock_minor_idr, dev->gd->first_minor);
>> +    put_disk(dev->gd);
> 
> I thought it's better to do put_disk() first then do 
> blk_mq_free_tag_set(), likes nbd, loop does. Will 
> put_disk->disk_release->blk_mq_exit_queue->blk_mq_exit_hw_queues access 
> tag_set which has been freed by blk_mq_free_tag_set()?
> 

All right, just ignore this comment. The nbd doesn't implement 
'->free_disk'. The 'ubiblock' will be freed in '->free_disk' invoked 
from put_disk, so 'blk_mq_free_tag_set(&dev->tag_set)' should be 
executed before put_disk.

>>   }
>>   int ubiblock_remove(struct ubi_volume_info *vi)
>> @@ -478,11 +489,11 @@ int ubiblock_remove(struct ubi_volume_info *vi)
>>       /* Remove from device list */
>>       list_del(&dev->list);
>> -    ubiblock_cleanup(dev);
>>       mutex_unlock(&dev->dev_mutex);
>>       mutex_unlock(&devices_mutex);
>> -    kfree(dev);
>> +    ubiblock_cleanup(dev);
>> +
>>       return 0;
>>   out_unlock_dev:
>> @@ -623,17 +634,19 @@ static void ubiblock_remove_all(void)
>>   {
>>       struct ubiblock *next;
>>       struct ubiblock *dev;
>> +    LIST_HEAD(list);
>>       mutex_lock(&devices_mutex);
>> -    list_for_each_entry_safe(dev, next, &ubiblock_devices, list) {
>> +    list_splice_init(&ubiblock_devices, &list);
>> +    mutex_unlock(&devices_mutex);
>> +
>> +    list_for_each_entry_safe(dev, next, &list, list) {
>>           /* The module is being forcefully removed */
>>           WARN_ON(dev->desc);
>>           /* Remove from device list */
>>           list_del(&dev->list);
>>           ubiblock_cleanup(dev);
>> -        kfree(dev);
>>       }
>> -    mutex_unlock(&devices_mutex);
>>   }
>>   int __init ubiblock_init(void)
>>
>> ---
>> base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
>> change-id: 20230523-ubiblock-remove-eab61cf683f0
>>
>> Best regards,
>>
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/