| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250227104640.sa6vqdz7j5hwjigs@vireshk-i7>
Date: Thu, 27 Feb 2025 16:16:40 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: a.hindborg@...nel.org, alex.gaynor@...il.com, benno.lossin@...ton.me,
bjorn3_gh@...tonmail.com, boqun.feng@...il.com, dakr@...hat.com,
gary@...yguo.net, linux-kernel@...r.kernel.org,
linux@...musvillemoes.dk, ojeda@...nel.org,
rust-for-linux@...r.kernel.org, tmgross@...ch.edu,
vincent.guittot@...aro.org, yury.norov@...il.com
Subject: Re: [PATCH 1/2] rust: Add initial cpumask abstractions
On 25-02-25, 18:02, Alice Ryhl wrote:
> diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
> +/// This corresponds to the C type alias `cpumask_var_t`.
> +pub struct CpumaskBox {
> + #[cfg(CONFIG_CPUMASK_OFFSTACK)]
> + ptr: NonNull<Cpumask>,
> + #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> + mask: Cpumask,
> +}
>
> +impl CpumaskBox {
> + pub fn new(_flags: Flags) -> Result<Self, AllocError> {
> + Ok(Self {
> + #[cfg(CONFIG_CPUMASK_OFFSTACK)]
> + ptr: {
> + let mut ptr: *mut bindings::cpumask = ptr::null_mut();
> + unsafe { bindings::zalloc_cpumask_var(&mut ptr, _flags.as_raw()) };
> + NonNull::new(ptr.cast()).ok_or(AllocError)?
> + },
> #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
> - // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
> - unsafe {
> - drop(KBox::from_raw(self.ptr))
> - };
> - }
> + mask: unsafe { core::mem::zeroed() },
This will work correctly, yes, but I wasn't sure if the Rust code
should do this or depend on the C API and call zalloc_cpumask_var().
The implementation of struct cpumask allows this to work right now,
but any change where some internal state management is done, for
example, within the structure may break the Rust side.
I am okay with this way of doing it if that looks okay to you.
I have made following changes over your diff.
Thanks for your help in simplifying the code.
--
viresh
-------------------------8<-------------------------
diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
index f82dafbc4e61..d56f7dc9d762 100644
--- a/rust/kernel/cpumask.rs
+++ b/rust/kernel/cpumask.rs
@@ -13,20 +13,28 @@
#[cfg(CONFIG_CPUMASK_OFFSTACK)]
use core::ptr::{self, NonNull};
+#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+use core::mem::MaybeUninit;
+
+use core::ops::{Deref, DerefMut};
+
/// This corresponds to the C type `struct cpumask`.
#[repr(transparent)]
-pub struct Cpumask {
- mask: Opaque<bindings::cpumask>,
-}
+pub struct Cpumask(Opaque<bindings::cpumask>);
impl Cpumask {
- /// Creates a reference to an existing `struct cpumask` pointer.
+ /// Creates a mutable reference to an existing `struct cpumask` pointer.
///
/// # Safety
///
/// The caller must ensure that `ptr` is valid for writing and remains valid for the lifetime
/// of the returned reference.
- pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask) -> &'a mut Self {
+ pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask_var_t) -> &'a mut Self {
+ // For this configuration, `cpumask_var_t` is defined as `[cpumask; 1]`.
+ #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+ let ptr: *mut bindings::cpumask = ptr as *mut _;
+
+ // SAFETY: Guaranteed by the safety requirements of the function.
unsafe { &mut *ptr.cast() }
}
@@ -36,7 +44,12 @@ pub unsafe fn from_raw_mut<'a>(ptr: *mut bindings::cpumask) -> &'a mut Self {
///
/// The caller must ensure that `ptr` is valid for reading and remains valid for the lifetime
/// of the returned reference.
- pub unsafe fn from_raw<'a>(ptr: *const bindings::cpumask) -> &'a Self {
+ pub unsafe fn from_raw<'a>(ptr: *const bindings::cpumask_var_t) -> &'a Self {
+ // For this configuration, `cpumask_var_t` is defined as `[cpumask; 1]`.
+ #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+ let ptr: *const bindings::cpumask = ptr as *const _;
+
+ // SAFETY: Guaranteed by the safety requirements of the function.
unsafe { &*ptr.cast() }
}
@@ -45,41 +58,37 @@ pub fn as_raw(&self) -> *mut bindings::cpumask {
self as *const Cpumask as *mut bindings::cpumask
}
- /// Sets CPU in the cpumask.
- ///
- /// Update the cpumask with a single CPU.
+ /// Sets `cpu` in the cpumask.
pub fn set(&mut self, cpu: u32) {
- // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+ // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
// call `cpumask_set_cpus()` for any CPU.
unsafe { bindings::cpumask_set_cpu(cpu, self.as_raw()) };
}
- /// Clears CPU in the cpumask.
- ///
- /// Update the cpumask with a single CPU.
+ /// Clears `cpu` in the cpumask.
pub fn clear(&mut self, cpu: i32) {
- // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+ // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
// call `cpumask_clear_cpu()` for any CPU.
unsafe { bindings::cpumask_clear_cpu(cpu, self.as_raw()) };
}
/// Sets all CPUs in the cpumask.
pub fn set_all(&mut self) {
- // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+ // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
// call `cpumask_setall()`.
unsafe { bindings::cpumask_setall(self.as_raw()) };
}
- /// Gets weight of a cpumask.
+ /// Gets weight of the cpumask.
pub fn weight(&self) -> u32 {
- // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+ // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
// call `cpumask_weight()`.
unsafe { bindings::cpumask_weight(self.as_raw()) }
}
/// Copies cpumask.
pub fn copy(&self, dstp: &mut Self) {
- // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+ // SAFETY: `mask` is guaranteed to be valid for the lifetime of `self`. And it is safe to
// call `cpumask_copy()`.
unsafe { bindings::cpumask_copy(dstp.as_raw(), self.as_raw()) };
}
@@ -94,26 +103,65 @@ pub struct CpumaskBox {
}
impl CpumaskBox {
+ /// Creates an initialized instance of the `CpumaskBox`.
pub fn new(_flags: Flags) -> Result<Self, AllocError> {
Ok(Self {
#[cfg(CONFIG_CPUMASK_OFFSTACK)]
ptr: {
let mut ptr: *mut bindings::cpumask = ptr::null_mut();
+
+ // SAFETY: Depending on the value of `_flags`, this call may sleep. Other than
+ // that, it is always safe to call this method.
unsafe { bindings::zalloc_cpumask_var(&mut ptr, _flags.as_raw()) };
NonNull::new(ptr.cast()).ok_or(AllocError)?
},
+
#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+ // SAFETY: FFI type is valid to be zero-initialized.
mask: unsafe { core::mem::zeroed() },
})
}
+
+ /// Creates an uninitialized instance of the `CpumaskBox`.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that the returned `CpumaskBox` is properly initialized before
+ /// getting used.
+ unsafe fn new_uninit(_flags: Flags) -> Result<Self, AllocError> {
+ Ok(Self {
+ #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+ ptr: {
+ let mut ptr: *mut bindings::cpumask = ptr::null_mut();
+
+ // SAFETY: Depending on the value of `_flags`, this call may sleep. Other than
+ // that, it is always safe to call this method.
+ unsafe { bindings::alloc_cpumask_var(&mut ptr, _flags.as_raw()) };
+ NonNull::new(ptr.cast()).ok_or(AllocError)?
+ },
+ #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+ // SAFETY: Guaranteed by the safety requirements of the function.
+ mask: unsafe { MaybeUninit::uninit().assume_init() },
+ })
+ }
+
+ /// Clones cpumask.
+ pub fn try_clone(cpumask: &Cpumask) -> Result<Self> {
+ // SAFETY: The returned cpumask_box is initialized right after this call.
+ let mut cpumask_box = unsafe { Self::new_uninit(GFP_KERNEL) }?;
+
+ cpumask.copy(&mut cpumask_box);
+ Ok(cpumask_box)
+ }
}
// Make CpumaskBox behave like a pointer to Cpumask.
-impl core::ops::Deref for CpumaskBox {
+impl Deref for CpumaskBox {
type Target = Cpumask;
#[cfg(CONFIG_CPUMASK_OFFSTACK)]
fn deref(&self) -> &Cpumask {
+ // SAFETY: The caller owns CpumaskBox, so it is safe to deref the cpumask.
unsafe { &*self.ptr.as_ptr() }
}
@@ -123,9 +171,23 @@ fn deref(&self) -> &Cpumask {
}
}
+impl DerefMut for CpumaskBox {
+ #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+ fn deref_mut(&mut self) -> &mut Cpumask {
+ // SAFETY: The caller owns CpumaskBox, so it is safe to deref the cpumask.
+ unsafe { self.ptr.as_mut() }
+ }
+
+ #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+ fn deref_mut(&mut self) -> &mut Cpumask {
+ &mut self.mask
+ }
+}
+
impl Drop for CpumaskBox {
fn drop(&mut self) {
#[cfg(CONFIG_CPUMASK_OFFSTACK)]
+ // SAFETY: It is safe to free the cpumask.
unsafe {
bindings::free_cpumask_var(self.as_raw())
};
Powered by blists - more mailing lists