[PATCH v3] rust: block: set GenDisk block_device_operations.owner to THIS_MODULE
Adarsh Das <[email protected]> Thu, 6 Aug 2026 16:10:27 +0530
| Newsgroups | org.kernel.vger.linux-block,org.kernel.vger.linux-kernel,org.kernel.vger.rust-for-linux |
|---|---|
| Message-ID | <[email protected]> |
GenDiskBuilder left block_device_operations.owner NULL. Set owner from a const block_device_operations table using this_module::<M>() and update rnull to pass NullBlkModule. Add ModuleMetadata::THIS_MODULE and this_module() so the owner pointer is available in const context. Rebased on Alvins fix-fops-owner series [1]. Link: https://lore.kernel.org/all/[email protected]/ v2: - Free fops in free_disk instead of GenDisk::drop to fix use-after-free when the device stays open after removal. (Sashiko) - Install the cleanup guard before fops allocation to avoid leaking gendisk on -ENOMEM. (Sashiko) - Link to v1: https://lore.kernel.org/all/[email protected]/ v3: - Use const fops with this_module::<M>() instead of heap allocation (as suggested by Andreas). - Integrate ModuleMetadata::THIS_MODULE from [1]. - Dismiss recover_data before fallible build steps after the cleanup guard. (Sashiko) - Link to v2: https://lore.kernel.org/all/[email protected]/ [1] https://lore.kernel.org/r/[email protected] Signed-off-by: Adarsh Das <[email protected]> --- drivers/block/rnull/rnull.rs | 2 +- rust/kernel/block/mq.rs | 13 ++++++-- rust/kernel/block/mq/gen_disk.rs | 56 +++++++++++++++++--------------- rust/kernel/lib.rs | 9 +++++ rust/macros/module.rs | 16 +++++++++ 5 files changed, 65 insertions(+), 31 deletions(-) diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs index 0ca8715febe8..912526be4ec2 100644 --- a/drivers/block/rnull/rnull.rs +++ b/drivers/block/rnull/rnull.rs @@ -61,7 +61,7 @@ fn new( .logical_block_size(block_size)? .physical_block_size(block_size)? .rotational(rotational) - .build(fmt!("{}", name.to_str()?), tagset, queue_data) + .build::<NullBlkModule, Self>(fmt!("{}", name.to_str()?), tagset, queue_data) } } diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs index 1fd0d54dd549..faa9fcbac935 100644 --- a/rust/kernel/block/mq.rs +++ b/rust/kernel/block/mq.rs @@ -8,8 +8,8 @@ //! - Implement [`Operations`] for a type `T`. //! - Create a [`TagSet<T>`]. //! - Create a [`GenDisk<T>`], via the [`GenDiskBuilder`]. -//! - Add the disk to the system by calling [`GenDiskBuilder::build`] passing in -//! the `TagSet` reference. +//! - Add the disk to the system by calling [`GenDiskBuilder::build`], passing in +//! the module type, the disk name, the `TagSet`, and queue data. //! //! The types available in this module that have direct C counterparts are: //! @@ -86,9 +86,16 @@ //! //! let tagset: Arc<TagSet<MyBlkDevice>> = //! Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?; +//! # struct MyModule; +//! # impl kernel::ModuleMetadata for MyModule { +//! # const NAME: &'static kernel::str::CStr = c"myblk"; +//! # // SAFETY: Doctest stub; no module is loaded. +//! # const THIS_MODULE: ThisModule = +//! # unsafe { ThisModule::from_ptr(core::ptr::null_mut()) }; +//! # } //! let mut disk = gen_disk::GenDiskBuilder::new() //! .capacity_sectors(4096) -//! .build(fmt!("myblk"), tagset, ())?; +//! .build::<MyModule, MyBlkDevice>(fmt!("myblk"), tagset, ())?; //! //! # Ok::<(), kernel::error::Error>(()) //! ``` diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs index fc97dd873974..4a503f183cc9 100644 --- a/rust/kernel/block/mq/gen_disk.rs +++ b/rust/kernel/block/mq/gen_disk.rs @@ -15,8 +15,34 @@ str::NullTerminatedFormatter, sync::Arc, types::{ForeignOwnable, ScopeGuard}, + ModuleMetadata, }; +struct BlockFops<M: ModuleMetadata>(core::marker::PhantomData<M>); + +impl<M: ModuleMetadata> BlockFops<M> { + const TABLE: bindings::block_device_operations = bindings::block_device_operations { + submit_bio: None, + open: None, + release: None, + ioctl: None, + compat_ioctl: None, + check_events: None, + unlock_native_capacity: None, + getgeo: None, + set_read_only: None, + swap_slot_free_notify: None, + report_zones: None, + devnode: None, + alternative_gpt_sector: None, + get_unique_id: None, + owner: crate::this_module::<M>().as_ptr(), + pr_ops: core::ptr::null_mut(), + free_disk: None, + poll_bio: None, + }; +} + /// A builder for [`GenDisk`]. /// /// Use this struct to configure and add new [`GenDisk`] to the VFS. @@ -95,7 +121,7 @@ pub fn capacity_sectors(mut self, capacity: u64) -> Self { } /// Build a new `GenDisk` and add it to the VFS. - pub fn build<T: Operations>( + pub fn build<M: ModuleMetadata, T: Operations>( self, name: fmt::Arguments<'_>, tagset: Arc<TagSet<T>>, @@ -125,30 +151,8 @@ pub fn build<T: Operations>( ) })?; - const TABLE: bindings::block_device_operations = bindings::block_device_operations { - submit_bio: None, - open: None, - release: None, - ioctl: None, - compat_ioctl: None, - check_events: None, - unlock_native_capacity: None, - getgeo: None, - set_read_only: None, - swap_slot_free_notify: None, - report_zones: None, - devnode: None, - alternative_gpt_sector: None, - get_unique_id: None, - // TODO: Set to `THIS_MODULE`. - owner: core::ptr::null_mut(), - pr_ops: core::ptr::null_mut(), - free_disk: None, - poll_bio: None, - }; - - // SAFETY: `gendisk` is a valid pointer as we initialized it above - unsafe { (*gendisk).fops = &TABLE }; + // SAFETY: `gendisk` is a valid pointer as we initialized it above. + unsafe { (*gendisk).fops = &BlockFops::<M>::TABLE }; let cleanup_failure = ScopeGuard::new_with_data((gendisk, data), |(gendisk, data)| { // SAFETY: `gendisk` came from `__blk_mq_alloc_disk()` above and @@ -159,8 +163,6 @@ pub fn build<T: Operations>( 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( diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 9512af7156df..9bbabaf84e20 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -185,6 +185,15 @@ fn init(module: &'static ThisModule) -> impl pin_init::PinInit<Self, error::Erro pub trait ModuleMetadata { /// The name of the module as specified in the `module!` macro. const NAME: &'static crate::str::CStr; + + /// The module's `THIS_MODULE` pointer. + const THIS_MODULE: ThisModule; +} + +/// Returns the [`ThisModule`] pointer for the given module type. +#[inline] +pub const fn this_module<M: ModuleMetadata>() -> &'static ThisModule { + &M::THIS_MODULE } /// Equivalent to `THIS_MODULE` in the C API. diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 06c18e207508..8df38718224b 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -519,6 +519,22 @@ pub(crate) fn module(info: ModuleInfo) -> Result<TokenStream> { impl ::kernel::ModuleMetadata for #type_ { const NAME: &'static ::kernel::str::CStr = #name_cstr; + + #[cfg(MODULE)] + const THIS_MODULE: ::kernel::ThisModule = { + extern "C" { + static __this_module: ::kernel::types::Opaque<::kernel::bindings::module>; + } + + // SAFETY: `__this_module` is constructed by the kernel at load time and lives + // until the module is unloaded. + unsafe { ::kernel::ThisModule::from_ptr(__this_module.get()) } + }; + + #[cfg(not(MODULE))] + const THIS_MODULE: ::kernel::ThisModule = unsafe { + ::kernel::ThisModule::from_ptr(::core::ptr::null_mut()) + }; } // Double nested modules, since then nobody can access the public items inside. -- 2.55.0