Skip to contentSkip to conversation

block: don't claim devices that are not live in bd_link_disk_holder

PATCH Christoph Hellwig 2 participants 5 messages Nov 2, 2022 – Nov 3, 2022

Subsystems (2)
Block layer Device mapper
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

For gendisk that are not live or their partitions, the bd_holder_dir pointer is not valid and the kobject might not have been allocated yet or freed already. Check that the disk is live before creating the linkage and error out otherwise.

Signed-off-by Christoph Hellwig <[email protected]>
---
 block/holder.c | 5 +++++
 1 file changed, 5 insertions(+)
block/holder.c +5 -0
index a8c355b9d0806..a8806bbed2112 100644
@@ -66,6 +66,11 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
 		return -EINVAL;
 
 	mutex_lock(&disk->open_mutex);
+	/* bd_holder_dir is only valid for live disks */
+	if (!disk_live(bdev->bd_disk)) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
 
 	WARN_ON_ONCE(!bdev->bd_holder);
Signature · 1 line
2.30.2
#1 lore raw

Hi,

Quoted 15 lines · Christoph Hellwig

For gendisk that are not live or their partitions, the bd_holder_dir pointer is not valid and the kobject might not have been allocated yet or freed already. Check that the disk is live before creating the linkage and error out otherwise.

Signed-off-by Christoph Hellwig <[email protected]>
---
  block/holder.c | 5 +++++
  1 file changed, 5 insertions(+)
block/holder.c +0 -0
index a8c355b9d0806..a8806bbed2112 100644

block/holder.c int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk) · line 73

Quoted 8 lines
block/holder.c +5 -0
@@ -66,6 +66,11 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
  		return -EINVAL;
 
  	mutex_lock(&disk->open_mutex);
+	/* bd_holder_dir is only valid for live disks */
+	if (!disk_live(bdev->bd_disk)) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}

I think this is still not safe 🤔

How about this:

block/holder.c +14 -5
index 5283bc804cc1..35fdfba558b8 100644
@@ -85,6 +85,20 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
                goto out_unlock;
        }
 
+       /*
+        * 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.
+        */
+       mutex_lock(&bdev->bd_disk->open_mutex);
+       if (!disk_live(bdev->bd_disk)) {
+               ret = -ENODEV;
+               mutex_unlock(&bdev->bd_disk->open_mutex);
+               goto out_unlock;
+       }
+
+       kobject_get(bdev->bd_holder_dir);
+       mutex_unlock(&bdev->bd_disk->open_mutex);
+
        holder = kzalloc(sizeof(*holder), GFP_KERNEL);
        if (!holder) {
                ret = -ENOMEM;

@@ -103,11 +117,6 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
        }
 
        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);

bdev->bd_disk->open_mutex should be hold, both dis_live() and kobject_get() should be protected.

Thansk,
Kuai

Quoted 4 lines
block/holder.c +0 -0
  	WARN_ON_ONCE(!bdev->bd_holder);
#2 lore raw
Quoted 70 lines · Yu Kuai

Hi,

Quoted 15 lines · Christoph Hellwig

For gendisk that are not live or their partitions, the bd_holder_dir pointer is not valid and the kobject might not have been allocated yet or freed already.  Check that the disk is live before creating the linkage and error out otherwise.

Signed-off-by Christoph Hellwig <[email protected]>

---
  block/holder.c | 5 +++++

  1 file changed, 5 insertions(+)
block/holder.c +0 -0
index a8c355b9d0806..a8806bbed2112 100644

block/holder.c int bd_link_disk_holder(struct block_device *bdev, · line 73

Quoted 8 lines
block/holder.c +5 -0
@@ -66,6 +66,11 @@ int bd_link_disk_holder(struct block_device *bdev, 
 struct gendisk *disk)
           return -EINVAL;
       mutex_lock(&disk->open_mutex);
+    /* bd_holder_dir is only valid for live disks */
+    if (!disk_live(bdev->bd_disk)) {
+        ret = -EINVAL;
+        goto out_unlock;
+    }

I think this is still not safe 🤔

How about this:

block/holder.c +2 -0
index 5283bc804cc1..35fdfba558b8 100644
@@ -85,6 +85,20 @@ int bd_link_disk_holder(struct block_device *bdev, 
 struct gendisk *disk)
                 goto out_unlock;
         }
 
+       /*
+        * 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. +        */
+       mutex_lock(&bdev->bd_disk->open_mutex);
+       if (!disk_live(bdev->bd_disk)) {
+               ret = -ENODEV;
+               mutex_unlock(&bdev->bd_disk->open_mutex); +               goto out_unlock;
+       }

block/holder.c +4 -2
+
+       kobject_get(bdev->bd_holder_dir);
+       mutex_unlock(&bdev->bd_disk->open_mutex);
+
         holder = kzalloc(sizeof(*holder), GFP_KERNEL);
         if (!holder) {
                 ret = -ENOMEM;

@@ -103,11 +117,6 @@ int bd_link_disk_holder(struct block_device *bdev, 
 struct gendisk *disk)
         }
 
         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);

Quoted 3 lines

bdev->bd_disk->open_mutex should be hold, both dis_live() and kobject_get() should be protected.

And kobject reference should be decreased in the following error path

Thanks,
Kuai

author
#3 lore raw
In reply to Yu Kuai
Quoted 1 line · Yu Kuai

I think this is still not safe 🤔

Indeed - wrong open_mutex.

block/holder.c int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk) · line 97

Quoted 11 lines
(quoted diff) +2 -0
+       /*
+        * del_gendisk drops the initial reference to bd_holder_dir, so we

need

block/holder.c +8 -0
+        * to keep our own here to allow for cleanup past that point.
+        */
+       mutex_lock(&bdev->bd_disk->open_mutex);
+       if (!disk_live(bdev->bd_disk)) {
+               ret = -ENODEV;
+               mutex_unlock(&bdev->bd_disk->open_mutex);
+               goto out_unlock;
+       }

I think this needs to be done before taking disk->open_mutex, otherwise we create a lock order dependency. Also the comment seems to overflow the usual 80 character limit, and as you noted in the next mail this needs more unwinding. But yes, otherwise this is the right thing to do. Do you want to send a replacement for this patch?

#4 lore raw

Hi,

Quoted 17 lines · Christoph Hellwig
Quoted 1 line · Yu Kuai

I think this is still not safe 🤔

Indeed - wrong open_mutex.

block/holder.c int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk) · line 97

Quoted 11 lines
(quoted diff) +2 -0
+       /*
+        * del_gendisk drops the initial reference to bd_holder_dir, so we

need

block/holder.c +8 -0
+        * to keep our own here to allow for cleanup past that point.
+        */
+       mutex_lock(&bdev->bd_disk->open_mutex);
+       if (!disk_live(bdev->bd_disk)) {
+               ret = -ENODEV;
+               mutex_unlock(&bdev->bd_disk->open_mutex);
+               goto out_unlock;
+       }
Quoted 6 lines

I think this needs to be done before taking disk->open_mutex, otherwise we create a lock order dependency. Also the comment seems to overflow the usual 80 character limit, and as you noted in the next mail this needs more unwinding. But yes, otherwise this is the right thing to do. Do you want to send a replacement for this patch?

Of course. And I just spotted a new problem here, I'll send them together.

Thanks,
Kuai

Quoted 2 lines

.