rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] rust: num: bounded: mark __new as unsafe
@ 2025-12-02  3:25 Hsiu Che Yu
  2025-12-03 12:29 ` Alexandre Courbot
  0 siblings, 1 reply; 3+ messages in thread
From: Hsiu Che Yu @ 2025-12-02  3:25 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, Hsiu Che Yu, Miguel Ojeda,
	Yury Norov, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Danilo Krummrich

The `Bounded::__new()` constructor relies on the caller to ensure the
value can be represented within N bits. Failing to uphold this
requirement breaks the type invariant. Mark it as unsafe and document
this requirement in a Safety section to make the contract explicit.

Update all call sites to use unsafe blocks and change their comments
from `INVARIANT:` to `SAFETY:`, as they are now justifying unsafe
operations rather than establishing type invariants.

Fixes: 01e345e82ec3a ("rust: num: add Bounded integer wrapping type")
Reported-by: Miguel Ojeda <ojeda@kernel.org>
Closes: https://github.com/Rust-for-Linux/linux/issues/1211

Signed-off-by: Hsiu Che Yu <yu.whisper.personal@gmail.com>
---
Changes in v2:
- Mark `Bounded::__new` as unsafe and add Safety documentation
- Update all call sites with unsafe blocks and SAFETY comments

Link to v1: https://lore.kernel.org/rust-for-linux/20251201062516.45495-1-yu.whisper.personal@gmail.com/
---
 rust/kernel/num/bounded.rs | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/rust/kernel/num/bounded.rs b/rust/kernel/num/bounded.rs
index f870080af8ac..5838c84f8a53 100644
--- a/rust/kernel/num/bounded.rs
+++ b/rust/kernel/num/bounded.rs
@@ -259,9 +259,9 @@ pub const fn new<const VALUE: $type>() -> Self {
                     assert!(fits_within!(VALUE, $type, N));
                 }
 
-                // INVARIANT: `fits_within` confirmed that `VALUE` can be represented within
+                // SAFETY: `fits_within` confirmed that `VALUE` can be represented within
                 // `N` bits.
-                Self::__new(VALUE)
+                unsafe { Self::__new(VALUE) }
             }
         }
         )*
@@ -284,7 +284,11 @@ impl<T, const N: u32> Bounded<T, N>
     ///
     /// The caller remains responsible for checking, either statically or dynamically, that `value`
     /// can be represented as a `T` using at most `N` bits.
-    const fn __new(value: T) -> Self {
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `value` can be represented within `N` bits.
+    const unsafe fn __new(value: T) -> Self {
         // Enforce the type invariants.
         const {
             // `N` cannot be zero.
@@ -328,8 +332,8 @@ const fn __new(value: T) -> Self {
     /// ```
     pub fn try_new(value: T) -> Option<Self> {
         fits_within(value, N).then(|| {
-            // INVARIANT: `fits_within` confirmed that `value` can be represented within `N` bits.
-            Self::__new(value)
+            // SAFETY: `fits_within` confirmed that `value` can be represented within `N` bits.
+            unsafe { Self::__new(value) }
         })
     }
 
@@ -370,8 +374,8 @@ pub fn from_expr(expr: T) -> Self {
             "Requested value larger than maximal representable value."
         );
 
-        // INVARIANT: `fits_within` confirmed that `expr` can be represented within `N` bits.
-        Self::__new(expr)
+        // SAFETY: `fits_within` confirmed that `expr` can be represented within `N` bits.
+        unsafe { Self::__new(expr) }
     }
 
     /// Returns the wrapped value as the backing type.
@@ -410,9 +414,9 @@ pub const fn extend<const M: u32>(self) -> Bounded<T, M> {
             );
         }
 
-        // INVARIANT: The value did fit within `N` bits, so it will all the more fit within
+        // SAFETY: The value did fit within `N` bits, so it will all the more fit within
         // the larger `M` bits.
-        Bounded::__new(self.0)
+        unsafe { Bounded::__new(self.0) }
     }
 
     /// Attempts to shrink the number of bits usable for `self`.
@@ -466,9 +470,9 @@ pub fn cast<U>(self) -> Bounded<U, N>
         // `U` and `T` have the same sign, hence this conversion cannot fail.
         let value = unsafe { U::try_from(self.get()).unwrap_unchecked() };
 
-        // INVARIANT: Although the backing type has changed, the value is still represented within
+        // SAFETY: Although the backing type has changed, the value is still represented within
         // `N` bits, and with the same signedness.
-        Bounded::__new(value)
+        unsafe { Bounded::__new(value) }
     }
 }
 
@@ -944,9 +948,9 @@ impl<T, const N: u32> From<$type> for Bounded<T, N>
             Self: AtLeastXBits<{ <$type as Integer>::BITS as usize }>,
         {
             fn from(value: $type) -> Self {
-                // INVARIANT: The trait bound on `Self` guarantees that `N` bits is
+                // SAFETY: The trait bound on `Self` guarantees that `N` bits is
                 // enough to hold any value of the source type.
-                Self::__new(T::from(value))
+                unsafe { Self::__new(T::from(value)) }
             }
         }
         )*
@@ -1051,8 +1055,8 @@ impl<T, const N: u32> From<bool> for Bounded<T, N>
     T: Integer + From<bool>,
 {
     fn from(value: bool) -> Self {
-        // INVARIANT: A boolean can be represented using a single bit, and thus fits within any
+        // SAFETY: A boolean can be represented using a single bit, and thus fits within any
         // integer type for any `N` > 0.
-        Self::__new(T::from(value))
+        unsafe { Self::__new(T::from(value)) }
     }
 }
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] rust: num: bounded: mark __new as unsafe
  2025-12-02  3:25 [PATCH v2] rust: num: bounded: mark __new as unsafe Hsiu Che Yu
@ 2025-12-03 12:29 ` Alexandre Courbot
  2025-12-03 14:23   ` Miguel Ojeda
  0 siblings, 1 reply; 3+ messages in thread
From: Alexandre Courbot @ 2025-12-03 12:29 UTC (permalink / raw)
  To: Hsiu Che Yu, Alexandre Courbot
  Cc: linux-kernel, rust-for-linux, Miguel Ojeda, Yury Norov,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich

On Tue Dec 2, 2025 at 12:25 PM JST, Hsiu Che Yu wrote:
> The `Bounded::__new()` constructor relies on the caller to ensure the
> value can be represented within N bits. Failing to uphold this
> requirement breaks the type invariant. Mark it as unsafe and document
> this requirement in a Safety section to make the contract explicit.
>
> Update all call sites to use unsafe blocks and change their comments
> from `INVARIANT:` to `SAFETY:`, as they are now justifying unsafe
> operations rather than establishing type invariants.
>
> Fixes: 01e345e82ec3a ("rust: num: add Bounded integer wrapping type")
> Reported-by: Miguel Ojeda <ojeda@kernel.org>

Wasn't it Alice who said that `__new` should be unsafe?

Although Miguel did create the bug. In any case, maybe add a `Link:` tag
to this:

https://lore.kernel.org/all/aS1qC_ol2XEpZ44b@google.com/

> Closes: https://github.com/Rust-for-Linux/linux/issues/1211
>
> Signed-off-by: Hsiu Che Yu <yu.whisper.personal@gmail.com>

The empty line is not needed.

With this,

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] rust: num: bounded: mark __new as unsafe
  2025-12-03 12:29 ` Alexandre Courbot
@ 2025-12-03 14:23   ` Miguel Ojeda
  0 siblings, 0 replies; 3+ messages in thread
From: Miguel Ojeda @ 2025-12-03 14:23 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Hsiu Che Yu, linux-kernel, rust-for-linux, Miguel Ojeda,
	Yury Norov, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Danilo Krummrich

On Wed, Dec 3, 2025 at 1:29 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> Wasn't it Alice who said that `__new` should be unsafe?

The issue was about making it unsafe -- I added the "Otherwise" branch
to make it a bit more "open", i.e. sometimes I try to make those "good
first issues" in a way that it is not completely automated, e.g. here
so that the new contributor has to think what it means for a function
to be unsafe or not, rather than just converting it. Given Hsiu's
reply in v1, it seems it worked! :)

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-12-03 14:23 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-02  3:25 [PATCH v2] rust: num: bounded: mark __new as unsafe Hsiu Che Yu
2025-12-03 12:29 ` Alexandre Courbot
2025-12-03 14:23   ` Miguel Ojeda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).