Re: [PATCH 7/7] block: store the holder kobject in bd_holder_disk

From: Christoph Hellwig <hch@lst.de>
To: Yu Kuai <yukuai1@huaweicloud.com>
Cc: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>,
	Alasdair Kergon <agk@redhat.com>,
	Mike Snitzer <snitzer@kernel.org>,
	dm-devel@redhat.com, linux-block@vger.kernel.org,
	"yukuai (C)" <yukuai3@huawei.com>
Subject: Re: [PATCH 7/7] block: store the holder kobject in bd_holder_disk
Date: Tue, 1 Nov 2022 12:21:31 +0100	[thread overview]
Message-ID: <20221101112131.GA14379@lst.de> (raw)
In-Reply-To: <d3f6ec1d-8141-19d1-ce4c-d42710f4a636@huaweicloud.com>

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);
