From: Miguel Ojeda <ojeda@xxxxxxxxxx> commit db4f72c904cb116e2bf56afdd67fc5167a607a7b upstream. Checking that we are not missing any `// SAFETY` comments in our `unsafe` blocks is something we have wanted to do for a long time, as well as cleaning up the remaining cases that were not documented [1]. Back when Rust for Linux started, this was something that could have been done via a script, like Rust's `tidy`. Soon after, in Rust 1.58.0, Clippy implemented the `undocumented_unsafe_blocks` lint [2]. Even though the lint has a few false positives, e.g. in some cases where attributes appear between the comment and the `unsafe` block [3], there are workarounds and the lint seems quite usable already. Thus enable the lint now. We still have a few cases to clean up, so just allow those for the moment by writing a `TODO` comment -- some of those may be good candidates for new contributors. Link: https://github.com/Rust-for-Linux/linux/issues/351 [1] Link: https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks [2] Link: https://github.com/rust-lang/rust-clippy/issues/13189 [3] Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> Reviewed-by: Trevor Gross <tmgross@xxxxxxxxx> Tested-by: Gary Guo <gary@xxxxxxxxxxx> Reviewed-by: Gary Guo <gary@xxxxxxxxxxx> Link: https://lore.kernel.org/r/20240904204347.168520-5-ojeda@xxxxxxxxxx Signed-off-by: Miguel Ojeda <ojeda@xxxxxxxxxx> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> --- Makefile | 1 + mm/kasan/kasan_test_rust.rs | 1 + rust/bindings/lib.rs | 1 + rust/kernel/alloc/allocator.rs | 2 ++ rust/kernel/error.rs | 9 ++++++--- rust/kernel/init.rs | 5 +++++ rust/kernel/init/__internal.rs | 2 ++ rust/kernel/init/macros.rs | 9 +++++++++ rust/kernel/list.rs | 1 + rust/kernel/print.rs | 2 ++ rust/kernel/str.rs | 7 ++++--- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 6 +++--- rust/kernel/types.rs | 4 ++++ rust/kernel/workqueue.rs | 4 ++++ rust/uapi/lib.rs | 1 + 16 files changed, 47 insertions(+), 10 deletions(-) --- a/Makefile +++ b/Makefile @@ -458,6 +458,7 @@ export rust_common_flags := --edition=20 -Wclippy::needless_continue \ -Aclippy::needless_lifetimes \ -Wclippy::no_mangle_with_rust_abi \ + -Wclippy::undocumented_unsafe_blocks \ -Wrustdoc::missing_crate_level_docs KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) \ --- a/mm/kasan/kasan_test_rust.rs +++ b/mm/kasan/kasan_test_rust.rs @@ -17,5 +17,6 @@ pub extern "C" fn kasan_test_rust_uaf() } let ptr: *mut u8 = addr_of_mut!(v[2048]); drop(v); + // SAFETY: Incorrect, on purpose. unsafe { *ptr } } --- a/rust/bindings/lib.rs +++ b/rust/bindings/lib.rs @@ -25,6 +25,7 @@ )] #[allow(dead_code)] +#[allow(clippy::undocumented_unsafe_blocks)] mod bindings_raw { // Use glob import here to expose all helpers. // Symbols defined within the module will take precedence to the glob import. --- a/rust/kernel/alloc/allocator.rs +++ b/rust/kernel/alloc/allocator.rs @@ -31,6 +31,7 @@ pub(crate) unsafe fn krealloc_aligned(pt unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 } } +// SAFETY: TODO. unsafe impl GlobalAlloc for KernelAllocator { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety @@ -39,6 +40,7 @@ unsafe impl GlobalAlloc for KernelAlloca } unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) { + // SAFETY: TODO. unsafe { bindings::kfree(ptr as *const core::ffi::c_void); } --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -171,9 +171,11 @@ impl fmt::Debug for Error { match self.name() { // Print out number if no name can be found. None => f.debug_tuple("Error").field(&-self.0).finish(), - // SAFETY: These strings are ASCII-only. Some(name) => f - .debug_tuple(unsafe { core::str::from_utf8_unchecked(name) }) + .debug_tuple( + // SAFETY: These strings are ASCII-only. + unsafe { core::str::from_utf8_unchecked(name) }, + ) .finish(), } } @@ -277,6 +279,8 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut if unsafe { bindings::IS_ERR(const_ptr) } { // SAFETY: The FFI function does not deref the pointer. let err = unsafe { bindings::PTR_ERR(const_ptr) }; + + #[allow(clippy::unnecessary_cast)] // CAST: If `IS_ERR()` returns `true`, // then `PTR_ERR()` is guaranteed to return a // negative value greater-or-equal to `-bindings::MAX_ERRNO`, @@ -286,7 +290,6 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut // // SAFETY: `IS_ERR()` ensures `err` is a // negative value greater-or-equal to `-bindings::MAX_ERRNO`. - #[allow(clippy::unnecessary_cast)] return Err(unsafe { Error::from_errno_unchecked(err as core::ffi::c_int) }); } Ok(ptr) --- a/rust/kernel/init.rs +++ b/rust/kernel/init.rs @@ -541,6 +541,7 @@ macro_rules! stack_try_pin_init { /// } /// pin_init!(&this in Buf { /// buf: [0; 64], +/// // SAFETY: TODO. /// ptr: unsafe { addr_of_mut!((*this.as_ptr()).buf).cast() }, /// pin: PhantomPinned, /// }); @@ -875,6 +876,7 @@ pub unsafe trait PinInit<T: ?Sized, E = /// } /// /// let foo = pin_init!(Foo { + /// // SAFETY: TODO. /// raw <- unsafe { /// Opaque::ffi_init(|s| { /// init_foo(s); @@ -1162,6 +1164,7 @@ where // SAFETY: Every type can be initialized by-value. unsafe impl<T, E> Init<T, E> for T { unsafe fn __init(self, slot: *mut T) -> Result<(), E> { + // SAFETY: TODO. unsafe { slot.write(self) }; Ok(()) } @@ -1170,6 +1173,7 @@ unsafe impl<T, E> Init<T, E> for T { // SAFETY: Every type can be initialized by-value. `__pinned_init` calls `__init`. unsafe impl<T, E> PinInit<T, E> for T { unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> { + // SAFETY: TODO. unsafe { self.__init(slot) } } } @@ -1411,6 +1415,7 @@ pub fn zeroed<T: Zeroable>() -> impl Ini macro_rules! impl_zeroable { ($($({$($generics:tt)*})? $t:ty, )*) => { + // SAFETY: Safety comments written in the macro invocation. $(unsafe impl$($($generics)*)? Zeroable for $t {})* }; } --- a/rust/kernel/init/__internal.rs +++ b/rust/kernel/init/__internal.rs @@ -112,10 +112,12 @@ impl<T: ?Sized> Clone for AllData<T> { impl<T: ?Sized> Copy for AllData<T> {} +// SAFETY: TODO. unsafe impl<T: ?Sized> InitData for AllData<T> { type Datee = T; } +// SAFETY: TODO. unsafe impl<T: ?Sized> HasInitData for T { type InitData = AllData<T>; --- a/rust/kernel/init/macros.rs +++ b/rust/kernel/init/macros.rs @@ -513,6 +513,7 @@ macro_rules! __pinned_drop { } ), ) => { + // SAFETY: TODO. unsafe $($impl_sig)* { // Inherit all attributes and the type/ident tokens for the signature. $(#[$($attr)*])* @@ -872,6 +873,7 @@ macro_rules! __pin_data { } } + // SAFETY: TODO. unsafe impl<$($impl_generics)*> $crate::init::__internal::PinData for __ThePinData<$($ty_generics)*> where $($whr)* @@ -997,6 +999,7 @@ macro_rules! __pin_data { slot: *mut $p_type, init: impl $crate::init::PinInit<$p_type, E>, ) -> ::core::result::Result<(), E> { + // SAFETY: TODO. unsafe { $crate::init::PinInit::__pinned_init(init, slot) } } )* @@ -1007,6 +1010,7 @@ macro_rules! __pin_data { slot: *mut $type, init: impl $crate::init::Init<$type, E>, ) -> ::core::result::Result<(), E> { + // SAFETY: TODO. unsafe { $crate::init::Init::__init(init, slot) } } )* @@ -1121,6 +1125,8 @@ macro_rules! __init_internal { // no possibility of returning without `unsafe`. struct __InitOk; // Get the data about fields from the supplied type. + // + // SAFETY: TODO. let data = unsafe { use $crate::init::__internal::$has_data; // Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal @@ -1176,6 +1182,7 @@ macro_rules! __init_internal { let init = move |slot| -> ::core::result::Result<(), $err> { init(slot).map(|__InitOk| ()) }; + // SAFETY: TODO. let init = unsafe { $crate::init::$construct_closure::<_, $err>(init) }; init }}; @@ -1324,6 +1331,8 @@ macro_rules! __init_internal { // Endpoint, nothing more to munch, create the initializer. // Since we are in the closure that is never called, this will never get executed. // We abuse `slot` to get the correct type inference here: + // + // SAFETY: TODO. unsafe { // Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal // information that is associated to already parsed fragments, so a path fragment --- a/rust/kernel/list.rs +++ b/rust/kernel/list.rs @@ -354,6 +354,7 @@ impl<T: ?Sized + ListItem<ID>, const ID: /// /// `item` must not be in a different linked list (with the same id). pub unsafe fn remove(&mut self, item: &T) -> Option<ListArc<T, ID>> { + // SAFETY: TODO. let mut item = unsafe { ListLinks::fields(T::view_links(item)) }; // SAFETY: The user provided a reference, and reference are never dangling. // --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -23,6 +23,7 @@ unsafe extern "C" fn rust_fmt_argument( use fmt::Write; // SAFETY: The C contract guarantees that `buf` is valid if it's less than `end`. let mut w = unsafe { RawFormatter::from_ptrs(buf.cast(), end.cast()) }; + // SAFETY: TODO. let _ = w.write_fmt(unsafe { *(ptr as *const fmt::Arguments<'_>) }); w.pos().cast() } @@ -102,6 +103,7 @@ pub unsafe fn call_printk( ) { // `_printk` does not seem to fail in any path. #[cfg(CONFIG_PRINTK)] + // SAFETY: TODO. unsafe { bindings::_printk( format_string.as_ptr() as _, --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -162,10 +162,10 @@ impl CStr { /// Returns the length of this string with `NUL`. #[inline] pub const fn len_with_nul(&self) -> usize { - // SAFETY: This is one of the invariant of `CStr`. - // We add a `unreachable_unchecked` here to hint the optimizer that - // the value returned from this function is non-zero. if self.0.is_empty() { + // SAFETY: This is one of the invariant of `CStr`. + // We add a `unreachable_unchecked` here to hint the optimizer that + // the value returned from this function is non-zero. unsafe { core::hint::unreachable_unchecked() }; } self.0.len() @@ -301,6 +301,7 @@ impl CStr { /// ``` #[inline] pub unsafe fn as_str_unchecked(&self) -> &str { + // SAFETY: TODO. unsafe { core::str::from_utf8_unchecked(self.as_bytes()) } } --- a/rust/kernel/sync/condvar.rs +++ b/rust/kernel/sync/condvar.rs @@ -92,8 +92,8 @@ pub struct CondVar { _pin: PhantomPinned, } -// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on any thread. #[allow(clippy::non_send_fields_in_send_ty)] +// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on any thread. unsafe impl Send for CondVar {} // SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on multiple threads --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -150,9 +150,9 @@ impl<T: ?Sized, B: Backend> Guard<'_, T, // SAFETY: The caller owns the lock, so it is safe to unlock it. unsafe { B::unlock(self.lock.state.get(), &self.state) }; - // SAFETY: The lock was just unlocked above and is being relocked now. - let _relock = - ScopeGuard::new(|| unsafe { B::relock(self.lock.state.get(), &mut self.state) }); + let _relock = ScopeGuard::new(|| + // SAFETY: The lock was just unlocked above and is being relocked now. + unsafe { B::relock(self.lock.state.get(), &mut self.state) }); cb() } --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -410,6 +410,7 @@ impl<T: AlwaysRefCounted> ARef<T> { /// /// struct Empty {} /// + /// # // SAFETY: TODO. /// unsafe impl AlwaysRefCounted for Empty { /// fn inc_ref(&self) {} /// unsafe fn dec_ref(_obj: NonNull<Self>) {} @@ -417,6 +418,7 @@ impl<T: AlwaysRefCounted> ARef<T> { /// /// let mut data = Empty {}; /// let ptr = NonNull::<Empty>::new(&mut data as *mut _).unwrap(); + /// # // SAFETY: TODO. /// let data_ref: ARef<Empty> = unsafe { ARef::from_raw(ptr) }; /// let raw_ptr: NonNull<Empty> = ARef::into_raw(data_ref); /// @@ -483,6 +485,7 @@ pub unsafe trait FromBytes {} macro_rules! impl_frombytes { ($($({$($generics:tt)*})? $t:ty, )*) => { + // SAFETY: Safety comments written in the macro invocation. $(unsafe impl$($($generics)*)? FromBytes for $t {})* }; } @@ -517,6 +520,7 @@ pub unsafe trait AsBytes {} macro_rules! impl_asbytes { ($($({$($generics:tt)*})? $t:ty, )*) => { + // SAFETY: Safety comments written in the macro invocation. $(unsafe impl$($($generics)*)? AsBytes for $t {})* }; } --- a/rust/kernel/workqueue.rs +++ b/rust/kernel/workqueue.rs @@ -519,6 +519,7 @@ impl_has_work! { impl{T} HasWork<Self> for ClosureWork<T> { self.work } } +// SAFETY: TODO. unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Arc<T> where T: WorkItem<ID, Pointer = Self>, @@ -536,6 +537,7 @@ where } } +// SAFETY: TODO. unsafe impl<T, const ID: u64> RawWorkItem<ID> for Arc<T> where T: WorkItem<ID, Pointer = Self>, @@ -564,6 +566,7 @@ where } } +// SAFETY: TODO. unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<Box<T>> where T: WorkItem<ID, Pointer = Self>, @@ -583,6 +586,7 @@ where } } +// SAFETY: TODO. unsafe impl<T, const ID: u64> RawWorkItem<ID> for Pin<Box<T>> where T: WorkItem<ID, Pointer = Self>, --- a/rust/uapi/lib.rs +++ b/rust/uapi/lib.rs @@ -14,6 +14,7 @@ #![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))] #