Add a `DropGuard` type to the Rust kernel memory module for running cleanup code when a scope is left. `DropGuard` runs a `FnOnce` callback when dropped and provides `dismiss()` to take the wrapped value without running the callback. Replace the current `ScopeGuard` users in `gen_disk`, `serdev`, and `sync::lock` with `DropGuard`. Keep `ScopeGuard` since it is still used elsewhere. Add KUnit tests for cleanup on drop and `dismiss()`. Tested with: - `make LLVM=1 -j$(nproc)` - KUnit: 8 tests passed - `make LLVM=1 rustfmtcheck` - `git diff --check` - `checkpatch.pl` - `make LLVM=1 rustdoc` Suggested-by: Gary Guo Link: https://github.com/Rust-for-Linux/linux/issues/1255 Signed-off-by: Mohamed Osama --- rust/kernel/Kconfig.test | 10 +++ rust/kernel/block/mq/gen_disk.rs | 11 +-- rust/kernel/mem.rs | 120 +++++++++++++++++++++++++++++++ rust/kernel/serdev.rs | 8 +-- rust/kernel/sync/lock.rs | 5 +- 5 files changed, 142 insertions(+), 12 deletions(-) diff --git a/rust/kernel/Kconfig.test b/rust/kernel/Kconfig.test index e6a5c7a795f0..011c72f14e2c 100644 --- a/rust/kernel/Kconfig.test +++ b/rust/kernel/Kconfig.test @@ -33,6 +33,16 @@ config RUST_KVEC_KUNIT_TEST If unsure, say N. +config RUST_DROP_GUARD_KUNIT_TEST + bool "KUnit tests for Rust DropGuard API" if !KUNIT_ALL_TESTS + default KUNIT_ALL_TESTS + help + This option enables KUnit tests for the Rust DropGuard API. + These are only for development and testing, not for regular + kernel use cases. + + If unsure, say N. + config RUST_BITMAP_KUNIT_TEST bool "KUnit tests for Rust bitmap API" if !KUNIT_ALL_TESTS default KUNIT_ALL_TESTS diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs index fc97dd873974..d9019fbbb361 100644 --- a/rust/kernel/block/mq/gen_disk.rs +++ b/rust/kernel/block/mq/gen_disk.rs @@ -10,11 +10,12 @@ block::mq::{Operations, TagSet}, error::{self, from_err_ptr, Result}, fmt::{self, Write}, + mem::DropGuard, prelude::*, static_lock_class, str::NullTerminatedFormatter, sync::Arc, - types::{ForeignOwnable, ScopeGuard}, + types::ForeignOwnable, }; /// A builder for [`GenDisk`]. @@ -102,7 +103,7 @@ pub fn build( queue_data: T::QueueData, ) -> Result> { let data = queue_data.into_foreign(); - let recover_data = ScopeGuard::new(|| { + let recover_data = DropGuard::new((), |_| { // SAFETY: T::QueueData was created by the call to `into_foreign()` above drop(unsafe { T::QueueData::from_foreign(data) }); }); @@ -150,7 +151,7 @@ pub fn build( // 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)| { + let cleanup_failure = DropGuard::new((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) }; @@ -161,7 +162,7 @@ pub fn build( // The failure guard now owns both pieces of cleanup; the early guard // must not run on this path anymore. - recover_data.dismiss(); + DropGuard::dismiss(recover_data); let mut writer = NullTerminatedFormatter::new( // SAFETY: `gendisk` points to a valid and initialized instance. We @@ -185,7 +186,7 @@ pub fn build( }, )?; - cleanup_failure.dismiss(); + DropGuard::dismiss(cleanup_failure); // INVARIANT: `gendisk` was initialized above. // INVARIANT: `gendisk` was added to the VFS via `device_add_disk` above. diff --git a/rust/kernel/mem.rs b/rust/kernel/mem.rs index f2d4cdf87d00..f2d1f9bc9337 100644 --- a/rust/kernel/mem.rs +++ b/rust/kernel/mem.rs @@ -4,6 +4,92 @@ use crate::prelude::*; +use core::{ + mem::ManuallyDrop, + ops::{Deref, DerefMut}, +}; + +/// Wraps a value and runs a closure when dropped. +/// +/// This is useful for running cleanup code when leaving a scope. +/// +/// The [`DropGuard::dismiss`] function can be used to take ownership of the wrapped +/// value without running the cleanup function. +#[doc(alias = "ScopeGuard")] +#[doc(alias = "defer")] +pub struct DropGuard +where + F: FnOnce(T), +{ + inner: ManuallyDrop, + f: ManuallyDrop, +} + +impl DropGuard +where + F: FnOnce(T), +{ + /// Creates a new `DropGuard`. + #[inline] + #[must_use] + pub fn new(inner: T, f: F) -> Self { + Self { + inner: ManuallyDrop::new(inner), + f: ManuallyDrop::new(f), + } + } + + /// Consumes the `DropGuard`, returning the wrapped value without + /// running the cleanup function. + #[inline] + pub fn dismiss(guard: Self) -> T { + let mut guard = ManuallyDrop::new(guard); + + // SAFETY: We have taken ownership of the guard and prevent its destructor from running. + let value = unsafe { ManuallyDrop::take(&mut guard.inner) }; + + // SAFETY: We have taken ownership of the guard. + unsafe { ManuallyDrop::drop(&mut guard.f) }; + + value + } +} + +impl Deref for DropGuard +where + F: FnOnce(T), +{ + type Target = T; + + fn deref(&self) -> &T { + &self.inner + } +} + +impl DerefMut for DropGuard +where + F: FnOnce(T), +{ + fn deref_mut(&mut self) -> &mut T { + &mut self.inner + } +} + +impl Drop for DropGuard +where + F: FnOnce(T), +{ + fn drop(&mut self) { + // SAFETY: `DropGuard` is in the process of being dropped. + let inner = unsafe { ManuallyDrop::take(&mut self.inner) }; + + // SAFETY: `DropGuard` is in the process of being dropped. + let f = unsafe { ManuallyDrop::take(&mut self.f) }; + + f(inner); + } +} + /// Transmute between two types. /// /// Use this instead of [`core::mem::transmute`] when it is known that sizes are identical but this @@ -232,3 +318,37 @@ unsafe impl AsReprMut for $signed {} // `usize` is not normalized to particular integer for portability. usize isize, } + +#[cfg(CONFIG_RUST_DROP_GUARD_KUNIT_TEST)] +#[macros::kunit_tests(rust_drop_guard)] +mod tests { + use super::*; + + #[test] + fn test_drop_runs_cleanup() { + let mut cleaned = false; + + { + let _guard = DropGuard::new(42, |value| { + assert_eq!(value, 42); + cleaned = true; + }); + } + + assert!(cleaned); + } + + #[test] + fn test_dismiss_returns_value_without_cleanup() { + let mut cleaned = false; + + let guard = DropGuard::new(42, |_| { + cleaned = true; + }); + + let value = DropGuard::dismiss(guard); + + assert_eq!(value, 42); + assert!(!cleaned); + } +} diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs index 17ca504b7f8d..a400d241c2ad 100644 --- a/rust/kernel/serdev.rs +++ b/rust/kernel/serdev.rs @@ -13,6 +13,7 @@ to_result, VTABLE_DEFAULT_ERROR, // }, + mem::DropGuard, new_mutex, of, prelude::*, @@ -21,10 +22,7 @@ Mutex, // }, time::Jiffies, - types::{ - Opaque, - ScopeGuard, // - }, // + types::Opaque, // }; use core::{ @@ -174,7 +172,7 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi: }))?; // SAFETY: We just set drvdata to `PrivateData<'_, T>`. let private_data = unsafe { sdev.as_ref().drvdata_borrow::>() }; - let private_data = ScopeGuard::new_with_data(private_data, |_| { + let private_data = DropGuard::new(private_data, |_| { // SAFETY: We just set drvdata to `PrivateData<'_, T>`. drop(unsafe { sdev.as_ref().drvdata_obtain::>() }); }); diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index 10b6b5e9b024..15f9cbe76c8d 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -7,8 +7,9 @@ use super::LockClassKey; use crate::{ + mem::DropGuard, str::{CStr, CStrExt as _}, - types::{NotThreadSafe, Opaque, ScopeGuard}, + types::{NotThreadSafe, Opaque}, }; use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin}; use pin_init::{pin_data, pin_init, PinInit, Wrapper}; @@ -242,7 +243,7 @@ pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce() -> U) -> U { // SAFETY: The caller owns the lock, so it is safe to unlock it. unsafe { B::unlock(self.lock.state.get(), &self.state) }; - let _relock = ScopeGuard::new(|| + let _relock = DropGuard::new((), |_| // SAFETY: The lock was just unlocked above and is being relocked now. unsafe { B::relock(self.lock.state.get(), &mut self.state) }); -- 2.43.0