| Message ID | 20230523-ubiblock-remove-v2-1-7671fc60ba49@axis.com |
|---|---|
| State | New |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | [v2] ubi: block: Fix cleanup handling | expand |
在 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, >
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
在 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/
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)
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(-) --- base-commit: 44c026a73be8038f03dbdeef028b642880cf1511 change-id: 20230523-ubiblock-remove-eab61cf683f0 Best regards,