Skip to contentSkip to conversation

rust: block: fix GenDiskBuilder failure cleanup

PATCH In review Ren Wei 3 participants 3 messages May 24 – May 26 1 message not in the archive

Subsystems (3)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ren Wei

author
#1 lore raw

PATCH 1/1 rust: block: fix GenDiskBuilder failure cleanup

From: Haoze Xie <[email protected]>

If GenDiskBuilder::build() fails after __blk_mq_alloc_disk(), the allocated gendisk is left behind until the caller drops the last tagset reference.

Handle the failure path by releasing the temporary gendisk first, then converting the foreign queue data back, so probe failures clean up both resources before returning an error.

Fixes 3253aba3408aa ("rust: block: introduce `kernel::block::mq` module")
---
 rust/kernel/block/mq/gen_disk.rs | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
rust/kernel/block/mq/gen_disk.rs +12 -1
index 912cb805caf5..100c7b937a7e 100644
@@ -149,6 +149,17 @@ pub fn build<T: Operations>(
 
         // SAFETY: `gendisk` is a valid pointer as we initialized it above
         unsafe { (*gendisk).fops = &TABLE };
+        let cleanup_failure = ScopeGuard::new_with_data((gendisk, data), |(gendisk, data)| {
+            // SAFETY: `gendisk` came from `__blk_mq_alloc_disk()` above and
+            // has not been added to the VFS on this cleanup path.
+            unsafe { bindings::put_disk(gendisk) };
+            // SAFETY: `data` came from `into_foreign()` above and has not been
+            // converted back on this cleanup path.
+            drop(unsafe { T::QueueData::from_foreign(data) });
+        });
+        // The failure guard now owns both pieces of cleanup; the early guard
+        // must not run on this path anymore.
+        recover_data.dismiss();
 
         let mut writer = NullTerminatedFormatter::new(
             // SAFETY: `gendisk` points to a valid and initialized instance. We
@@ -172,7 +183,7 @@ pub fn build<T: Operations>(
             },
         )?;
 
-        recover_data.dismiss();
+        cleanup_failure.dismiss();
 
         // INVARIANT: `gendisk` was initialized above.
         // INVARIANT: `gendisk` was added to the VFS via `device_add_disk` above.
Signature · 1 line
2.47.3
maintainer Reviewed-by on v1 1/1
#2 lore raw
Quoted 10 lines · Ren Wei

From: Haoze Xie <[email protected]>

If GenDiskBuilder::build() fails after __blk_mq_alloc_disk(), the allocated gendisk is left behind until the caller drops the last tagset reference.

Handle the failure path by releasing the temporary gendisk first, then converting the foreign queue data back, so probe failures clean up both resources before returning an error.

Quoted 6 lines
Fixes 3253aba3408aa ("rust: block: introduce `kernel::block::mq` module")

Thanks for reporting and fixing!

I see we also lack a put_disk in GenDisk::drop after del_gendisk. Do you want to patch that as well?

Best regards,
Andreas Hindborg

#3 lore raw
Quoted 21 lines · Andreas Hindborg
Quoted 10 lines · Ren Wei

From: Haoze Xie <[email protected]>

If GenDiskBuilder::build() fails after __blk_mq_alloc_disk(), the allocated gendisk is left behind until the caller drops the last tagset reference.

Handle the failure path by releasing the temporary gendisk first, then converting the foreign queue data back, so probe failures clean up both resources before returning an error.

Quoted 6 lines
Fixes 3253aba3408aa ("rust: block: introduce `kernel::block::mq` module")
Quoted 9 lines

Thanks for reporting and fixing!

I see we also lack a put_disk in GenDisk::drop after del_gendisk. Do you want to patch that as well?

Best regards,
Andreas Hindborg

Hi Andreas,

Thanks for the review and for pointing this out.

We will run some additional experiments around the GenDisk::drop() teardown path first. If it does turn out to be a real issue, we will send a follow-up patch.

Best regards,
Haoze