Re: [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 <h...@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);
        }

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel