| 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: <20250414112943.r2h3a54r2jm4iana@vireshk-i7>
Date: Mon, 14 Apr 2025 16:59:43 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Yury Norov <yury.norov@...il.com>, Danilo Krummrich <dakr@...hat.com>,
Miguel Ojeda <ojeda@...nel.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Danilo Krummrich <dakr@...nel.org>, linux-pm@...r.kernel.org,
Vincent Guittot <vincent.guittot@...aro.org>,
Stephen Boyd <sboyd@...nel.org>, Nishanth Menon <nm@...com>,
rust-for-linux@...r.kernel.org,
Manos Pitsidianakis <manos.pitsidianakis@...aro.org>,
Erik Schilling <erik.schilling@...aro.org>,
Alex Bennée <alex.bennee@...aro.org>,
Joakim Bech <joakim.bech@...aro.org>, Rob Herring <robh@...nel.org>,
Burak Emir <bqe@...gle.com>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Russell King <linux@...linux.org.uk>, linux-clk@...r.kernel.org,
Michael Turquette <mturquette@...libre.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH V9 03/17] rust: cpumask: Add initial abstractions
On 11-04-25, 11:54, Yury Norov wrote:
> On Fri, Apr 11, 2025 at 04:25:02PM +0530, Viresh Kumar wrote:
> > + 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.
>
> I'm not sure I understand this sentence. What's wrong with safety when
> the alloc() function sleeps? Even if something is wrong. If you really
> want to protect your users, you'd introduce new_sync() version that
> returns error if user provides sleeping flags.
I borrowed this SAFETY comment from similar allocation done in
page.rs, I think we can skip it though. Here is the delta so far:
@@ -27,8 +27,8 @@
///
/// A [`Cpumask`] instance always corresponds to a valid C `struct cpumask`.
///
-/// The callers must ensure that the `struct cpumask` is valid for access and remains valid for the
-/// lifetime of the returned reference.
+/// The callers must ensure that the `struct cpumask` is valid for access and
+/// remains valid for the lifetime of the returned reference.
///
/// ## Examples
///
@@ -86,7 +86,9 @@ pub fn as_raw(&self) -> *mut bindings::cpumask {
/// Set `cpu` in the cpumask.
///
- /// Equivalent to the kernel's `__cpumask_set_cpu` API.
+ /// ATTENTION: Contrary to C, this Rust `set()` method is non-atomic.
+ /// This mismatches kernel naming convention and corresponds to the C
+ /// function `__cpumask_set_cpu()`.
#[inline]
pub fn set(&mut self, cpu: u32) {
// SAFETY: By the type invariant, `self.as_raw` is a valid argument to `__cpumask_set_cpu`.
@@ -95,7 +97,9 @@ pub fn set(&mut self, cpu: u32) {
/// Clear `cpu` in the cpumask.
///
- /// Equivalent to the kernel's `__cpumask_clear_cpu` API.
+ /// ATTENTION: Contrary to C, this Rust `clear()` method is non-atomic.
+ /// This mismatches kernel naming convention and corresponds to the C
+ /// function `__cpumask_clear_cpu()`.
#[inline]
pub fn clear(&mut self, cpu: i32) {
// SAFETY: By the type invariant, `self.as_raw` is a valid argument to
@@ -198,15 +202,14 @@ pub struct CpumaskVar {
}
impl CpumaskVar {
- /// Creates an initialized instance of the [`CpumaskVar`].
- pub fn new(_flags: Flags) -> Result<Self, AllocError> {
+ /// Creates a zero-initialized instance of the [`CpumaskVar`].
+ pub fn new_zero(_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.
+ // SAFETY: It is safe to call this method as the reference to `ptr` is valid.
//
// INVARIANT: The associated memory is freed when the `CpumaskVar` goes out of
// scope.
@@ -222,20 +225,19 @@ pub fn new(_flags: Flags) -> Result<Self, AllocError> {
})
}
- /// Creates an uninitialized instance of the [`CpumaskVar`].
+ /// Creates an instance of the [`CpumaskVar`].
///
/// # Safety
///
/// The caller must ensure that the returned [`CpumaskVar`] is properly initialized before
/// getting used.
- unsafe fn new_uninit(_flags: Flags) -> Result<Self, AllocError> {
+ unsafe 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.
+ // SAFETY: It is safe to call this method as the reference to `ptr` is valid.
//
// INVARIANT: The associated memory is freed when the `CpumaskVar` goes out of
// scope.
> > + /// Creates a mutable reference to an existing `struct cpumask_var_t` 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_var_t) -> &'a mut Self {
>
> The 'from' (wrt cpumasks) has a special meaning: search for a cpu
> starting from a given one. This 'from_raw' may confuse readers. Have
> you any other name for it in mind?
'from_raw' is widely used in Rust for similar methods, though I do
understand your concerns.
Danilo / Miguel, what do you suggest I rename these to ?
> > + // SAFETY: Guaranteed by the safety requirements of the function.
> > + //
> > + // INVARIANT: The caller ensures that `ptr` is valid for writing and remains valid for the
> > + // lifetime of the returned reference.
> > + unsafe { &mut *ptr.cast() }
> > + }
> > + /// Clones cpumask.
> > + pub fn try_clone(cpumask: &Cpumask) -> Result<Self> {
>
> Just clone(), I think.
The method 'clone()' is already used by the 'Clone' trait [1], and
that's what I wanted to use initially. But 'clone' doesn't return a
'Result'.
--
viresh
[1] https://doc.rust-lang.org/std/clone/trait.Clone.html
Powered by blists - more mailing lists