This series is aimed at fixing a soundness issue with how dynamically allocated LockClassKeys are handled. Currently, LockClassKeys can be used without being Pin'd, which can break lockdep since it relies on address stability. Similarly, these keys are not automatically (de)registered with lockdep.
At the suggestion of Alice Ryhl, this series includes a patch for -stable kernels that disables dynamically allocated keys. This prevents backported patches from using the unsound implementation.
Currently, this series requires that all dynamically allocated LockClassKeys have a lifetime of 'static (i.e., they must be leaked after allocation). This is because Lock does not currently keep a reference to the LockClassKey, instead passing it to C via FFI. This causes a problem because the rust compiler would allow creating a 'static Lock with a 'a LockClassKey (with 'a < 'static) while C would expect the LockClassKey to live as long as the lock. This problem represents an avenue for future work.
--- Changes from RFC: - Split into two commits so that dynamically allocated LockClassKeys are removed from stable kernels. (Thanks Alice Ryhl) - Extract calls to C lockdep functions into helpers so things build properly when LOCKDEP=n. (Thanks Benno Lossin) - Remove extraneous `get_ref()` calls. (Thanks Benno Lossin) - Provide better documentation for `new_dynamic()`. (Thanks Benno Lossin) - Ran rustfmt to fix formatting and some extraneous changes. (Thanks Alice Ryhl and Benno Lossin) - Link to RFC: https://lore.kernel.org/r/20240905-rust-lockdep-v1-1-d2c9c21aa8b2@gmail.com
--- Changes in v2: - Dropped formatting change that's already fixed upstream (Thanks Dirk Behme). - Moved safety comment to the right point in the patch series (Thanks Dirk Behme and Boqun Feng). - Added an example of dynamic LockClassKey usage (Thanks Boqun Feng). - Link to v1: https://lore.kernel.org/r/20241004-rust-lockdep-v1-0-e9a5c45721fc@gmail.com
--- Mitchell Levy (2): rust: lockdep: Remove support for dynamically allocated LockClassKeys rust: lockdep: Use Pin for all LockClassKey usages
rust/helpers/helpers.c | 1 + rust/helpers/sync.c | 13 +++++++++ rust/kernel/sync.rs | 63 ++++++++++++++++++++++++++++++++++------- rust/kernel/sync/condvar.rs | 5 ++-- rust/kernel/sync/lock.rs | 9 ++---- rust/kernel/sync/lock/global.rs | 5 ++-- rust/kernel/sync/poll.rs | 2 +- rust/kernel/workqueue.rs | 3 +- 8 files changed, 78 insertions(+), 23 deletions(-) --- base-commit: 0c5928deada15a8d075516e6e0d9ee19011bb000 change-id: 20240905-rust-lockdep-d3e30521c8ba
Best regards,
Currently, dynamically allocated LockCLassKeys can be used from the Rust side without having them registered. This is a soundness issue, so remove them.
Suggested-by: Alice Ryhl aliceryhl@google.com Link: https://lore.kernel.org/rust-for-linux/20240815074519.2684107-3-nmi@metaspac... Cc: stable@vger.kernel.org Signed-off-by: Mitchell Levy levymitchell0@gmail.com --- rust/kernel/sync.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index 1eab7ebf25fd..ae16bfd98de2 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -29,28 +29,20 @@ unsafe impl Sync for LockClassKey {}
impl LockClassKey { - /// Creates a new lock class key. - pub const fn new() -> Self { - Self(Opaque::uninit()) - } - pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key { self.0.get() } }
-impl Default for LockClassKey { - fn default() -> Self { - Self::new() - } -} - /// Defines a new static lock class and returns a pointer to it. #[doc(hidden)] #[macro_export] macro_rules! static_lock_class { () => {{ - static CLASS: $crate::sync::LockClassKey = $crate::sync::LockClassKey::new(); + // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated + // lock_class_key + static CLASS: $crate::sync::LockClassKey = + unsafe { ::core::mem::MaybeUninit::uninit().assume_init() }; &CLASS }}; }
Hi Mitchell,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 0c5928deada15a8d075516e6e0d9ee19011bb000]
url: https://github.com/intel-lab-lkp/linux/commits/Mitchell-Levy/rust-lockdep-Re... base: 0c5928deada15a8d075516e6e0d9ee19011bb000 patch link: https://lore.kernel.org/r/20241219-rust-lockdep-v2-1-f65308fbc5ca%40gmail.co... patch subject: [PATCH v2 1/2] rust: lockdep: Remove support for dynamically allocated LockClassKeys config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20241225/202412251433.T3BhO2CQ-lkp@i...) compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241225/202412251433.T3BhO2CQ-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202412251433.T3BhO2CQ-lkp@intel.com/
All warnings (new ones prefixed by >>):
warning: unsafe block missing a safety comment
--> rust/kernel/sync.rs:45:13 | 45 | unsafe { ::core::mem::MaybeUninit::uninit().assume_init() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ::: rust/kernel/block/mq/gen_disk.rs:111:17 | 111 | static_lock_class!().as_ptr(), | -------------------- in this macro invocation | = help: consider adding a safety comment on the preceding line = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsaf... = note: requested on the command line with `-W clippy::undocumented-unsafe-blocks` = note: this warning originates in the macro `static_lock_class` (in Nightly builds, run with -Z macro-backtrace for more info) --
warning: unsafe block missing a safety comment
--> rust/kernel/sync.rs:45:13 | 45 | unsafe { ::core::mem::MaybeUninit::uninit().assume_init() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ::: rust/kernel/workqueue.rs:218:21 | 218 | work <- new_work!("Queue::try_spawn"), | ----------------------------- in this macro invocation | = help: consider adding a safety comment on the preceding line = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsaf... = note: this warning originates in the macro `$crate::static_lock_class` which comes from the expansion of the macro `new_work` (in Nightly builds, run with -Z macro-backtrace for more info)
On Thu, Dec 19, 2024 at 12:58:55PM -0800, Mitchell Levy wrote:
About the clippy warning reported by 0day, I think you could resolve that by moving the above safety comment here.
Regards, Boqun
On Thu, Dec 19, 2024 at 12:58:54PM -0800, Mitchell Levy wrote:
Thanks for doing this! I found some clippy warnings with the current version, but overall it looks good to me. That said, appreciate it if patch #2 gets more reviews on the interface changes, thanks!
Regards, Boqun
linux-stable-mirror@lists.linaro.org