* [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).