linux-kernel - Re: [PATCH V2 1/2] rust: cpu: Introduce CpuId abstraction

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250610060711.zp6lua4kcwi2z777@vireshk-i7>
Date: Tue, 10 Jun 2025 11:37:11 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: Boqun Feng <boqun.feng@...il.com>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Yury Norov <yury.norov@...il.com>, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 1/2] rust: cpu: Introduce CpuId abstraction

Thanks Miguel for the feedback.

On 09-06-25, 14:01, Miguel Ojeda wrote:
> On Mon, Jun 9, 2025 at 12:51 PM Viresh Kumar <viresh.kumar@...aro.org> wrote:
> > +    pub unsafe fn from_i32_unchecked(id: i32) -> Self {
> 
> Why do we need the `i32` versions?
> 
> Is it just for `bios_limit_callback`? If so, I would just convert there.

It looked like that there are few interfaces in the kernel which
accept an `i32` CPU ID. One example is cpumask layer, where
`cpumask_set_cpu()` expects an `unsigned int` but
`cpumask_{clear|test}_cpu()` expects an `int`.

Though the only user of the `i32` version for now is
`bios_limit_callback`, which I can handle without it too.

What do you suggest ?

> Relatedly, why isn't that callback's type `c_int` on the Rust side?

Sorry, not sure I understood that. Are you talking about type of the
`cpu` argument ?

    extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> kernel::ffi::c_int

> Finally, can we add a `debug_assert!()` on the `_unchecked()` variant,
> since it is something we can easily check?

Changes so far:

diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
index 68d7d86df27c..8d48946090e3 100644
--- a/rust/kernel/cpu.rs
+++ b/rust/kernel/cpu.rs
@@ -23,11 +23,11 @@ pub fn nr_cpu_ids() -> u32 {

 /// The CPU ID.
 ///
-/// Represents a CPU identifier as a wrapper around an `u32`.
+/// Represents a CPU identifier as a wrapper around an [`u32`].
 ///
 /// # Invariants
 ///
-/// The CPU ID must always lie within the range `[0, nr_cpu_ids())`.
+/// The CPU ID lies within the range `[0, nr_cpu_ids())`.
 ///
 /// # Examples
 ///
@@ -54,6 +54,9 @@ impl CpuId {
     /// The caller must ensure that `id` is a valid CPU ID (i.e., `0 <= id < nr_cpu_ids()`).
     #[inline]
     pub unsafe fn from_i32_unchecked(id: i32) -> Self {
+        debug_assert!(id >= 0);
+        debug_assert!((id as u32) < nr_cpu_ids());
+
         // INVARIANT: The function safety guarantees `id` is a valid CPU id.
         Self(id as u32)
     }
@@ -63,8 +66,8 @@ pub fn from_i32(id: i32) -> Option<Self> {
         if id < 0 || id as u32 >= nr_cpu_ids() {
             None
         } else {
-            // SAFETY: `id` has just been checked as a valid CPU ID.
-            Some(unsafe { Self::from_i32_unchecked(id) })
+            // INVARIANT: `id` has just been checked as a valid CPU ID.
+            Some(Self(id as u32))
         }
     }

@@ -75,6 +78,8 @@ pub fn from_i32(id: i32) -> Option<Self> {
     /// The caller must ensure that `id` is a valid CPU ID (i.e., `0 <= id < nr_cpu_ids()`).
     #[inline]
     pub unsafe fn from_u32_unchecked(id: u32) -> Self {
+        debug_assert!(id < nr_cpu_ids());
+
         // INVARIANT: The function safety guarantees `id` is a valid CPU id.
         Self(id)
     }
@@ -84,8 +89,8 @@ pub fn from_u32(id: u32) -> Option<Self> {
         if id >= nr_cpu_ids() {
             None
         } else {
-            // SAFETY: `id` has just been checked as a valid CPU ID.
-            Some(unsafe { Self::from_u32_unchecked(id) })
+            // INVARIANT: `id` has just been checked as a valid CPU ID.
+            Some(Self(id))
         }
     }


-- 
viresh