On 2019/4/1 18:12, Jan Kara wrote:
On Mon 01-04-19 17:32:32, yuyufen wrote:
add Cc
On 2019/4/1 17:34, Yufen Yu wrote:
commit 2da78092dda "block: Fix dev_t minor allocation lifetime"
specifically moved blk_free_devt(dev->devt) call to part_release()
to avoid reallocating device number before the device is fully
shutdown.
However, it can cause use-after-free on gendisk in get_gendisk().
We use md device as example to show the race scenes:
Process1 Worker Process2
md_free
blkdev_open
del_gendisk
add delete_partition_work_fn() to wq
__blkdev_get
get_gendisk
put_disk
disk_release
kfree(disk)
find part from ext_devt_idr
get_disk_and_module(disk)
cause use after free
delete_partition_work_fn
put_device(part)
part_release
remove part from ext_devt_idr
Before <devt, hd_struct pointer> is removed from ext_devt_idr by
delete_partition_work_fn(), we can find the devt and then access
gendisk by hd_struct pointer. But, if we access the gendisk after
it have been freed, it can cause in use-after-freeon gendisk in
get_gendisk().
We fix this by adding a new helper blk_invalidate_devt() in
delete_partition() and del_gendisk(). It replaces hd_struct
pointer in idr with value 'NULL', and deletes the entry from
idr in part_release() as we do now.
Fixes: 2da78092dda1 ("block: Fix dev_t minor allocation lifetime")
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: Bart Van Assche <bart.vanassche@xxxxxxx>
Cc: Jan Kara <jack@xxxxxxx>
Cc: Keith Busch <keith.busch@xxxxxxxxx>
Signed-off-by: Yufen Yu <yuyufen@xxxxxxxxxx>
Thanks for the patch! Just two nits below:
diff --git a/block/genhd.c b/block/genhd.c
index 961b2bc4634f..7144153c6bf1 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -529,6 +529,18 @@ void blk_free_devt(dev_t devt)
}
}
+/**
+ * We invalidate devt by assigning NULL pointer for devt in idr.
+ */
+void blk_invalidate_devt(dev_t devt)
+{
+ if (MAJOR(devt) == BLOCK_EXT_MAJOR) {
+ spin_lock_bh(&ext_devt_lock);
+ idr_replace(&ext_devt_idr, NULL, blk_mangle_minor(MINOR(devt)));
+ spin_unlock_bh(&ext_devt_lock);
+ }
+}
+
static char *bdevt_str(dev_t devt, char *buf)
{
if (MAJOR(devt) <= 0xff && MINOR(devt) <= 0xff) {
@@ -801,6 +813,11 @@ void del_gendisk(struct gendisk *disk)
sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
device_del(disk_to_dev(disk));
+
+ /*
+ * we need to invalidate devt before remove it from idr.
+ */
+ blk_invalidate_devt(disk_devt(disk));
I would move this slightly up to where blk_unregister_region() is called
because these two are just different means to lookup the gendisk (idr or
bdev_map depending of major number used). Also I'd update the comment to
something like:
/*
* Remove gendisk pointer from idr so that it cannot be looked up
* while RCU period before freeing gendisk is running to prevent
* use-after-free issues. Note that the device number stays
* "in-use" until we really free the gendisk.
*/
Thanks a lot for your nice suggestion. As you say, they are just two
different means to lookup
the gendisk. So, moving blk_invalidate_devt() up to
blk_unregister_region() is more reasonable
and also improve the code readability.
Yufen
Thanks
}
EXPORT_SYMBOL(del_gendisk);
diff --git a/block/partition-generic.c b/block/partition-generic.c
index 1ee3e1d1bc2a..922230b5a907 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -288,6 +288,12 @@ void delete_partition(struct gendisk *disk, int partno)
kobject_put(part->holder_dir);
device_del(part_to_dev(part));
+ /*
+ * We need to invalidate devt by assigning NULL pointer for devt
+ * before remove it from ext_devt_idr, which can avoid use-after-free
+ * on gendisk.
+ */
+ blk_invalidate_devt(part_devt(part));
hd_struct_kill(part);
}
And here I'd just use the same comment as above.
Honza
