[dm-devel] [PATCH 7/7] block: store the holder kobject in bd_holder_disk

[Yu Kuai]

Hi

在 2022/10/30 23:31, Christoph Hellwig 写道:
> We hold a reference to the holder kobject for each bd_holder_disk,
> so to make the code a bit more robust, use a reference to it instead
> of the block_device.  As long as no one clears ->bd_holder_dir in
> before freeing the disk, this isn't strictly required, but it does
> make the code more clear and more robust.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>   block/holder.c | 23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/block/holder.c b/block/holder.c
> index dd9327b43ce05..a8c355b9d0806 100644
> --- a/block/holder.c
> +++ b/block/holder.c
> @@ -4,7 +4,7 @@
>   
>   struct bd_holder_disk {
>   	struct list_head	list;
> -	struct block_device	*bdev;
> +	struct kobject		*holder_dir;
>   	int			refcnt;
>   };
>   
> @@ -14,7 +14,7 @@ static struct bd_holder_disk *bd_find_holder_disk(struct block_device *bdev,
>   	struct bd_holder_disk *holder;
>   
>   	list_for_each_entry(holder, &disk->slave_bdevs, list)
> -		if (holder->bdev == bdev)
> +		if (holder->holder_dir == bdev->bd_holder_dir)
>   			return holder;
>   	return NULL;
>   }
> @@ -82,27 +82,24 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
>   	}
>   
>   	INIT_LIST_HEAD(&holder->list);
> -	holder->bdev = bdev;
>   	holder->refcnt = 1;
> +	holder->holder_dir = kobject_get(bdev->bd_holder_dir);

I wonder is this safe here, if kobject reference is 0 here and
bd_holder_dir is about to be freed. Here in kobject_get, kref_get() will
warn about uaf, and kobject_get will return a address that is about to
be freed.

Thansk,
Kuai
> +
>   	ret = add_symlink(disk->slave_dir, bdev_kobj(bdev));
>   	if (ret)
> -		goto out_free_holder;
> -	ret = add_symlink(bdev->bd_holder_dir, &disk_to_dev(disk)->kobj);
> +		goto out_put_holder_dir;
> +	ret = add_symlink(holder->holder_dir, &disk_to_dev(disk)->kobj);
>   	if (ret)
>   		goto out_del_symlink;
>   	list_add(&holder->list, &disk->slave_bdevs);
>   
> -	/*
> -	 * del_gendisk drops the initial reference to bd_holder_dir, so we need
> -	 * to keep our own here to allow for cleanup past that point.
> -	 */
> -	kobject_get(bdev->bd_holder_dir);
>   	mutex_unlock(&disk->open_mutex);
>   	return 0;
>   
>   out_del_symlink:
>   	del_symlink(disk->slave_dir, bdev_kobj(bdev));
> -out_free_holder:
> +out_put_holder_dir:
> +	kobject_put(holder->holder_dir);
>   	kfree(holder);
>   out_unlock:
>   	mutex_unlock(&disk->open_mutex);
> @@ -131,8 +128,8 @@ void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk)
>   	holder = bd_find_holder_disk(bdev, disk);
>   	if (!WARN_ON_ONCE(holder == NULL) && !--holder->refcnt) {
>   		del_symlink(disk->slave_dir, bdev_kobj(bdev));
> -		del_symlink(bdev->bd_holder_dir, &disk_to_dev(disk)->kobj);
> -		kobject_put(bdev->bd_holder_dir);
> +		del_symlink(holder->holder_dir, &disk_to_dev(disk)->kobj);
> +		kobject_put(holder->holder_dir);
>   		list_del_init(&holder->list);
>   		kfree(holder);
>   	}
>