rust: enable `clippy::undocumented_unsafe_blocks` lint

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.

Miguel Ojeda committed 2 years ago

Commit db4f72c1 parent 567cdff16 files · +47 −10Browse files

Unified · Split

16 files

Makefile +1 −0
mm/kasan/kasan_test_rust.rs +1 −0
rust/bindings/lib.rs +1 −0
rust/kernel/alloc/allocator.rs +2 −0
rust/kernel/error.rs +6 −3
rust/kernel/init.rs +5 −0
rust/kernel/init/__internal.rs +2 −0
rust/kernel/init/macros.rs +9 −0
rust/kernel/list.rs +1 −0
rust/kernel/print.rs +2 −0
rust/kernel/str.rs +4 −3
rust/kernel/sync/condvar.rs +1 −1
rust/kernel/sync/lock.rs +3 −3
rust/kernel/types.rs +4 −0
rust/kernel/workqueue.rs +4 −0
rust/uapi/lib.rs +1 −0

Makefile+1−0

@@ -457,6 +457,7 @@ export rust_common_flags := --edition=2021 \ · 456 unmodified lines
457	-Wclippy::needless_bitwise_bool \	457	-Wclippy::needless_bitwise_bool \
458	-Wclippy::needless_continue \	458	-Wclippy::needless_continue \
459	-Wclippy::no_mangle_with_rust_abi \	459	-Wclippy::no_mangle_with_rust_abi \
		460	-Wclippy::undocumented_unsafe_blocks \
460	-Wrustdoc::missing_crate_level_docs	461	-Wrustdoc::missing_crate_level_docs
461		462
462	KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) \	463	KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) \

mm/kasan/kasan_test_rust.rs+1−0

@@ -17,5 +17,6 @@ pub extern "C" fn kasan_test_rust_uaf() -> u8 { · 16 unmodified lines
17	}	17	}
18	let ptr: *mut u8 = addr_of_mut!(v[2048]);	18	let ptr: *mut u8 = addr_of_mut!(v[2048]);
19	drop(v);	19	drop(v);
		20	// SAFETY: Incorrect, on purpose.
20	unsafe { *ptr }	21	unsafe { *ptr }
21	}	22	}

rust/bindings/lib.rs+1−0

@@ -25,6 +25,7 @@ · 24 unmodified lines
25	)]	25	)]
26		26
27	#[allow(dead_code)]	27	#[allow(dead_code)]
		28	#[allow(clippy::undocumented_unsafe_blocks)]
28	mod bindings_raw {	29	mod bindings_raw {
29	// Use glob import here to expose all helpers.	30	// Use glob import here to expose all helpers.
30	// Symbols defined within the module will take precedence to the glob import.	31	// Symbols defined within the module will take precedence to the glob import.

rust/kernel/alloc/allocator.rs+2−0

     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
     }
     unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
+        // SAFETY: TODO.
         unsafe {
             bindings::kfree(ptr as *const core::ffi::c_void);
         }

rust/kernel/error.rs+6−3

         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(),
         }
     }
     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`,
         //
         // 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)

rust/kernel/init.rs+5−0

 /// }
 /// pin_init!(&this in Buf {
 ///     buf: [0; 64],
+///     // SAFETY: TODO.
 ///     ptr: unsafe { addr_of_mut!((*this.as_ptr()).buf).cast() },
 ///     pin: PhantomPinned,
 /// });
     /// }
     ///
     /// let foo = pin_init!(Foo {
+    ///     // SAFETY: TODO.
     ///     raw <- unsafe {
     ///         Opaque::ffi_init(|s| {
     ///             init_foo(s);
 // 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(())
     }
 // 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) }
     }
 }
 macro_rules! impl_zeroable {
     ($($({$($generics:tt)*})? $t:ty, )*) => {
+        // SAFETY: Safety comments written in the macro invocation.
         $(unsafe impl$($($generics)*)? Zeroable for $t {})*
     };
 }

rust/kernel/init/__internal.rs+2−0

 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>;

rust/kernel/init/macros.rs+9−0

             }
         ),
     ) => {
+        // SAFETY: TODO.
         unsafe $($impl_sig)* {
             // Inherit all attributes and the type/ident tokens for the signature.
             $(#[$($attr)*])*
                 }
             }
+            // SAFETY: TODO.
             unsafe impl<$($impl_generics)*>
                 $crate::init::__internal::PinData for __ThePinData<$($ty_generics)*>
             where $($whr)*
                     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) }
                 }
             )*
                     slot: *mut $type,
                     init: impl $crate::init::Init<$type, E>,
                 ) -> ::core::result::Result<(), E> {
+                    // SAFETY: TODO.
                     unsafe { $crate::init::Init::__init(init, slot) }
                 }
             )*
         // 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
         let init = move |slot| -> ::core::result::Result<(), $err> {
             init(slot).map(|__InitOk| ())
         };
+        // SAFETY: TODO.
         let init = unsafe { $crate::init::$construct_closure::<_, $err>(init) };
         init
     }};
         // 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

rust/kernel/list.rs+1−0

@@ -354,6 +354,7 @@ impl<T: ?Sized + ListItem<ID>, const ID: u64> List<T, ID> { · 353 unmodified lines
354	///	354	///
355	/// `item` must not be in a different linked list (with the same id).	355	/// `item` must not be in a different linked list (with the same id).
356	pub unsafe fn remove(&mut self, item: &T) -> Option<ListArc<T, ID>> {	356	pub unsafe fn remove(&mut self, item: &T) -> Option<ListArc<T, ID>> {
		357	// SAFETY: TODO.
357	let mut item = unsafe { ListLinks::fields(T::view_links(item)) };	358	let mut item = unsafe { ListLinks::fields(T::view_links(item)) };
358	// SAFETY: The user provided a reference, and reference are never dangling.	359	// SAFETY: The user provided a reference, and reference are never dangling.
359	//	360	//

rust/kernel/print.rs+2−0

     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()
 }
 ) {
     // `_printk` does not seem to fail in any path.
     #[cfg(CONFIG_PRINTK)]
+    // SAFETY: TODO.
     unsafe {
         bindings::_printk(
             format_string.as_ptr() as _,

rust/kernel/str.rs+4−3

     /// 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()
     /// ```
     #[inline]
     pub unsafe fn as_str_unchecked(&self) -> &str {
+        // SAFETY: TODO.
         unsafe { core::str::from_utf8_unchecked(self.as_bytes()) }
     }

rust/kernel/sync/condvar.rs+1−1

@@ -92,8 +92,8 @@ pub struct CondVar { · 91 unmodified lines
92	_pin: PhantomPinned,	92	_pin: PhantomPinned,
93	}	93	}
94		94
95	// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on any thread.
96	#[allow(clippy::non_send_fields_in_send_ty)]	95	#[allow(clippy::non_send_fields_in_send_ty)]
		96	// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on any thread.
97	unsafe impl Send for CondVar {}	97	unsafe impl Send for CondVar {}
98		98
99	// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on multiple threads	99	// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on multiple threads

rust/kernel/sync/lock.rs+3−3

@@ -150,9 +150,9 @@ impl<T: ?Sized, B: Backend> Guard<'_, T, B> { · 149 unmodified lines
150	// SAFETY: The caller owns the lock, so it is safe to unlock it.	150	// SAFETY: The caller owns the lock, so it is safe to unlock it.
151	unsafe { B::unlock(self.lock.state.get(), &self.state) };	151	unsafe { B::unlock(self.lock.state.get(), &self.state) };
152		152
153	// SAFETY: The lock was just unlocked above and is being relocked now.	153	let _relock = ScopeGuard::new(\|\|
154	let _relock =	154	// SAFETY: The lock was just unlocked above and is being relocked now.
155	ScopeGuard::new(\|\| unsafe { B::relock(self.lock.state.get(), &mut self.state) });	155	unsafe { B::relock(self.lock.state.get(), &mut self.state) });
156		156
157	cb()	157	cb()
158	}	158	}

rust/kernel/types.rs+4−0

     ///
     /// struct Empty {}
     ///
+    /// # // SAFETY: TODO.
     /// unsafe impl AlwaysRefCounted for Empty {
     ///     fn inc_ref(&self) {}
     ///     unsafe fn dec_ref(_obj: NonNull<Self>) {}
     ///
     /// 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);
     ///
 macro_rules! impl_frombytes {
     ($($({$($generics:tt)*})? $t:ty, )*) => {
+        // SAFETY: Safety comments written in the macro invocation.
         $(unsafe impl$($($generics)*)? FromBytes for $t {})*
     };
 }
 macro_rules! impl_asbytes {
     ($($({$($generics:tt)*})? $t:ty, )*) => {
+        // SAFETY: Safety comments written in the macro invocation.
         $(unsafe impl$($($generics)*)? AsBytes for $t {})*
     };
 }

rust/kernel/workqueue.rs+4−0

     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>,
     }
 }
+// SAFETY: TODO.
 unsafe impl<T, const ID: u64> RawWorkItem<ID> for Arc<T>
 where
     T: WorkItem<ID, Pointer = Self>,
     }
 }
+// SAFETY: TODO.
 unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<Box<T>>
 where
     T: WorkItem<ID, Pointer = Self>,
     }
 }
+// SAFETY: TODO.
 unsafe impl<T, const ID: u64> RawWorkItem<ID> for Pin<Box<T>>
 where
     T: WorkItem<ID, Pointer = Self>,

rust/uapi/lib.rs+1−0

@@ -14,6 +14,7 @@ · 13 unmodified lines
14	#![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))]	14	#![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))]
15	#![allow(	15	#![allow(
16	clippy::all,	16	clippy::all,
		17	clippy::undocumented_unsafe_blocks,
17	dead_code,	18	dead_code,
18	missing_docs,	19	missing_docs,
19	non_camel_case_types,	20	non_camel_case_types,