Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable various new clippy lints #1025

Open
wants to merge 7 commits into
base: rust-next
Choose a base branch
from
Open
14 changes: 1 addition & 13 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions rust/bindings/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
32 changes: 18 additions & 14 deletions rust/kernel/allocator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()) };
}
}

Expand All @@ -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()
}
}
13 changes: 8 additions & 5 deletions rust/kernel/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}
}
Expand Down Expand Up @@ -281,18 +283,19 @@ pub(crate) fn from_err_ptr<T>(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`,
// which always fits in an `i16`, as per the invariant above.
// 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)
}
Expand Down
22 changes: 18 additions & 4 deletions rust/kernel/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// SAFETY: TODO
// SAFETY: we are in one of the macros permitted to call `__pin_data()` .

let data = unsafe {
use $crate::init::__internal::HasPinData;
$t$(::<$($generics),*>)?::__pin_data()
Expand Down Expand Up @@ -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
}};
Expand Down Expand Up @@ -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:
Comment on lines -738 to +742
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave the original comment and just add SAFETY: this code is never executed.

unsafe {
::core::ptr::write($slot, $t {
$($acc)*
Expand Down Expand Up @@ -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) };
Comment on lines +783 to 784
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can leave these (forgetting guards) with a SAFETY: TODO, since this part will be rewritten in an update soon and then no unsafe will be needed.


$crate::try_pin_init!(forget_guards:
Expand All @@ -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:
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// SAFETY: `HasInitData` is a marker trait which we are allowed to use in this context
// SAFETY: we are in one of the macros permitted to call `__init_data()` .

let data = unsafe {
use $crate::init::__internal::HasInitData;
$t$(::<$($generics),*>)?::__init_data()
Expand Down Expand Up @@ -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
}};
Expand Down Expand Up @@ -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:
Comment on lines -1002 to +1010
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

unsafe {
::core::ptr::write($slot, $t {
$($acc)*
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -1197,6 +1206,7 @@ pub fn uninit<T, E>() -> impl Init<MaybeUninit<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: `slot` is valid per the invariants of `__init`
unsafe { slot.write(self) };
Ok(())
}
Expand Down Expand Up @@ -1371,8 +1381,12 @@ pub fn zeroed<T: Zeroable>() -> impl Init<T> {
}
}

/// # Safety
///
/// This can only be used on types that meet `Zeroable`'s guidelines
Comment on lines +1384 to +1386
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// # Safety
///
/// This can only be used on types that meet `Zeroable`'s guidelines
/// # Safety
///
/// This can only be used on types that meet `Zeroable`'s safety contract.

macro_rules! impl_zeroable {
($($({$($generics:tt)*})? $t:ty, )*) => {
// SAFETY: upheld by documented use
$(unsafe impl$($($generics)*)? Zeroable for $t {})*
};
}
Expand Down
2 changes: 2 additions & 0 deletions rust/kernel/init/__internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
//! - `macros/pin_data.rs`
//! - `macros/pinned_drop.rs`

#![allow(clippy::undocumented_unsafe_blocks)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also document these here.

Copy link
Collaborator Author

@tgross35 tgross35 Jul 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only misssing two:

unsafe impl<T: ?Sized> InitData for AllData<T>

and

unsafe impl<T: ?Sized> HasInitData for T 

Would you mind providing the comments for them?


use super::*;

/// See the [nomicon] for what subtyping is. See also [this table].
Expand Down
4 changes: 4 additions & 0 deletions rust/kernel/init/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)*
{
Expand Down Expand Up @@ -841,6 +842,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)*
Expand Down Expand Up @@ -965,6 +967,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) }
}
)*
Expand All @@ -974,6 +977,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function has no # Safety section, so it effectively has no contract. I think we should just give it the same contract as Init::__init.

unsafe { $crate::init::Init::__init(init, slot) }
}
)*
Expand Down
28 changes: 18 additions & 10 deletions rust/kernel/print.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,20 @@
use core::{
ffi::{c_char, c_void},
fmt,
ptr::addr_of,
};

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,
Expand All @@ -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::<fmt::Arguments<'_>>()) });
w.pos().cast()
}

Expand All @@ -49,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');
Expand Down Expand Up @@ -105,13 +113,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::<c_void>(),
);
}
}
Expand All @@ -124,14 +133,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::<c_void>(),
);
}
}
Expand Down
7 changes: 4 additions & 3 deletions rust/kernel/str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()) }
}

Expand Down
Loading