On Tue, Nov 01, 2022 at 07:12:51PM +0800, Yu Kuai wrote:
>> But how could the reference be 0 here? The driver that calls
>> bd_link_disk_holder must have the block device open and thus hold a
>> reference to it.
>
> Like I said before, the caller of bd_link_disk_holder() get bdev by
> blkdev_get_by_dev(), which do not grab reference of holder_dir, and
> grab disk reference can only prevent disk_release() to be called, not
> del_gendisk() while holder_dir reference is dropped in del_gendisk()
> and can be decreased to 0.
Oh, the bd_holder_dir reference, not the block_device one. So yes,
I agree.
> If you agree with above explanation, I tried to fix this:
>
> 1) move kobject_put(bd_holder_dir) from del_gendisk to disk_release,
> there seems to be a lot of other dependencies.
>
> 2) protect bd_holder_dir reference by open_mutex.
I think simply switching the kobject_get in bd_link_disk_holder
into a kobject_get_unless_zero and unwinding if there is no reference
should be enough:
diff --git a/block/holder.c b/block/holder.c
index a8c355b9d0806..cd18064f6ff80 100644
--- a/block/holder.c
+++ b/block/holder.c
@@ -83,7 +83,11 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
INIT_LIST_HEAD(&holder->list);
holder->refcnt = 1;
- holder->holder_dir = kobject_get(bdev->bd_holder_dir);
+ if (!kobject_get_unless_zero(bdev->bd_holder_dir)) {
+ ret = -EBUSY;
+ goto out_free_holder;
+ }
+ holder->holder_dir = bdev->bd_holder_dir;
ret = add_symlink(disk->slave_dir, bdev_kobj(bdev));
if (ret)
@@ -100,6 +104,7 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
del_symlink(disk->slave_dir, bdev_kobj(bdev));
out_put_holder_dir:
kobject_put(holder->holder_dir);
+out_free_holder:
kfree(holder);
out_unlock:
mutex_unlock(&disk->open_mutex);
