From 3754849fde7f66c3598770ce4a778252f77d0d07 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sat, 15 Jul 2023 23:19:33 -0400 Subject: [PATCH 1/7] rust: move 'rust_common_flags' to a separate configuration file Currently, 'rustc_common_flags' lives in the top-level 'Makefile'. This means that we need to touch this whenever adjustments to Rust's CLI configuration are desired, such as changing what lints Clippy should emit. This patch moves these flags to a separate file within 'rust/' so that necessary changes can be made without affecting 'Makefile'. It copies the behavior currently used with 'bindgen_parameters', which accepts comments. Additionally, 'let_unit_value` is no longer specifically named as it is now part of the 'style' group [1]. [1]: https://github.com/rust-lang/rust-clippy/pull/8563 Signed-off-by: Trevor Gross --- Makefile | 14 +------------- rust/rust_common_flags | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 13 deletions(-) create mode 100644 rust/rust_common_flags diff --git a/Makefile b/Makefile index 836643eaefee0e..45e028837af185 100644 --- a/Makefile +++ b/Makefile @@ -454,19 +454,7 @@ KBUILD_USERLDFLAGS := $(USERLDFLAGS) # These flags apply to all Rust code in the tree, including the kernel and # host programs. -export rust_common_flags := --edition=2021 \ - -Zbinary_dep_depinfo=y \ - -Dunsafe_op_in_unsafe_fn -Drust_2018_idioms \ - -Dunreachable_pub -Dnon_ascii_idents \ - -Wmissing_docs \ - -Drustdoc::missing_crate_level_docs \ - -Dclippy::correctness -Dclippy::style \ - -Dclippy::suspicious -Dclippy::complexity \ - -Dclippy::perf \ - -Dclippy::let_unit_value -Dclippy::mut_mut \ - -Dclippy::needless_bitwise_bool \ - -Dclippy::needless_continue \ - -Wclippy::dbg_macro +export rust_common_flags := $(shell grep -v '^#\|^$$' $(srctree)/rust/rust_common_flags) KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) $(HOSTCFLAGS) KBUILD_HOSTCXXFLAGS := -Wall -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS) diff --git a/rust/rust_common_flags b/rust/rust_common_flags new file mode 100644 index 00000000000000..e15432e986d0cc --- /dev/null +++ b/rust/rust_common_flags @@ -0,0 +1,32 @@ +# SPDX-License-Identifier: GPL-2.0 +# Parameters passed to rust tools (`rustc`, `clippy`, `rustdoc`) whenever they +# are run. + +# Standard configuration +--edition=2021 +-Zbinary_dep_depinfo=y + +# Standard rust lints +-Dnon_ascii_idents +-Drust_2018_idioms +-Dunreachable_pub +-Dunsafe_op_in_unsafe_fn + +# Documentation lints +-Wmissing_docs +-Drustdoc::missing_crate_level_docs + +# Clippy groups +-Dclippy::complexity +-Dclippy::correctness +-Dclippy::perf +-Dclippy::style +-Dclippy::suspicious + +# Clippy lints from pedantic +-Dclippy::mut_mut +-Dclippy::needless_bitwise_bool +-Dclippy::needless_continue + +# Clippy lints that we only warn on +-Wclippy::dbg_macro From 82157b8a04c999c4334ffc77332bd121f67e5820 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sun, 16 Jul 2023 02:10:10 -0400 Subject: [PATCH 2/7] rust: clippy: enable lints from 'pedantic' that have no errors This patch selects a subset of lints from Clippy's 'pedantic' group and enables them. The only lints enabled here are those that do not show any errors with any of the files we currently have. Signed-off-by: Trevor Gross --- rust/rust_common_flags | 52 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/rust/rust_common_flags b/rust/rust_common_flags index e15432e986d0cc..2c8d67ddb95a3f 100644 --- a/rust/rust_common_flags +++ b/rust/rust_common_flags @@ -24,9 +24,61 @@ -Dclippy::suspicious # Clippy lints from pedantic +-Dclippy::bool_to_int_with_if +-Dclippy::cast_precision_loss +-Dclippy::cast_ptr_alignment +-Dclippy::cast_sign_loss +-Dclippy::checked_conversions +-Dclippy::cloned_instead_of_copied +-Dclippy::copy_iterator +-Dclippy::doc_link_with_quotes +-Dclippy::doc_markdown +-Dclippy::empty_enum +-Dclippy::explicit_into_iter_loop +-Dclippy::explicit_iter_loop +-Dclippy::filter_map_next +-Dclippy::flat_map_option +-Dclippy::float_cmp +-Dclippy::from_iter_instead_of_collect +-Dclippy::if_not_else +-Dclippy::implicit_clone +-Dclippy::inconsistent_struct_constructor +-Dclippy::invalid_upcast_comparisons +-Dclippy::iter_not_returning_iterator +-Dclippy::large_types_passed_by_value +-Dclippy::macro_use_imports +-Dclippy::manual_let_else +-Dclippy::manual_ok_or +-Dclippy::match_bool +-Dclippy::match_wildcard_for_single_variants +-Dclippy::maybe_infinite_iter +-Dclippy::mismatching_type_param_order -Dclippy::mut_mut -Dclippy::needless_bitwise_bool -Dclippy::needless_continue +-Dclippy::needless_for_each +-Dclippy::no_effect_underscore_binding +-Dclippy::option_option +-Dclippy::range_minus_one +-Dclippy::range_plus_one +-Dclippy::ref_binding_to_reference +-Dclippy::ref_option_ref +-Dclippy::return_self_not_must_use +-Dclippy::same_functions_in_if_condition +-Dclippy::stable_sort_primitive +-Dclippy::too_many_lines +-Dclippy::unicode_not_nfc +-Dclippy::unnecessary_join +-Dclippy::unnested_or_patterns + +# Clippy lints from restriction +-Dclippy::default_union_representation +-Dclippy::disallowed_script_idents +-Dclippy::float_cmp_const +-Dclippy::lossy_float_literal +-Dclippy::semicolon_outside_block +-Dclippy::unnecessary_safety_doc # Clippy lints that we only warn on -Wclippy::dbg_macro +-Wclippy::todo From bbc6d1c4545bdfd5a7cf0f0c6baeb2d6364cd873 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sun, 16 Jul 2023 03:40:59 -0400 Subject: [PATCH 3/7] rust: clippy: enable 'undocumented_unsafe_blocks' lint This patch enables the 'undocumented_unsafe_blocks' lint and adds comments where needed for this lint to pass. Clippy will now deny unsafe code that does not have an associated '// SAFETY: ' comment, which helps improve the maintainability of potentially dangerous code. Signed-off-by: Trevor Gross --- rust/bindings/lib.rs | 1 + rust/kernel/allocator.rs | 32 ++++++++++++++++++-------------- rust/kernel/error.rs | 13 ++++++++----- rust/kernel/init.rs | 22 ++++++++++++++++++---- rust/kernel/init/__internal.rs | 2 ++ rust/kernel/init/macros.rs | 3 +++ rust/kernel/print.rs | 27 +++++++++++++++++---------- rust/kernel/str.rs | 7 ++++--- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 6 +++--- rust/rust_common_flags | 4 ++++ rust/uapi/lib.rs | 1 + 12 files changed, 80 insertions(+), 40 deletions(-) diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs index 9bcbea04dac305..2e9ee32abd7dc6 100644 --- a/rust/bindings/lib.rs +++ b/rust/bindings/lib.rs @@ -15,6 +15,7 @@ #![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))] #![allow( clippy::all, + clippy::undocumented_unsafe_blocks, missing_docs, non_camel_case_types, non_upper_case_globals, diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs index 397a3dd57a9b13..5775cca29063cf 100644 --- a/rust/kernel/allocator.rs +++ b/rust/kernel/allocator.rs @@ -9,17 +9,22 @@ use crate::bindings; struct KernelAllocator; +// SAFETY: this implementation meets `GlobalAlloc` safety invariants: +// - `kalloc` does not unwind +// - `kalloc` has no stricter safety requirements than those of `GlobalAlloc` itself +// - Allocating has no further side effects unsafe impl GlobalAlloc for KernelAllocator { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { // `krealloc()` is used instead of `kmalloc()` because the latter is // an inline function and cannot be bound to as a result. - unsafe { bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL) as *mut u8 } + // SAFETY: `krealloc` is a FFI call with no invariants to meet + unsafe { bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL).cast() } } unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) { - unsafe { - bindings::kfree(ptr as *const core::ffi::c_void); - } + // SAFETY: `dealloc` has the invariant that `ptr` was allocated by this + // allocator. `kfree` has no additional invariants. + unsafe { bindings::kfree(ptr.cast()) }; } } @@ -33,32 +38,31 @@ static ALLOCATOR: KernelAllocator = KernelAllocator; // Note that `#[no_mangle]` implies exported too, nowadays. #[no_mangle] fn __rust_alloc(size: usize, _align: usize) -> *mut u8 { - unsafe { bindings::krealloc(core::ptr::null(), size, bindings::GFP_KERNEL) as *mut u8 } + // SAFETY: `krealloc` is a FFI call with no invariants to meet + unsafe { bindings::krealloc(core::ptr::null(), size, bindings::GFP_KERNEL).cast() } } #[no_mangle] fn __rust_dealloc(ptr: *mut u8, _size: usize, _align: usize) { - unsafe { bindings::kfree(ptr as *const core::ffi::c_void) }; + // SAFETY: `ptr` only ever comes from `krealloc` via `__rust_alloc` + unsafe { bindings::kfree(ptr.cast()) }; } #[no_mangle] fn __rust_realloc(ptr: *mut u8, _old_size: usize, _align: usize, new_size: usize) -> *mut u8 { - unsafe { - bindings::krealloc( - ptr as *const core::ffi::c_void, - new_size, - bindings::GFP_KERNEL, - ) as *mut u8 - } + // SAFETY: `ptr` only ever comes from `krealloc` via `__rust_alloc` + unsafe { bindings::krealloc(ptr.cast(), new_size, bindings::GFP_KERNEL).cast() } } #[no_mangle] fn __rust_alloc_zeroed(size: usize, _align: usize) -> *mut u8 { + // SAFETY: `krealloc` can handle zero-sized allocations with `__GFP_ZERO` unsafe { bindings::krealloc( core::ptr::null(), size, bindings::GFP_KERNEL | bindings::__GFP_ZERO, - ) as *mut u8 + ) + .cast() } } diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 05fcab6abfe63e..f951f20697996f 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -166,9 +166,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(), } } @@ -281,7 +283,7 @@ pub(crate) fn from_err_ptr(ptr: *mut T) -> Result<*mut T> { // SAFETY: The FFI function does not deref the pointer. if unsafe { bindings::IS_ERR(const_ptr) } { // SAFETY: The FFI function does not deref the pointer. - let err = unsafe { bindings::PTR_ERR(const_ptr) }; + let errno = unsafe { bindings::PTR_ERR(const_ptr) }; // CAST: If `IS_ERR()` returns `true`, // then `PTR_ERR()` is guaranteed to return a // negative value greater-or-equal to `-bindings::MAX_ERRNO`, @@ -289,10 +291,11 @@ pub(crate) fn from_err_ptr(ptr: *mut T) -> Result<*mut T> { // And an `i16` always fits in an `i32`. So casting `err` to // an `i32` can never overflow, and is always valid. // + #[allow(clippy::unnecessary_cast)] // 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) }); + let err = unsafe { Error::from_errno_unchecked(errno as core::ffi::c_int) }; + return Err(err); } Ok(ptr) } diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs index b4332a4ec1f4d5..04c2b956003b14 100644 --- a/rust/kernel/init.rs +++ b/rust/kernel/init.rs @@ -621,6 +621,7 @@ macro_rules! try_pin_init { // no possibility of returning without `unsafe`. struct __InitOk; // Get the pin data from the supplied type. + // SAFETY: TODO let data = unsafe { use $crate::init::__internal::HasPinData; $t$(::<$($generics),*>)?::__pin_data() @@ -664,6 +665,7 @@ macro_rules! try_pin_init { let init = move |slot| -> ::core::result::Result<(), $err> { init(slot).map(|__InitOk| ()) }; + // SAFETY: TODO let init = unsafe { $crate::init::pin_init_from_closure::<_, $err>(init) }; init }}; @@ -735,8 +737,9 @@ macro_rules! try_pin_init { @acc($($acc:tt)*), ) => { // Endpoint, nothing more to munch, create the initializer. - // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to - // get the correct type inference here: + + // SAFETY: Since we are in the `if false` branch, this will never get executed. We abuse + // `slot` to get the correct type inference here: unsafe { ::core::ptr::write($slot, $t { $($acc)* @@ -777,6 +780,7 @@ macro_rules! try_pin_init { (forget_guards: @munch_fields($field:ident <- $val:expr, $($rest:tt)*), ) => { + // SAFETY: forgetting the guard is intentional unsafe { $crate::init::__internal::DropGuard::forget($field) }; $crate::try_pin_init!(forget_guards: @@ -786,6 +790,7 @@ macro_rules! try_pin_init { (forget_guards: @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*), ) => { + // SAFETY: forgetting the guard is intentional unsafe { $crate::init::__internal::DropGuard::forget($field) }; $crate::try_pin_init!(forget_guards: @@ -891,6 +896,7 @@ macro_rules! try_init { // no possibility of returning without `unsafe`. struct __InitOk; // Get the init data from the supplied type. + // SAFETY: `HasInitData` is a marker trait which we are allowed to use in this context let data = unsafe { use $crate::init::__internal::HasInitData; $t$(::<$($generics),*>)?::__init_data() @@ -933,6 +939,7 @@ macro_rules! try_init { let init = move |slot| -> ::core::result::Result<(), $err> { init(slot).map(|__InitOk| ()) }; + // SAFETY: TODO let init = unsafe { $crate::init::init_from_closure::<_, $err>(init) }; init }}; @@ -999,8 +1006,8 @@ macro_rules! try_init { @acc($($acc:tt)*), ) => { // Endpoint, nothing more to munch, create the initializer. - // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to - // get the correct type inference here: + // SAFETY: Since we are in the `if false` branch, this will never get + // executed. We abuse `slot` to get the correct type inference here: unsafe { ::core::ptr::write($slot, $t { $($acc)* @@ -1041,6 +1048,7 @@ macro_rules! try_init { (forget_guards: @munch_fields($field:ident <- $val:expr, $($rest:tt)*), ) => { + // SAFETY: forgetting the guard is intentional unsafe { $crate::init::__internal::DropGuard::forget($field) }; $crate::try_init!(forget_guards: @@ -1050,6 +1058,7 @@ macro_rules! try_init { (forget_guards: @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*), ) => { + // SAFETY: forgetting the guard is intentional unsafe { $crate::init::__internal::DropGuard::forget($field) }; $crate::try_init!(forget_guards: @@ -1197,6 +1206,7 @@ pub fn uninit() -> impl Init, E> { // SAFETY: Every type can be initialized by-value. unsafe impl Init for T { unsafe fn __init(self, slot: *mut T) -> Result<(), E> { + // SAFETY: `slot` is valid per the invariants of `__init` unsafe { slot.write(self) }; Ok(()) } @@ -1371,8 +1381,12 @@ pub fn zeroed() -> impl Init { } } +/// # Safety +/// +/// This can only be used on types that meet `Zeroable`'s guidelines macro_rules! impl_zeroable { ($($({$($generics:tt)*})? $t:ty, )*) => { + // SAFETY: upheld by documented use $(unsafe impl$($($generics)*)? Zeroable for $t {})* }; } diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs index 44751fb62b51a2..0543303941c2a1 100644 --- a/rust/kernel/init/__internal.rs +++ b/rust/kernel/init/__internal.rs @@ -7,6 +7,8 @@ //! - `macros/pin_data.rs` //! - `macros/pinned_drop.rs` +#![allow(clippy::undocumented_unsafe_blocks)] + use super::*; /// See the [nomicon] for what subtyping is. See also [this table]. diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs index 00aa4e956c0ae9..46822574871dd4 100644 --- a/rust/kernel/init/macros.rs +++ b/rust/kernel/init/macros.rs @@ -841,6 +841,7 @@ macro_rules! __pin_data { } } + // SAFETY: `__ThePinData` has the correct projections unsafe impl<$($impl_generics)*> $crate::init::__internal::PinData for __ThePinData<$($ty_generics)*> where $($whr)* @@ -965,6 +966,7 @@ macro_rules! __pin_data { slot: *mut $p_type, init: impl $crate::init::PinInit<$p_type, E>, ) -> ::core::result::Result<(), E> { + // SAFETY: `slot` is valid per this function's contract unsafe { $crate::init::PinInit::__pinned_init(init, slot) } } )* @@ -974,6 +976,7 @@ macro_rules! __pin_data { slot: *mut $type, init: impl $crate::init::Init<$type, E>, ) -> ::core::result::Result<(), E> { + // SAFETY: `slot` is valid per this function's contract unsafe { $crate::init::Init::__init(init, slot) } } )* diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index 8009184bf6d768..bd47a71cf2a59c 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -9,6 +9,7 @@ use core::{ ffi::{c_char, c_void}, fmt, + ptr::addr_of, }; use crate::str::RawFormatter; @@ -16,7 +17,12 @@ use crate::str::RawFormatter; #[cfg(CONFIG_PRINTK)] use crate::bindings; -// Called from `vsprintf` with format specifier `%pA`. +/// Called from `vsprintf` with format specifier `%pA`. +/// +/// # Safety +/// +/// - Memory within `buf..end` must be valid for write +/// - `ptr` must point to a valid instance of `core::fmt::Arguments` #[no_mangle] unsafe extern "C" fn rust_fmt_argument( buf: *mut c_char, @@ -26,7 +32,8 @@ 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()) }; - let _ = w.write_fmt(unsafe { *(ptr as *const fmt::Arguments<'_>) }); + // SAFETY: Upheld by this function's safety contract + let _ = w.write_fmt(unsafe { *(ptr.cast::>()) }); w.pos().cast() } @@ -105,13 +112,14 @@ pub unsafe fn call_printk( module_name: &[u8], args: fmt::Arguments<'_>, ) { - // `_printk` does not seem to fail in any path. #[cfg(CONFIG_PRINTK)] + // `_printk` does not seem to fail in any path. + // SAFETY: This function's invariants make this call safe. unsafe { bindings::_printk( - format_string.as_ptr() as _, + format_string.as_ptr().cast(), module_name.as_ptr(), - &args as *const _ as *const c_void, + addr_of!(args).cast::(), ); } } @@ -124,14 +132,13 @@ pub unsafe fn call_printk( #[doc(hidden)] #[cfg_attr(not(CONFIG_PRINTK), allow(unused_variables))] pub fn call_printk_cont(args: fmt::Arguments<'_>) { - // `_printk` does not seem to fail in any path. - // - // SAFETY: The format string is fixed. #[cfg(CONFIG_PRINTK)] + // `_printk` does not seem to fail in any path. + // SAFETY: `CONT` and `args` meet `_printk`'s safety contract unsafe { bindings::_printk( - format_strings::CONT.as_ptr() as _, - &args as *const _ as *const c_void, + format_strings::CONT.as_ptr().cast(), + addr_of!(args).cast::(), ); } } diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index c9dd3bf59e34c6..4330e87311fd21 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -72,10 +72,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() @@ -198,6 +198,7 @@ impl CStr { /// ``` #[inline] pub unsafe fn as_str_unchecked(&self) -> &str { + // SAFETY: invariant is upheld by this function's safety contract unsafe { core::str::from_utf8_unchecked(self.as_bytes()) } } diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs index ed353399c4e56f..bd7c8b8959528c 100644 --- a/rust/kernel/sync/condvar.rs +++ b/rust/kernel/sync/condvar.rs @@ -81,8 +81,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 a2216325632dff..406aab47a3931a 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -144,9 +144,9 @@ impl Guard<'_, T, B> { // 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/rust_common_flags b/rust/rust_common_flags index 2c8d67ddb95a3f..1309769bfec81a 100644 --- a/rust/rust_common_flags +++ b/rust/rust_common_flags @@ -67,8 +67,12 @@ -Dclippy::same_functions_in_if_condition -Dclippy::stable_sort_primitive -Dclippy::too_many_lines +-Dclippy::undocumented_unsafe_blocks -Dclippy::unicode_not_nfc -Dclippy::unnecessary_join +# FIXME: enable this at the next version bump. Disabled because of false +# positive in macros: https://github.com/rust-lang/rust-clippy/issues/10084 +# -Dclippy::unnecessary_safety_comment -Dclippy::unnested_or_patterns # Clippy lints from restriction diff --git a/rust/uapi/lib.rs b/rust/uapi/lib.rs index 0caad902ba40a2..6da1af3e50b638 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, missing_docs, non_camel_case_types, non_upper_case_globals, From 8f4700dc36f134da4018b71054225715bf1659e6 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sun, 16 Jul 2023 04:05:54 -0400 Subject: [PATCH 4/7] rust: clippy: enable three trivial lints with corrections This patch enables three trivial lints and adds corrections so existing code passes. Lints added were: - 'expl_impl_clone_on_copy': check for implementing `Clone` when `Copy` already exists. - 'explicit_deref_methods': check for usage of `.deref()` to encourage cleaner code. - 'trivially_copy_pass_by_ref': check for small types that would be easier passed by value than by reference. Signed-off-by: Trevor Gross --- rust/kernel/init/macros.rs | 1 + rust/kernel/print.rs | 1 + rust/kernel/sync/arc.rs | 12 ++++++------ rust/rust_common_flags | 3 +++ 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs index 46822574871dd4..e47bca019ada79 100644 --- a/rust/kernel/init/macros.rs +++ b/rust/kernel/init/macros.rs @@ -808,6 +808,7 @@ macro_rules! __pin_data { >, } + #[allow(clippy::expl_impl_clone_on_copy)] impl<$($impl_generics)*> ::core::clone::Clone for __ThePinData<$($ty_generics)*> where $($whr)* { diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index bd47a71cf2a59c..c7b546d4d8b89c 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -56,6 +56,7 @@ pub mod format_strings { /// given `prefix`, which are the kernel's `KERN_*` constants. /// /// [`_printk`]: ../../../../include/linux/printk.h + #[allow(clippy::trivially_copy_pass_by_ref)] const fn generate(is_cont: bool, prefix: &[u8; 3]) -> [u8; LENGTH] { // Ensure the `KERN_*` macros are what we expect. assert!(prefix[0] == b'\x01'); diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index a89843cacaad07..e1ed07fbdc34f2 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -268,7 +268,7 @@ impl Deref for Arc { impl AsRef for Arc { fn as_ref(&self) -> &T { - self.deref() + self } } @@ -595,7 +595,7 @@ impl Deref for UniqueArc { type Target = T; fn deref(&self) -> &Self::Target { - self.inner.deref() + &self.inner } } @@ -610,24 +610,24 @@ impl DerefMut for UniqueArc { impl fmt::Display for UniqueArc { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Display::fmt(self.deref(), f) + fmt::Display::fmt(&self, f) } } impl fmt::Display for Arc { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Display::fmt(self.deref(), f) + fmt::Display::fmt(&self, f) } } impl fmt::Debug for UniqueArc { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Debug::fmt(self.deref(), f) + fmt::Debug::fmt(&self, f) } } impl fmt::Debug for Arc { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Debug::fmt(self.deref(), f) + fmt::Debug::fmt(&self, f) } } diff --git a/rust/rust_common_flags b/rust/rust_common_flags index 1309769bfec81a..3ae3494a4c392c 100644 --- a/rust/rust_common_flags +++ b/rust/rust_common_flags @@ -34,6 +34,8 @@ -Dclippy::doc_link_with_quotes -Dclippy::doc_markdown -Dclippy::empty_enum +-Dclippy::expl_impl_clone_on_copy +-Dclippy::explicit_deref_methods -Dclippy::explicit_into_iter_loop -Dclippy::explicit_iter_loop -Dclippy::filter_map_next @@ -67,6 +69,7 @@ -Dclippy::same_functions_in_if_condition -Dclippy::stable_sort_primitive -Dclippy::too_many_lines +-Dclippy::trivially_copy_pass_by_ref -Dclippy::undocumented_unsafe_blocks -Dclippy::unicode_not_nfc -Dclippy::unnecessary_join From a764f0a12c2d7b55fe6a4189c4155b8e35cdc629 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sun, 16 Jul 2023 04:17:37 -0400 Subject: [PATCH 5/7] rust: clippy: enable two pointer-related lints - 'ptr_as_ptr': enforce that '*const -> *const' and '*mut -> *mut' conversions be done via '.cast()' rather than 'as', to avoid accidental changes in mutability. - 'transmute_ptr_to_ptr': detect transumtes that could be written as lessi dangerous casts. Signed-off-by: Trevor Gross --- rust/bindings/lib.rs | 2 ++ rust/kernel/error.rs | 2 +- rust/kernel/str.rs | 4 ++-- rust/rust_common_flags | 4 ++++ 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs index 2e9ee32abd7dc6..9eaaabf4e74d78 100644 --- a/rust/bindings/lib.rs +++ b/rust/bindings/lib.rs @@ -15,6 +15,8 @@ #![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))] #![allow( clippy::all, + clippy::ptr_as_ptr, + clippy::transmute_ptr_to_ptr, clippy::undocumented_unsafe_blocks, missing_docs, non_camel_case_types, diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index f951f20697996f..653b6b3dff793d 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -134,7 +134,7 @@ impl Error { #[allow(dead_code)] pub(crate) fn to_ptr(self) -> *mut T { // SAFETY: self.0 is a valid error due to its invariant. - unsafe { bindings::ERR_PTR(self.0.into()) as *mut _ } + unsafe { bindings::ERR_PTR(self.0.into()).cast() } } /// Returns a string representing the error, if one exists. diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 4330e87311fd21..c6cbd8dfa5423e 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -100,7 +100,7 @@ impl CStr { // to a `NUL`-terminated C string. let len = unsafe { bindings::strlen(ptr) } + 1; // SAFETY: Lifetime guaranteed by the safety precondition. - let bytes = unsafe { core::slice::from_raw_parts(ptr as _, len as _) }; + let bytes = unsafe { core::slice::from_raw_parts(ptr.cast(), len as _) }; // SAFETY: As `len` is returned by `strlen`, `bytes` does not contain interior `NUL`. // As we have added 1 to `len`, the last byte is known to be `NUL`. unsafe { Self::from_bytes_with_nul_unchecked(bytes) } @@ -146,7 +146,7 @@ impl CStr { /// Returns a C pointer to the string. #[inline] pub const fn as_char_ptr(&self) -> *const core::ffi::c_char { - self.0.as_ptr() as _ + self.0.as_ptr().cast() } /// Convert the string to a byte slice without the trailing 0 byte. diff --git a/rust/rust_common_flags b/rust/rust_common_flags index 3ae3494a4c392c..50cab39ce5ef91 100644 --- a/rust/rust_common_flags +++ b/rust/rust_common_flags @@ -61,6 +61,9 @@ -Dclippy::needless_for_each -Dclippy::no_effect_underscore_binding -Dclippy::option_option +-Dclippy::ptr_as_ptr +# FIXME: enable this at version bump, not supported in 1.68 +# -Dclippy::ptr_cast_constness -Dclippy::range_minus_one -Dclippy::range_plus_one -Dclippy::ref_binding_to_reference @@ -69,6 +72,7 @@ -Dclippy::same_functions_in_if_condition -Dclippy::stable_sort_primitive -Dclippy::too_many_lines +-Dclippy::transmute_ptr_to_ptr -Dclippy::trivially_copy_pass_by_ref -Dclippy::undocumented_unsafe_blocks -Dclippy::unicode_not_nfc From 2118d15a98ba32519a9667172d6f1751a97c6191 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sun, 16 Jul 2023 04:31:05 -0400 Subject: [PATCH 6/7] rust: clippy: enable lints related to improving style - 'manual_assert': detect combined usage of 'if' and 'panic' when 'assert' would have been more straightforward. - 'redundant_else': enforce that blocks are not nested within 'else' after conditional control flow statments. - 'semicolon_if_nothing_returned': enforce consistent style that makes it more clear that a statement is not used. Signed-off-by: Trevor Gross --- rust/bindings/lib.rs | 1 + rust/kernel/str.rs | 2 +- rust/kernel/sync/condvar.rs | 4 ++-- rust/kernel/sync/lock.rs | 2 +- rust/kernel/sync/locked_by.rs | 8 ++------ rust/kernel/types.rs | 2 +- rust/macros/helpers.rs | 29 ++++++++++++++--------------- rust/macros/module.rs | 34 +++++++++++++++++----------------- rust/rust_common_flags | 3 +++ 9 files changed, 42 insertions(+), 43 deletions(-) diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs index 9eaaabf4e74d78..12a52c12310b22 100644 --- a/rust/bindings/lib.rs +++ b/rust/bindings/lib.rs @@ -16,6 +16,7 @@ #![allow( clippy::all, clippy::ptr_as_ptr, + clippy::semicolon_if_nothing_returned, clippy::transmute_ptr_to_ptr, clippy::undocumented_unsafe_blocks, missing_docs, diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index c6cbd8dfa5423e..96e8c3f2e26bfc 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -471,7 +471,7 @@ impl fmt::Write for RawFormatter { s.as_bytes().as_ptr(), self.pos as *mut u8, len_to_copy, - ) + ); }; } diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs index bd7c8b8959528c..836859b93489e2 100644 --- a/rust/kernel/sync/condvar.rs +++ b/rust/kernel/sync/condvar.rs @@ -98,7 +98,7 @@ impl CondVar { // SAFETY: `slot` is valid while the closure is called and both `name` and `key` have // static lifetimes so they live indefinitely. wait_list <- Opaque::ffi_init(|slot| unsafe { - bindings::__init_waitqueue_head(slot, name.as_char_ptr(), key.as_ptr()) + bindings::__init_waitqueue_head(slot, name.as_char_ptr(), key.as_ptr()); }), }) } @@ -140,7 +140,7 @@ impl CondVar { /// Similar to [`CondVar::wait`], except that the wait is not interruptible. That is, the /// thread won't wake up due to signals. It may, however, wake up supirously. pub fn wait_uninterruptible(&self, guard: &mut Guard<'_, T, B>) { - self.wait_internal(bindings::TASK_UNINTERRUPTIBLE, guard) + self.wait_internal(bindings::TASK_UNINTERRUPTIBLE, guard); } /// Calls the kernel function to notify the appropriate number of threads with the given flags. diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index 406aab47a3931a..6858a8543014de 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -107,7 +107,7 @@ impl Lock { // SAFETY: `slot` is valid while the closure is called and both `name` and `key` have // static lifetimes so they live indefinitely. state <- Opaque::ffi_init(|slot| unsafe { - B::init(slot, name.as_char_ptr(), key.as_ptr()) + B::init(slot, name.as_char_ptr(), key.as_ptr()); }), }) } diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs index b17ee5cd98f3eb..2b2bc03892264a 100644 --- a/rust/kernel/sync/locked_by.rs +++ b/rust/kernel/sync/locked_by.rs @@ -120,9 +120,7 @@ impl LockedBy { size_of::() > 0, "`U` cannot be a ZST because `owner` wouldn't be unique" ); - if !ptr::eq(owner, self.owner) { - panic!("mismatched owners"); - } + assert!(ptr::eq(owner, self.owner), "mismatched owners"); // SAFETY: `owner` is evidence that the owner is locked. unsafe { &*self.data.get() } @@ -146,9 +144,7 @@ impl LockedBy { size_of::() > 0, "`U` cannot be a ZST because `owner` wouldn't be unique" ); - if !ptr::eq(owner, self.owner) { - panic!("mismatched owners"); - } + assert!(ptr::eq(owner, self.owner), "mismatched owners"); // SAFETY: `owner` is evidence that there is only one reference to the owner. unsafe { &mut *self.data.get() } diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index 1e5380b16ed553..977c2a6c287a6c 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -215,7 +215,7 @@ impl Drop for ScopeGuard { fn drop(&mut self) { // Run the cleanup function if one is still present. if let Some((data, cleanup)) = self.0.take() { - cleanup(data) + cleanup(data); } } } diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs index afb0f2e3a36a9d..fa454c7d998020 100644 --- a/rust/macros/helpers.rs +++ b/rust/macros/helpers.rs @@ -22,9 +22,10 @@ pub(crate) fn try_string(it: &mut token_stream::IntoIter) -> Option { try_literal(it).and_then(|string| { if string.starts_with('\"') && string.ends_with('\"') { let content = &string[1..string.len() - 1]; - if content.contains('\\') { - panic!("Escape sequences in string literals not yet handled"); - } + assert!( + !content.contains('\\'), + "Escape sequences in string literals not yet handled" + ); Some(content.to_string()) } else if string.starts_with("r\"") { panic!("Raw string literals are not yet handled"); @@ -65,9 +66,7 @@ pub(crate) fn expect_group(it: &mut token_stream::IntoIter) -> Group { } pub(crate) fn expect_end(it: &mut token_stream::IntoIter) { - if it.next().is_some() { - panic!("Expected end"); - } + assert!(it.next().is_none(), "Expected end"); } pub(crate) struct Generics { @@ -103,15 +102,15 @@ pub(crate) fn parse_generics(input: TokenStream) -> (Generics, Vec) { // This is a parsing error, so we just end it here. if nesting == 0 { break; - } else { - nesting -= 1; - if nesting >= 1 { - // We are still inside of the generics and part of some bound. - impl_generics.push(tt); - } - if nesting == 0 { - break; - } + } + + nesting -= 1; + if nesting >= 1 { + // We are still inside of the generics and part of some bound. + impl_generics.push(tt); + } + if nesting == 0 { + break; } } tt => { diff --git a/rust/macros/module.rs b/rust/macros/module.rs index fb1244f8c2e694..f09eb978575c55 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -76,11 +76,11 @@ impl<'a> ModInfoBuilder<'a> { } fn emit_only_builtin(&mut self, field: &str, content: &str) { - self.emit_base(field, content, true) + self.emit_base(field, content, true); } fn emit_only_loadable(&mut self, field: &str, content: &str) { - self.emit_base(field, content, false) + self.emit_base(field, content, false); } fn emit(&mut self, field: &str, content: &str) { @@ -115,12 +115,11 @@ impl ModuleInfo { None => break, }; - if seen_keys.contains(&key) { - panic!( - "Duplicated key \"{}\". Keys can only be specified once.", - key - ); - } + assert!( + !seen_keys.contains(&key), + "Duplicated key \"{}\". Keys can only be specified once.", + key + ); assert_eq!(expect_punct(it), ':'); @@ -145,9 +144,11 @@ impl ModuleInfo { expect_end(it); for key in REQUIRED_KEYS { - if !seen_keys.iter().any(|e| e == key) { - panic!("Missing required key \"{}\".", key); - } + assert!( + seen_keys.iter().any(|e| e == key), + "Missing required key \"{}\".", + key + ); } let mut ordered_keys: Vec<&str> = Vec::new(); @@ -157,12 +158,11 @@ impl ModuleInfo { } } - if seen_keys != ordered_keys { - panic!( - "Keys are not ordered as expected. Order them like: {:?}.", - ordered_keys - ); - } + assert_eq!( + seen_keys, ordered_keys, + "Keys are not ordered as expected. Order them like: {:?}.", + ordered_keys + ); info } diff --git a/rust/rust_common_flags b/rust/rust_common_flags index 50cab39ce5ef91..b4c3b9328d649e 100644 --- a/rust/rust_common_flags +++ b/rust/rust_common_flags @@ -49,6 +49,7 @@ -Dclippy::iter_not_returning_iterator -Dclippy::large_types_passed_by_value -Dclippy::macro_use_imports +-Dclippy::manual_assert -Dclippy::manual_let_else -Dclippy::manual_ok_or -Dclippy::match_bool @@ -66,10 +67,12 @@ # -Dclippy::ptr_cast_constness -Dclippy::range_minus_one -Dclippy::range_plus_one +-Dclippy::redundant_else -Dclippy::ref_binding_to_reference -Dclippy::ref_option_ref -Dclippy::return_self_not_must_use -Dclippy::same_functions_in_if_condition +-Dclippy::semicolon_if_nothing_returned -Dclippy::stable_sort_primitive -Dclippy::too_many_lines -Dclippy::transmute_ptr_to_ptr From b2d5dbcbe63464146d5bd04a9c632087d932b3f1 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sun, 16 Jul 2023 04:38:50 -0400 Subject: [PATCH 7/7] rust: clippy: enable 'items_after_statements' and 'redundant_closure_for_...' - 'items_after_statements': prevent confusing scope since items and statements do not follow the same rules. - 'redundant_closure_for_method_calls': simplify closures that call a single method. This is the associated function equivalent of 'redundant_closure'. Signed-off-by: Trevor Gross --- rust/kernel/sync/arc.rs | 5 +++-- rust/macros/module.rs | 4 ++-- rust/rust_common_flags | 2 ++ 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index e1ed07fbdc34f2..6022aabeb2b6b4 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -25,6 +25,7 @@ use crate::{ use alloc::boxed::Box; use core::{ alloc::AllocError, + convert::Into, fmt, marker::{PhantomData, Unsize}, mem::{ManuallyDrop, MaybeUninit}, @@ -182,7 +183,7 @@ impl Arc { where Error: From, { - UniqueArc::pin_init(init).map(|u| u.into()) + UniqueArc::pin_init(init).map(Into::into) } /// Use the given initializer to in-place initialize a `T`. @@ -193,7 +194,7 @@ impl Arc { where Error: From, { - UniqueArc::init(init).map(|u| u.into()) + UniqueArc::init(init).map(Into::into) } } diff --git a/rust/macros/module.rs b/rust/macros/module.rs index f09eb978575c55..92ee0bba9944f9 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -101,11 +101,11 @@ struct ModuleInfo { impl ModuleInfo { fn parse(it: &mut token_stream::IntoIter) -> Self { - let mut info = ModuleInfo::default(); - const EXPECTED_KEYS: &[&str] = &["type", "name", "author", "description", "license", "alias"]; const REQUIRED_KEYS: &[&str] = &["type", "name", "license"]; + + let mut info = ModuleInfo::default(); let mut seen_keys = Vec::new(); loop { diff --git a/rust/rust_common_flags b/rust/rust_common_flags index b4c3b9328d649e..ae59237e232f01 100644 --- a/rust/rust_common_flags +++ b/rust/rust_common_flags @@ -46,6 +46,7 @@ -Dclippy::implicit_clone -Dclippy::inconsistent_struct_constructor -Dclippy::invalid_upcast_comparisons +-Dclippy::items_after_statements -Dclippy::iter_not_returning_iterator -Dclippy::large_types_passed_by_value -Dclippy::macro_use_imports @@ -67,6 +68,7 @@ # -Dclippy::ptr_cast_constness -Dclippy::range_minus_one -Dclippy::range_plus_one +-Dclippy::redundant_closure_for_method_calls -Dclippy::redundant_else -Dclippy::ref_binding_to_reference -Dclippy::ref_option_ref