rust: block: fix GenDiskBuilder failure cleanup
Subsystems (3)
A message that is not in the archive (sent off-list or not yet received): [email protected]
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.
--- rust/kernel/block/mq/gen_disk.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
rust/kernel/block/mq/gen_disk.rs +12 -1
@@ -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
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")Reported-by Yuan Tan <[email protected]>Reported-by Xin Liu <[email protected]>Signed-off-by Haoze Xie <[email protected]>Signed-off-by Ren Wei <[email protected]>
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
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")Reported-by Yuan Tan <[email protected]>Reported-by Xin Liu <[email protected]>Signed-off-by Haoze Xie <[email protected]>Signed-off-by Ren Wei <[email protected]>Reviewed-by Andreas Hindborg <[email protected]>
Quoted 9 lines
Thanks for reporting and fixing!
I see we also lack a
put_diskinGenDisk::dropafterdel_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