Re: [RFC PATCH 1/5] doc: rust: create safety standard
From: Miguel Ojeda
Date: Fri Jul 19 2024 - 13:28:30 EST
On Fri, Jul 19, 2024 at 6:24 PM Daniel Almeida
<daniel.almeida@xxxxxxxxxxxxx> wrote:
>
> A new clippy lint would make sense here, since we already have clippy support
> in the kernel anyways.
There is one already, which we want to enable.
Here is a quick patch (untested!) of how it could look like, in case
one wants to fill the TODOs, or we can just merge it as-is and clean
it up later to avoid adding new ones.
Cheers,
Miguel
From dd8c4746fd1bd95f2e88b02405d913d7c63c2d3a Mon Sep 17 00:00:00 2001
From: Miguel Ojeda <ojeda@kernel.org>
Date: Fri, 19 Jul 2024 19:19:07 +0200
Subject: [PATCH] rust: enable `clippy::undocumented_unsafe_blocks`
Untested, please double-check.
---
Makefile | 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/print.rs | 2 +
rust/kernel/str.rs | 3 +-
rust/kernel/sync/condvar.rs | 2 +-
rust/kernel/sync/lock.rs | 6 +--
rust/kernel/types.rs | 70 ++++++++++++++++++----------------
rust/kernel/workqueue.rs | 4 ++
rust/uapi/lib.rs | 1 +
14 files changed, 76 insertions(+), 41 deletions(-)
diff --git a/Makefile b/Makefile
index 9044fdb9adb1..e792fd59413a 100644
--- a/Makefile
+++ b/Makefile
@@ -472,6 +472,7 @@ export rust_common_flags := --edition=2021 \
-Wclippy::needless_bitwise_bool \
-Wclippy::needless_continue \
-Wclippy::no_mangle_with_rust_abi \
+ -Wclippy::undocumented_unsafe_blocks \
-Wclippy::dbg_macro
KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) $(HOSTCFLAGS)
diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs
index 93a1a3fc97bc..d6da3011281a 100644
--- 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.
diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
index 229642960cd1..de85ec0aab32 100644
--- a/rust/kernel/alloc/allocator.rs
+++ b/rust/kernel/alloc/allocator.rs
@@ -38,6 +38,7 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
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
@@ -46,6 +47,7 @@ unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
}
unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
+ // SAFETY: TODO.
unsafe {
bindings::kfree(ptr as *const core::ffi::c_void);
}
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 55280ae9fe40..b31808e70364 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -162,9 +162,11 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
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(),
}
}
@@ -268,6 +270,8 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
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`,
@@ -277,7 +281,6 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
//
// 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)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 495c09ebe3a3..75982961d006 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -540,6 +540,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,
/// });
@@ -806,6 +807,7 @@ pub unsafe trait PinInit<T: ?Sized, E = Infallible>: Sized {
/// }
///
/// let foo = pin_init!(Foo {
+ /// // SAFETY: TODO.
/// raw <- unsafe {
/// Opaque::ffi_init(|s| {
/// init_foo(s);
@@ -1093,6 +1095,7 @@ pub fn pin_init_array_from_fn<I, const N: usize, T, E>(
// 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(())
}
@@ -1101,6 +1104,7 @@ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
// 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) }
}
}
@@ -1276,6 +1280,7 @@ pub fn zeroed<T: Zeroable>() -> impl Init<T> {
macro_rules! impl_zeroable {
($($({$($generics:tt)*})? $t:ty, )*) => {
+ // SAFETY: Safety comments written in the macro invocation.
$(unsafe impl$($($generics)*)? Zeroable for $t {})*
};
}
diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs
index db3372619ecd..e0b03ed971ab 100644
--- a/rust/kernel/init/__internal.rs
+++ b/rust/kernel/init/__internal.rs
@@ -112,10 +112,12 @@ fn clone(&self) -> Self {
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>;
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index 02ecedc4ae7a..93af8f3d8d4d 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -513,6 +513,7 @@ fn drop($($sig:tt)*) {
}
),
) => {
+ // SAFETY: TODO.
unsafe $($impl_sig)* {
// Inherit all attributes and the type/ident tokens for the signature.
$(#[$($attr)*])*
@@ -872,6 +873,7 @@ unsafe fn __pin_data() -> Self::PinData {
}
}
+ // SAFETY: TODO.
unsafe impl<$($impl_generics)*>
$crate::init::__internal::PinData for __ThePinData<$($ty_generics)*>
where $($whr)*
@@ -997,6 +999,7 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
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 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
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 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
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 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
// 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
diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index a78aa3514a0a..3c2a87a751b5 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -23,6 +23,7 @@
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 _,
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index bb8d4f41475b..428ffdbe7e72 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -162,10 +162,10 @@ pub const fn len(&self) -> usize {
/// 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`.
unsafe { core::hint::unreachable_unchecked() };
}
self.0.len()
@@ -301,6 +301,7 @@ pub fn to_str(&self) -> Result<&str, core::str::Utf8Error> {
/// ```
#[inline]
pub unsafe fn as_str_unchecked(&self) -> &str {
+ // SAFETY: TODO.
unsafe { core::str::from_utf8_unchecked(self.as_bytes()) }
}
diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index 2b306afbe56d..7e00048bf4b1 100644
--- 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
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index f6c34ca4d819..07fcf2d8efc6 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -150,9 +150,9 @@ pub(crate) fn do_unlocked<U>(&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) };
- // 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()
}
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index bd189d646adb..8a8fc981f0b4 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -422,21 +422,23 @@ pub enum Either<L, R> {
/// All bit-patterns must be valid for this type. This type must not have interior mutability.
pub unsafe trait FromBytes {}
-// SAFETY: All bit patterns are acceptable values of the types below.
-unsafe impl FromBytes for u8 {}
-unsafe impl FromBytes for u16 {}
-unsafe impl FromBytes for u32 {}
-unsafe impl FromBytes for u64 {}
-unsafe impl FromBytes for usize {}
-unsafe impl FromBytes for i8 {}
-unsafe impl FromBytes for i16 {}
-unsafe impl FromBytes for i32 {}
-unsafe impl FromBytes for i64 {}
-unsafe impl FromBytes for isize {}
-// SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
-// patterns are also acceptable for arrays of that type.
-unsafe impl<T: FromBytes> FromBytes for [T] {}
-unsafe impl<T: FromBytes, const N: usize> FromBytes for [T; N] {}
+macro_rules! impl_frombytes {
+ ($($({$($generics:tt)*})? $t:ty, )*) => {
+ // SAFETY: Safety comments written in the macro invocation.
+ $(unsafe impl$($($generics)*)? FromBytes for $t {})*
+ };
+}
+
+impl_frombytes! {
+ // SAFETY: All bit patterns are acceptable values of the types below.
+ u8, u16, u32, u64, usize,
+ i8, i16, i32, i64, isize,
+
+ // SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
+ // patterns are also acceptable for arrays of that type.
+ {<T: FromBytes>} [T],
+ {<T: FromBytes, const N: usize>} [T; N],
+}
/// Types that can be viewed as an immutable slice of initialized bytes.
///
@@ -455,21 +457,23 @@ unsafe impl<T: FromBytes> FromBytes for [T] {}
/// mutability.
pub unsafe trait AsBytes {}
-// SAFETY: Instances of the following types have no uninitialized portions.
-unsafe impl AsBytes for u8 {}
-unsafe impl AsBytes for u16 {}
-unsafe impl AsBytes for u32 {}
-unsafe impl AsBytes for u64 {}
-unsafe impl AsBytes for usize {}
-unsafe impl AsBytes for i8 {}
-unsafe impl AsBytes for i16 {}
-unsafe impl AsBytes for i32 {}
-unsafe impl AsBytes for i64 {}
-unsafe impl AsBytes for isize {}
-unsafe impl AsBytes for bool {}
-unsafe impl AsBytes for char {}
-unsafe impl AsBytes for str {}
-// SAFETY: If individual values in an array have no uninitialized portions, then the array itself
-// does not have any uninitialized portions either.
-unsafe impl<T: AsBytes> AsBytes for [T] {}
-unsafe impl<T: AsBytes, const N: usize> AsBytes for [T; N] {}
+macro_rules! impl_asbytes {
+ ($($({$($generics:tt)*})? $t:ty, )*) => {
+ // SAFETY: Safety comments written in the macro invocation.
+ $(unsafe impl$($($generics)*)? AsBytes for $t {})*
+ };
+}
+
+impl_asbytes! {
+ // SAFETY: Instances of the following types have no uninitialized portions.
+ u8, u16, u32, u64, usize,
+ i8, i16, i32, i64, isize,
+ bool,
+ char,
+ str,
+
+ // SAFETY: If individual values in an array have no uninitialized portions, then the array itself
+ // does not have any uninitialized portions either.
+ {<T: AsBytes>} [T],
+ {<T: AsBytes, const N: usize>} [T; N],
+}
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 553a5cba2adc..e828d0b9a73c 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -520,6 +520,7 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_typ
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>,
@@ -537,6 +538,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Arc<T>
}
}
+// SAFETY: TODO.
unsafe impl<T, const ID: u64> RawWorkItem<ID> for Arc<T>
where
T: WorkItem<ID, Pointer = Self>,
@@ -565,6 +567,7 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
}
}
+// SAFETY: TODO.
unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<Box<T>>
where
T: WorkItem<ID, Pointer = Self>,
@@ -584,6 +587,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<Box<T>>
}
}
+// SAFETY: TODO.
unsafe impl<T, const ID: u64> RawWorkItem<ID> for Pin<Box<T>>
where
T: WorkItem<ID, Pointer = Self>,
diff --git a/rust/uapi/lib.rs b/rust/uapi/lib.rs
index 80a00260e3e7..fea2de330d19 100644
--- a/rust/uapi/lib.rs
+++ b/rust/uapi/lib.rs
@@ -14,6 +14,7 @@
#![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))]
#![allow(
clippy::all,
+ clippy::undocumented_unsafe_blocks,
dead_code,
missing_docs,
non_camel_case_types,
base-commit: b1263411112305acf2af728728591465becb45b0
--
2.45.2