From faa3cbcca03d0dec8f8e43f1d8d5c0860d98a23f Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Sun, 9 May 2021 03:45:34 +0100 Subject: [PATCH 1/3] rust: CStr overhaul `CStr` is overhauled to make using it more similar to use a `str`. Signed-off-by: Gary Guo --- drivers/android/rust_binder.rs | 4 +- drivers/char/hw_random/bcm2835_rng_rust.rs | 4 +- rust/kernel/c_types.rs | 15 -- rust/kernel/chrdev.rs | 10 +- rust/kernel/lib.rs | 4 +- rust/kernel/miscdev.rs | 9 +- rust/kernel/module_param.rs | 3 +- rust/kernel/of.rs | 6 +- rust/kernel/platdev.rs | 8 +- rust/kernel/str.rs | 229 +++++++++++++++++++++ rust/kernel/sync/condvar.rs | 7 +- rust/kernel/sync/mod.rs | 7 +- rust/kernel/sync/mutex.rs | 7 +- rust/kernel/sync/spinlock.rs | 7 +- rust/kernel/sysctl.rs | 12 +- rust/kernel/types.rs | 45 ---- rust/module.rs | 2 +- samples/rust/rust_chrdev.rs | 4 +- samples/rust/rust_miscdev.rs | 4 +- samples/rust/rust_semaphore.rs | 4 +- 20 files changed, 284 insertions(+), 107 deletions(-) create mode 100644 rust/kernel/str.rs diff --git a/drivers/android/rust_binder.rs b/drivers/android/rust_binder.rs index 5d04784819fe31..617f7a8cab832b 100644 --- a/drivers/android/rust_binder.rs +++ b/drivers/android/rust_binder.rs @@ -10,7 +10,7 @@ use alloc::{boxed::Box, sync::Arc}; use core::pin::Pin; use kernel::{ - cstr, + c_str, io_buffer::IoBufferWriter, linked_list::{GetLinks, GetLinksWrapped, Links}, miscdev::Registration, @@ -111,7 +111,7 @@ impl KernelModule for BinderModule { let pinned_ctx = Context::new()?; let ctx = unsafe { Pin::into_inner_unchecked(pinned_ctx) }; let reg = Registration::>::new_pinned::( - cstr!("rust_binder"), + c_str!("rust_binder"), None, ctx, )?; diff --git a/drivers/char/hw_random/bcm2835_rng_rust.rs b/drivers/char/hw_random/bcm2835_rng_rust.rs index 8b9e83b013b58a..9ae5395172c317 100644 --- a/drivers/char/hw_random/bcm2835_rng_rust.rs +++ b/drivers/char/hw_random/bcm2835_rng_rust.rs @@ -9,7 +9,7 @@ use alloc::boxed::Box; use core::pin::Pin; use kernel::of::OfMatchTable; use kernel::prelude::*; -use kernel::{cstr, platdev}; +use kernel::{c_str, platdev}; module! { type: RngModule, @@ -25,7 +25,7 @@ struct RngModule { impl KernelModule for RngModule { fn init() -> Result { - let of_match_tbl = OfMatchTable::new(&cstr!("brcm,bcm2835-rng"))?; + let of_match_tbl = OfMatchTable::new(&c_str!("brcm,bcm2835-rng"))?; let pdev = platdev::Registration::new_pinned( cstr!("bcm2835-rng-rust"), diff --git a/rust/kernel/c_types.rs b/rust/kernel/c_types.rs index cbed43be2353e8..07593a3ba8bedb 100644 --- a/rust/kernel/c_types.rs +++ b/rust/kernel/c_types.rs @@ -117,18 +117,3 @@ mod c { } pub use c::*; - -/// Reads string until null byte is reached and returns slice excluding the -/// terminating null. -/// -/// # Safety -/// -/// The data from the pointer until the null terminator must be valid for reads -/// and not mutated for all of `'a`. The length of the string must also be less -/// than `isize::MAX`. See the documentation on -/// [`core::slice::from_raw_parts()`] for further details on safety of -/// converting a pointer to a slice. -pub unsafe fn c_string_bytes<'a>(ptr: *const crate::c_types::c_char) -> &'a [u8] { - let length = crate::bindings::strlen(ptr) as usize; - &core::slice::from_raw_parts(ptr as *const u8, length) -} diff --git a/rust/kernel/chrdev.rs b/rust/kernel/chrdev.rs index 1768b1ec01c75d..631405920c29c7 100644 --- a/rust/kernel/chrdev.rs +++ b/rust/kernel/chrdev.rs @@ -17,7 +17,7 @@ use crate::bindings; use crate::c_types; use crate::error::{Error, Result}; use crate::file_operations; -use crate::types::CStr; +use crate::str::CStr; /// Character device. /// @@ -87,7 +87,7 @@ struct RegistrationInner { /// /// May contain up to a fixed number (`N`) of devices. Must be pinned. pub struct Registration { - name: CStr<'static>, + name: &'static CStr, minors_start: u16, this_module: &'static crate::ThisModule, inner: Option>, @@ -104,7 +104,7 @@ impl Registration<{ N }> { /// are going to pin the registration right away, call /// [`Self::new_pinned()`] instead. pub fn new( - name: CStr<'static>, + name: &'static CStr, minors_start: u16, this_module: &'static crate::ThisModule, ) -> Self { @@ -120,7 +120,7 @@ impl Registration<{ N }> { /// /// This does *not* register the device: see [`Self::register()`]. pub fn new_pinned( - name: CStr<'static>, + name: &'static CStr, minors_start: u16, this_module: &'static crate::ThisModule, ) -> Result>> { @@ -146,7 +146,7 @@ impl Registration<{ N }> { &mut dev, this.minors_start.into(), N.try_into()?, - this.name.as_ptr() as *const c_types::c_char, + this.name.as_char_ptr(), ) }; if res != 0 { diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index c90c9a466e726e..a79c367f695cc5 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -19,6 +19,7 @@ const_fn, const_mut_refs, const_panic, + const_raw_ptr_deref, try_reserve )] #![deny(clippy::complexity)] @@ -44,6 +45,7 @@ pub mod file; pub mod file_operations; pub mod miscdev; pub mod pages; +pub mod str; pub mod linked_list; mod raw_list; @@ -72,7 +74,7 @@ pub mod user_ptr; pub use build_error::build_error; pub use crate::error::{Error, Result}; -pub use crate::types::{CStr, Mode}; +pub use crate::types::Mode; /// Page size defined in terms of the `PAGE_SHIFT` macro from C. /// diff --git a/rust/kernel/miscdev.rs b/rust/kernel/miscdev.rs index d61cc61f2f2bbb..3f66202cb66ddf 100644 --- a/rust/kernel/miscdev.rs +++ b/rust/kernel/miscdev.rs @@ -6,9 +6,10 @@ //! //! Reference: +use crate::bindings; use crate::error::{Error, Result}; use crate::file_operations::{FileOpenAdapter, FileOpener, FileOperationsVtable}; -use crate::{bindings, c_types, CStr}; +use crate::str::CStr; use alloc::boxed::Box; use core::marker::PhantomPinned; use core::pin::Pin; @@ -41,7 +42,7 @@ impl Registration { /// /// Returns a pinned heap-allocated representation of the registration. pub fn new_pinned>( - name: CStr<'static>, + name: &'static CStr, minor: Option, context: T, ) -> Result>> { @@ -56,7 +57,7 @@ impl Registration { /// self-referential. If a minor is not given, the kernel allocates a new one if possible. pub fn register>( self: Pin<&mut Self>, - name: CStr<'static>, + name: &'static CStr, minor: Option, ) -> Result { // SAFETY: We must ensure that we never move out of `this`. @@ -68,7 +69,7 @@ impl Registration { // SAFETY: The adapter is compatible with `misc_register`. this.mdev.fops = unsafe { FileOperationsVtable::::build() }; - this.mdev.name = name.as_ptr() as *const c_types::c_char; + this.mdev.name = name.as_char_ptr(); this.mdev.minor = minor.unwrap_or(bindings::MISC_DYNAMIC_MINOR as i32); let ret = unsafe { bindings::misc_register(&mut this.mdev) }; diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs index e8d51fe613f5b7..c70f61367347f3 100644 --- a/rust/kernel/module_param.rs +++ b/rust/kernel/module_param.rs @@ -4,6 +4,7 @@ //! //! C header: [`include/linux/moduleparam.h`](../../../include/linux/moduleparam.h) +use crate::str::CStr; use core::fmt::Write; /// Types that can be used for module parameters. @@ -70,7 +71,7 @@ pub trait ModuleParam: core::fmt::Display + core::marker::Sized { let arg = if val.is_null() { None } else { - Some(crate::c_types::c_string_bytes(val)) + Some(CStr::from_char_ptr(val).as_bytes()) }; match Self::try_from_param_arg(arg) { Some(new_value) => { diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs index e74c0da023a434..b7f5e429369151 100644 --- a/rust/kernel/of.rs +++ b/rust/kernel/of.rs @@ -9,8 +9,8 @@ use alloc::boxed::Box; use crate::{ bindings, c_types, error::{Error, Result}, + str::CStr, types::PointerWrapper, - CStr, }; use core::mem::transmute; @@ -32,7 +32,7 @@ pub struct OfMatchTable(InnerTable); impl OfMatchTable { /// Creates a [`OfMatchTable`] from a single `compatible` string. - pub fn new(compatible: &CStr<'static>) -> Result { + pub fn new(compatible: &'static CStr) -> Result { let tbl = Box::try_new([ Self::new_of_device_id(compatible)?, bindings::of_device_id::default(), @@ -43,7 +43,7 @@ impl OfMatchTable { Ok(Self(tbl)) } - fn new_of_device_id(compatible: &CStr<'static>) -> Result { + fn new_of_device_id(compatible: &'static CStr) -> Result { let mut buf = [0_u8; 128]; if compatible.len() > buf.len() { return Err(Error::EINVAL); diff --git a/rust/kernel/platdev.rs b/rust/kernel/platdev.rs index ff95300a1ae127..c4e6061eb40440 100644 --- a/rust/kernel/platdev.rs +++ b/rust/kernel/platdev.rs @@ -11,8 +11,8 @@ use crate::{ error::{Error, Result}, of::OfMatchTable, pr_info, + str::CStr, types::PointerWrapper, - CStr, }; use alloc::boxed::Box; use core::{marker::PhantomPinned, pin::Pin}; @@ -43,7 +43,7 @@ extern "C" fn remove_callback(_pdev: *mut bindings::platform_device) -> c_types: impl Registration { fn register( self: Pin<&mut Self>, - name: CStr<'static>, + name: &'static CStr, of_match_table: Option, module: &'static crate::ThisModule, ) -> Result { @@ -53,7 +53,7 @@ impl Registration { // Already registered. return Err(Error::EINVAL); } - this.pdrv.driver.name = name.as_ptr() as *const c_types::c_char; + this.pdrv.driver.name = name.as_char_ptr(); if let Some(tbl) = of_match_table { let ptr = tbl.into_pointer(); this.of_table = Some(ptr); @@ -82,7 +82,7 @@ impl Registration { /// /// Returns a pinned heap-allocated representation of the registration. pub fn new_pinned( - name: CStr<'static>, + name: &'static CStr, of_match_tbl: Option, module: &'static crate::ThisModule, ) -> Result>> { diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs new file mode 100644 index 00000000000000..939fa0feec4f14 --- /dev/null +++ b/rust/kernel/str.rs @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! String representations. + +use core::ops::{self, Deref, Index}; + +use crate::bindings; +use crate::c_types; + +/// Byte string without UTF-8 validity guarantee. +/// +/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning. +pub type BStr = [u8]; + +/// Creates a new [`BStr`] from a string literal. +/// +/// `b_str!` converts the supplied string literal to byte string, so non-ASCII +/// characters can be included. +/// +/// # Examples +/// +/// ```rust,no_run +/// const MY_BSTR: &'static BStr = b_str!("My awesome BStr!"); +/// ``` +#[macro_export] +macro_rules! b_str { + ($str:literal) => {{ + const S: &'static str = $str; + const C: &'static $crate::str::BStr = S.as_bytes(); + C + }}; +} + +/// Possible errors when using conversion functions in [`CStr`]. +#[derive(Debug, Clone, Copy)] +pub enum CStrConvertError { + /// Supplied bytes contain an interior `NUL`. + InteriorNul, + + /// Supplied bytes are not terminated by `NUL`. + NotNulTerminated, +} + +impl From for crate::Error { + #[inline] + fn from(_: CStrConvertError) -> crate::Error { + crate::Error::EINVAL + } +} + +/// A string that is guaranteed to have exactly one `NUL` byte, which is at the +/// end. +/// +/// Used for interoperability with kernel APIs that take C strings. +#[repr(transparent)] +pub struct CStr([u8]); + +impl CStr { + /// Returns the length of this string excluding `NUL`. + #[inline] + pub const fn len(&self) -> usize { + self.0.len() - 1 + } + + /// Returns the length of this string with `NUL`. + #[inline] + pub const fn len_with_nul(&self) -> usize { + self.0.len() + } + + /// Returns `true` if the string only includes `NUL`. + #[inline] + pub const fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// Wraps a raw C string pointer. + /// + /// # Safety + /// + /// `ptr` must be a valid pointer to a `NUL`-terminated C string, and it must + /// last at least `'a`. When `CStr` is alive, the memory pointed by `ptr` + /// must not be mutated. + #[inline] + pub unsafe fn from_char_ptr<'a>(ptr: *const c_types::c_char) -> &'a Self { + let len = bindings::strlen(ptr) + 1; + Self::from_bytes_with_nul_unchecked(core::slice::from_raw_parts(ptr as _, len as _)) + } + + /// Creates a [`CStr`] from a `[u8]`. + /// + /// The provided slice must be `NUL`-terminated, does not contain any + /// interior `NUL` bytes. + pub fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError> { + if bytes.is_empty() { + return Err(CStrConvertError::NotNulTerminated); + } + if bytes[bytes.len() - 1] != 0 { + return Err(CStrConvertError::NotNulTerminated); + } + if bytes[..bytes.len()].contains(&0) { + return Err(CStrConvertError::InteriorNul); + } + // SAFETY: We just checked that all properties hold. + Ok(unsafe { Self::from_bytes_with_nul_unchecked(bytes) }) + } + + /// Creates a [`CStr`] from a `[u8]` without performing any additional + /// checks. + /// + /// # Safety + /// + /// `bytes` *must* end with a `NUL` byte, and should only have a single + /// `NUL` byte (or the string will be truncated). + #[inline] + pub const unsafe fn from_bytes_with_nul_unchecked(bytes: &[u8]) -> &CStr { + // Note: This can be done using pointer deref (which requires + // `const_raw_ptr_deref` to be const) or `transmute` (which requires + // `const_transmute` to be const) or `ptr::from_raw_parts` (which + // requires `ptr_metadata`). + // While none of them are current stable, it is very likely that one of + // them will eventually be. + &*(bytes as *const [u8] as *const Self) + } + + /// Returns a C pointer to the string. + #[inline] + pub const fn as_char_ptr(&self) -> *const c_types::c_char { + self.0.as_ptr() as _ + } + + /// Convert the string to a byte slice without the trailing 0 byte. + #[inline] + pub fn as_bytes(&self) -> &[u8] { + &self.0[..self.0.len() - 1] + } + + /// Convert the string to a byte slice containing the trailing 0 byte. + #[inline] + pub const fn as_bytes_with_nul(&self) -> &[u8] { + &self.0 + } +} + +impl AsRef for CStr { + #[inline] + fn as_ref(&self) -> &BStr { + self.as_bytes() + } +} + +impl Deref for CStr { + type Target = BStr; + + #[inline] + fn deref(&self) -> &Self::Target { + self.as_bytes() + } +} + +impl Index> for CStr { + type Output = CStr; + + #[inline] + fn index(&self, index: ops::RangeFrom) -> &Self::Output { + assert!( + index.start <= self.len(), + "range start index {} out of range for slice of length {}", + index.start, + self.len() + ); + // SAFETY: We just checked the length. + unsafe { Self::from_bytes_with_nul_unchecked(&self.0[index.start..]) } + } +} + +impl Index for CStr { + type Output = CStr; + + #[inline] + fn index(&self, _index: ops::RangeFull) -> &Self::Output { + self + } +} + +mod private { + use core::ops; + + // Marker trait for index types that can be forward to `BStr`. + pub trait CStrIndex {} + + impl CStrIndex for usize {} + impl CStrIndex for ops::Range {} + impl CStrIndex for ops::RangeInclusive {} + impl CStrIndex for ops::RangeToInclusive {} +} + +impl Index for CStr +where + Idx: private::CStrIndex, + BStr: Index, +{ + type Output = >::Output; + + #[inline] + fn index(&self, index: Idx) -> &Self::Output { + &self.as_bytes()[index] + } +} + +/// Creates a new [`CStr`] from a string literal. +/// +/// The string literal should not contain any `NUL` bytes. +/// +/// # Examples +/// +/// ```rust,no_run +/// const MY_CSTR: &'static CStr = c_str!("My awesome CStr!"); +/// ``` +#[macro_export] +macro_rules! c_str { + ($str:literal) => {{ + // FIXME: Check that `$str` does not contain interior `NUL`. + const S: &str = concat!($str, "\0"); + const C: &$crate::str::CStr = + { unsafe { $crate::str::CStr::from_bytes_with_nul_unchecked(S.as_bytes()) } }; + C + }}; +} diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs index be6012794777c5..3c5bcbadab71cb 100644 --- a/rust/kernel/sync/condvar.rs +++ b/rust/kernel/sync/condvar.rs @@ -6,7 +6,8 @@ //! variable. use super::{Guard, Lock, NeedsLockClass}; -use crate::{bindings, CStr}; +use crate::bindings; +use crate::str::CStr; use core::{cell::UnsafeCell, marker::PhantomPinned, mem::MaybeUninit, pin::Pin}; extern "C" { @@ -130,7 +131,7 @@ impl CondVar { } impl NeedsLockClass for CondVar { - unsafe fn init(self: Pin<&Self>, name: CStr<'static>, key: *mut bindings::lock_class_key) { - bindings::__init_waitqueue_head(self.wait_list.get(), name.as_ptr() as _, key); + unsafe fn init(self: Pin<&Self>, name: &'static CStr, key: *mut bindings::lock_class_key) { + bindings::__init_waitqueue_head(self.wait_list.get(), name.as_char_ptr(), key); } } diff --git a/rust/kernel/sync/mod.rs b/rust/kernel/sync/mod.rs index 0a8cec021e5c54..779d04e79e874d 100644 --- a/rust/kernel/sync/mod.rs +++ b/rust/kernel/sync/mod.rs @@ -17,7 +17,8 @@ //! } //! ``` -use crate::{bindings, c_types, CStr}; +use crate::str::CStr; +use crate::{bindings, c_types}; use core::pin::Pin; mod arc; @@ -51,7 +52,7 @@ macro_rules! init_with_lockdep { // SAFETY: `CLASS` is never used by Rust code directly; the kernel may change it though. #[allow(unused_unsafe)] unsafe { - $crate::sync::NeedsLockClass::init($obj, $crate::cstr!($name), CLASS.as_mut_ptr()) + $crate::sync::NeedsLockClass::init($obj, $crate::c_str!($name), CLASS.as_mut_ptr()) }; }}; } @@ -69,7 +70,7 @@ pub trait NeedsLockClass { /// # Safety /// /// `key` must point to a valid memory location as it will be used by the kernel. - unsafe fn init(self: Pin<&Self>, name: CStr<'static>, key: *mut bindings::lock_class_key); + unsafe fn init(self: Pin<&Self>, name: &'static CStr, key: *mut bindings::lock_class_key); } /// Determines if a signal is pending on the current process. diff --git a/rust/kernel/sync/mutex.rs b/rust/kernel/sync/mutex.rs index e528228d16c101..313a1d0920b24d 100644 --- a/rust/kernel/sync/mutex.rs +++ b/rust/kernel/sync/mutex.rs @@ -5,7 +5,8 @@ //! This module allows Rust code to use the kernel's [`struct mutex`]. use super::{Guard, Lock, NeedsLockClass}; -use crate::{bindings, CStr}; +use crate::bindings; +use crate::str::CStr; use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin}; /// Safely initialises a [`Mutex`] with the given name, generating a new lock class. @@ -71,8 +72,8 @@ impl Mutex { } impl NeedsLockClass for Mutex { - unsafe fn init(self: Pin<&Self>, name: CStr<'static>, key: *mut bindings::lock_class_key) { - bindings::__mutex_init(self.mutex.get(), name.as_ptr() as _, key); + unsafe fn init(self: Pin<&Self>, name: &'static CStr, key: *mut bindings::lock_class_key) { + bindings::__mutex_init(self.mutex.get(), name.as_char_ptr(), key); } } diff --git a/rust/kernel/sync/spinlock.rs b/rust/kernel/sync/spinlock.rs index 49a7d5fd837b2f..7fd2ab54db680a 100644 --- a/rust/kernel/sync/spinlock.rs +++ b/rust/kernel/sync/spinlock.rs @@ -7,7 +7,8 @@ //! See . use super::{Guard, Lock, NeedsLockClass}; -use crate::{bindings, c_types, CStr}; +use crate::str::CStr; +use crate::{bindings, c_types}; use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin}; extern "C" { @@ -85,8 +86,8 @@ impl SpinLock { } impl NeedsLockClass for SpinLock { - unsafe fn init(self: Pin<&Self>, name: CStr<'static>, key: *mut bindings::lock_class_key) { - rust_helper_spin_lock_init(self.spin_lock.get(), name.as_ptr() as _, key); + unsafe fn init(self: Pin<&Self>, name: &'static CStr, key: *mut bindings::lock_class_key) { + rust_helper_spin_lock_init(self.spin_lock.get(), name.as_char_ptr(), key); } } diff --git a/rust/kernel/sysctl.rs b/rust/kernel/sysctl.rs index f70b09c9147ef2..df90c218479aef 100644 --- a/rust/kernel/sysctl.rs +++ b/rust/kernel/sysctl.rs @@ -15,6 +15,7 @@ use core::sync::atomic; use crate::{ bindings, c_types, error, io_buffer::IoBufferWriter, + str::CStr, types, user_ptr::{UserSlicePtr, UserSlicePtrWriter}, }; @@ -130,19 +131,19 @@ unsafe extern "C" fn proc_handler( impl Sysctl { /// Registers a single entry in `sysctl`. pub fn register( - path: types::CStr<'static>, - name: types::CStr<'static>, + path: &'static CStr, + name: &'static CStr, storage: T, mode: types::Mode, ) -> error::Result> { - if name.contains('/') { + if name.contains(&b'/') { return Err(error::Error::EINVAL); } let storage = Box::try_new(storage)?; let mut table = vec![ bindings::ctl_table { - procname: name.as_ptr() as *const i8, + procname: name.as_char_ptr(), mode: mode.as_int(), data: &*storage as *const T as *mut c_types::c_void, proc_handler: Some(proc_handler::), @@ -157,8 +158,7 @@ impl Sysctl { ] .into_boxed_slice(); - let result = - unsafe { bindings::register_sysctl(path.as_ptr() as *const i8, table.as_mut_ptr()) }; + let result = unsafe { bindings::register_sysctl(path.as_char_ptr(), table.as_mut_ptr()) }; if result.is_null() { return Err(error::Error::ENOMEM); } diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index 5af7cf8cf80664..63d8477acb9183 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -31,51 +31,6 @@ impl Mode { } } -/// A string that is guaranteed to have exactly one `NUL` byte, which is at the -/// end. -/// -/// Used for interoperability with kernel APIs that take C strings. -#[repr(transparent)] -pub struct CStr<'a>(&'a str); - -impl CStr<'_> { - /// Creates a [`CStr`] from a [`str`] without performing any additional - /// checks. - /// - /// # Safety - /// - /// `data` *must* end with a `NUL` byte, and should only have only a single - /// `NUL` byte (or the string will be truncated). - pub const unsafe fn new_unchecked(data: &str) -> CStr { - CStr(data) - } -} - -impl Deref for CStr<'_> { - type Target = str; - - fn deref(&self) -> &str { - self.0 - } -} - -/// Creates a new `CStr` from a string literal. -/// -/// The string literal should not contain any `NUL` bytes. -/// -/// # Examples -/// -/// ```rust,no_run -/// const MY_CSTR: CStr<'static> = cstr!("My awesome CStr!"); -/// ``` -#[macro_export] -macro_rules! cstr { - ($str:expr) => {{ - let s = concat!($str, "\x00"); - unsafe { $crate::CStr::new_unchecked(s) } - }}; -} - /// Used to convert an object into a raw pointer that represents it. /// /// It can eventually be converted back into the object. This is used to store objects as pointers diff --git a/rust/module.rs b/rust/module.rs index dfd2d2cfce0277..9a7009644d3cd7 100644 --- a/rust/module.rs +++ b/rust/module.rs @@ -821,7 +821,7 @@ pub fn module_misc_device(ts: TokenStream) -> TokenStream { fn init() -> kernel::Result {{ Ok(Self {{ _dev: kernel::miscdev::Registration::new_pinned::<{type_}>( - kernel::cstr!(\"{name}\"), + kernel::c_str!(\"{name}\"), None, (), )?, diff --git a/samples/rust/rust_chrdev.rs b/samples/rust/rust_chrdev.rs index 3bfe624de0658c..0e8151dd783940 100644 --- a/samples/rust/rust_chrdev.rs +++ b/samples/rust/rust_chrdev.rs @@ -8,7 +8,7 @@ use alloc::boxed::Box; use core::pin::Pin; use kernel::prelude::*; -use kernel::{chrdev, cstr, file_operations::FileOperations}; +use kernel::{c_str, chrdev, file_operations::FileOperations}; module! { type: RustChrdev, @@ -34,7 +34,7 @@ impl KernelModule for RustChrdev { pr_info!("Rust character device sample (init)\n"); let mut chrdev_reg = - chrdev::Registration::new_pinned(cstr!("rust_chrdev"), 0, &THIS_MODULE)?; + chrdev::Registration::new_pinned(c_str!("rust_chrdev"), 0, &THIS_MODULE)?; // Register the same kind of device twice, we're just demonstrating // that you can use multiple minors. There are two minors in this case diff --git a/samples/rust/rust_miscdev.rs b/samples/rust/rust_miscdev.rs index a3c65fb3077012..f3293b4800904b 100644 --- a/samples/rust/rust_miscdev.rs +++ b/samples/rust/rust_miscdev.rs @@ -9,7 +9,7 @@ use alloc::{boxed::Box, sync::Arc}; use core::pin::Pin; use kernel::prelude::*; use kernel::{ - cstr, + c_str, file::File, file_operations::{FileOpener, FileOperations}, io_buffer::{IoBufferReader, IoBufferWriter}, @@ -135,7 +135,7 @@ impl KernelModule for RustMiscdev { let state = SharedState::try_new()?; Ok(RustMiscdev { - _dev: miscdev::Registration::new_pinned::(cstr!("rust_miscdev"), None, state)?, + _dev: miscdev::Registration::new_pinned::(c_str!("rust_miscdev"), None, state)?, }) } } diff --git a/samples/rust/rust_semaphore.rs b/samples/rust/rust_semaphore.rs index 157d2523863b12..b3843381f7c968 100644 --- a/samples/rust/rust_semaphore.rs +++ b/samples/rust/rust_semaphore.rs @@ -22,7 +22,7 @@ use core::{ sync::atomic::{AtomicU64, Ordering}, }; use kernel::{ - condvar_init, cstr, declare_file_operations, + c_str, condvar_init, declare_file_operations, file::File, file_operations::{FileOpener, FileOperations, IoctlCommand, IoctlHandler}, io_buffer::{IoBufferReader, IoBufferWriter}, @@ -140,7 +140,7 @@ impl KernelModule for RustSemaphore { mutex_init!(Pin::new_unchecked(&sema.inner), "Semaphore::inner"); Ok(Self { - _dev: Registration::new_pinned::(cstr!("rust_semaphore"), None, sema)?, + _dev: Registration::new_pinned::(c_str!("rust_semaphore"), None, sema)?, }) } } From 27f0e08ab6e03f321fbf94840b8313554cd319b0 Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Tue, 18 May 2021 20:24:20 +0100 Subject: [PATCH 2/3] rust: check literal does not contain `NUL` in `c_str!` Suggested-by: Wedson Almeida Filho Signed-off-by: Gary Guo --- rust/kernel/str.rs | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 939fa0feec4f14..db57d50bc3a174 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -91,20 +91,36 @@ impl CStr { /// /// The provided slice must be `NUL`-terminated, does not contain any /// interior `NUL` bytes. - pub fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError> { + pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError> { if bytes.is_empty() { return Err(CStrConvertError::NotNulTerminated); } if bytes[bytes.len() - 1] != 0 { return Err(CStrConvertError::NotNulTerminated); } - if bytes[..bytes.len()].contains(&0) { - return Err(CStrConvertError::InteriorNul); + let mut i = 0; + while i < bytes.len() - 1 { + if bytes[i] == 0 { + return Err(CStrConvertError::InteriorNul); + } + i += 1; } // SAFETY: We just checked that all properties hold. Ok(unsafe { Self::from_bytes_with_nul_unchecked(bytes) }) } + /// Creates a [`CStr`] from a `[u8]`, panic if input is not valid. + /// + /// This function is only meant to be used by `c_str!` macro, so + /// crates using `c_str!` macro don't have to enable `const_panic` feature. + #[doc(hidden)] + pub const fn from_bytes_with_nul_unwrap(bytes: &[u8]) -> &Self { + match Self::from_bytes_with_nul(bytes) { + Ok(v) => v, + Err(_) => panic!("string contains interior NUL"), + } + } + /// Creates a [`CStr`] from a `[u8]` without performing any additional /// checks. /// @@ -220,10 +236,8 @@ where #[macro_export] macro_rules! c_str { ($str:literal) => {{ - // FIXME: Check that `$str` does not contain interior `NUL`. const S: &str = concat!($str, "\0"); - const C: &$crate::str::CStr = - { unsafe { $crate::str::CStr::from_bytes_with_nul_unchecked(S.as_bytes()) } }; + const C: &$crate::str::CStr = $crate::str::CStr::from_bytes_with_nul_unwrap(S.as_bytes()); C }}; } From 3a859fde34d5d307ddcecdd9f5132d64e126ea01 Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Thu, 20 May 2021 04:03:16 +0100 Subject: [PATCH 3/3] rust: optimize bounds checking for `CStr` Signed-off-by: Gary Guo --- rust/kernel/lib.rs | 1 + rust/kernel/str.rs | 26 ++++++++++++++++---------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index a79c367f695cc5..1e7c2262ab26af 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -20,6 +20,7 @@ const_mut_refs, const_panic, const_raw_ptr_deref, + const_unreachable_unchecked, try_reserve )] #![deny(clippy::complexity)] diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index db57d50bc3a174..9d3f0fa3b801bf 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -59,12 +59,18 @@ impl CStr { /// Returns the length of this string excluding `NUL`. #[inline] pub const fn len(&self) -> usize { - self.0.len() - 1 + self.len_with_nul() - 1 } /// 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() { + unsafe { core::hint::unreachable_unchecked() }; + } self.0.len() } @@ -99,7 +105,9 @@ impl CStr { return Err(CStrConvertError::NotNulTerminated); } let mut i = 0; - while i < bytes.len() - 1 { + // `i + 1 < bytes.len()` allows LLVM to optimize away bounds checking, + // while it couldn't optimize away bounds checks for `i < bytes.len() - 1`. + while i + 1 < bytes.len() { if bytes[i] == 0 { return Err(CStrConvertError::InteriorNul); } @@ -148,7 +156,7 @@ impl CStr { /// Convert the string to a byte slice without the trailing 0 byte. #[inline] pub fn as_bytes(&self) -> &[u8] { - &self.0[..self.0.len() - 1] + &self.0[..self.len()] } /// Convert the string to a byte slice containing the trailing 0 byte. @@ -178,14 +186,12 @@ impl Index> for CStr { type Output = CStr; #[inline] + // Clippy false positive + #[allow(clippy::unnecessary_operation)] fn index(&self, index: ops::RangeFrom) -> &Self::Output { - assert!( - index.start <= self.len(), - "range start index {} out of range for slice of length {}", - index.start, - self.len() - ); - // SAFETY: We just checked the length. + // Delegate bounds checking to slice. + &self.as_bytes()[index.start..]; + // SAFETY: We just checked the bounds. unsafe { Self::from_bytes_with_nul_unchecked(&self.0[index.start..]) } } }