mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v3] rust: num: document why Integer is sealed
@ 2026-09-16 15:10 Younes Akhouayri via B4 Relay
  2026-09-22 15:04 ` Alexandre Courbot
  0 siblings, 1 reply; 2+ messages in thread
From: Younes Akhouayri via B4 Relay @ 2026-09-16 15:10 UTC (permalink / raw)
  To: Alexandre Courbot, Yury Norov, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, Danilo Krummrich, Daniel Almeida,
	Tamir Duberstein, Onur Özkan
  Cc: rust-for-linux, linux-kernel, Younes Akhouayri

From: Younes Akhouayri <git@younes.io>

Bounded relies on Integer implementations to provide primitive integer
semantics when justifying unchecked operations.

Explain why Integer is sealed next to its private supertrait. Document
the dependency inside fits_within, so callers can rely on its result
without repeating that explanation. Mention sealing in safety comments
that directly rely on integer metadata, shifts or conversions.

Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Suggested-by: Gary Guo <gary@garyguo.net>
Suggested-by: Alexandre Courbot <acourbot@nvidia.com>
Link: https://lore.kernel.org/all/CANiq72m8kycbfQ1teyne-OtB5d5TsUcX_WW0FCkWB3Ayyg-qWw@mail.gmail.com/
Link: https://lore.kernel.org/all/DL88SQWYU15W.2CVZB5NVSSJGK@garyguo.net/
Link: https://lore.kernel.org/all/CANiq72kx-YPPEruOFdu-Dp7GX+8=Et6svG+sQtqEmmF7kpnVyQ@mail.gmail.com/
Link: https://lore.kernel.org/all/DLDSZ09SM8HI.3PHPYGLV2YZBX@nvidia.com/
Signed-off-by: Younes Akhouayri <git@younes.io>
---
Changes in v3:
- Document the sealing dependency inside fits_within and restore its
  callers' original safety comments, following Alexandre's feedback.
- Restore extend's invariant-based comment and remove the repeated
  sealing explanation from the second cast safety comment.
- Link to v2: https://patch.msgid.link/20260908-docs-rust-num-integer-sealing-safety-v2-1-e8c65234db82@younes.io

Changes in v2:
- Shorten the comment explaining why `Integer` is sealed.
- Mention the seal in the `SAFETY` comments that rely on it.
- Link to v1: https://patch.msgid.link/20260906-docs-rust-num-integer-sealing-safety-v1-1-78057391302c@younes.io

To: Alexandre Courbot <acourbot@nvidia.com>
To: Yury Norov <yury.norov@gmail.com>
To: Miguel Ojeda <ojeda@kernel.org>
To: Boqun Feng <boqun@kernel.org>
To: Gary Guo <gary@garyguo.net>
To: Björn Roy Baron <bjorn3_gh@protonmail.com>
To: Benno Lossin <lossin@kernel.org>
To: Andreas Hindborg <a.hindborg@kernel.org>
To: Alice Ryhl <aliceryhl@google.com>
To: Trevor Gross <tmgross@umich.edu>
To: Danilo Krummrich <dakr@kernel.org>
To: Daniel Almeida <daniel.almeida@collabora.com>
To: Tamir Duberstein <tamird@kernel.org>
To: Onur Özkan <work@onurozkan.dev>
Cc: rust-for-linux@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 rust/kernel/num.rs         |  1 +
 rust/kernel/num/bounded.rs | 32 +++++++++++++++++++-------------
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs
index de589792a77a..0449e84a384a 100644
--- a/rust/kernel/num.rs
+++ b/rust/kernel/num.rs
@@ -21,6 +21,7 @@ pub trait Sealed {}
 
 /// Describes core properties of integer types.
 pub trait Integer:
+    // Sealed so that unsafe code can rely on the correctness of its implementations.
     private::Sealed
     + Sized
     + Copy
diff --git a/rust/kernel/num/bounded.rs b/rust/kernel/num/bounded.rs
index 2a2b0a4bca5e..aff35e2eb616 100644
--- a/rust/kernel/num/bounded.rs
+++ b/rust/kernel/num/bounded.rs
@@ -40,6 +40,8 @@ macro_rules! fits_within {
 /// Returns `true` if `value` can be represented with at most `N` bits in a `T`.
 #[inline(always)]
 fn fits_within<T: Integer>(value: T, num_bits: u32) -> bool {
+    // `Integer` is sealed, so the bit width, shifts and equality have primitive integer semantics.
+    // Unsafe code relies on this function correctly checking whether `value` fits.
     fits_within!(value, T, num_bits)
 }
 
@@ -472,8 +474,9 @@ pub fn cast<U>(self) -> Bounded<U, N>
         T: Integer,
         U: Integer<Signedness = T::Signedness>,
     {
-        // SAFETY: The converted value is represented using `N` bits, `U` can contain `N` bits, and
-        // `U` and `T` have the same sign, hence this conversion cannot fail.
+        // SAFETY: `Integer` is sealed, so the bit widths and signedness are correct. The converted
+        // value is represented using `N` bits, `U` can contain `N` bits, and `U` and `T` have the
+        // same sign, hence this conversion cannot fail.
         let value = unsafe { U::try_from(self.get()).unwrap_unchecked() };
 
         // SAFETY: Although the backing type has changed, the value is still represented within
@@ -498,8 +501,9 @@ pub fn shr<const SHIFT: u32, const RES: u32>(self) -> Bounded<T, RES> {
         const_assert!(SHIFT < T::BITS);
         const_assert!(RES + SHIFT >= N);
 
-        // SAFETY: We shift the value right by `SHIFT`, reducing the number of bits needed to
-        // represent the shifted value by as much, and just asserted that `RES >= N - SHIFT`.
+        // SAFETY: `Integer` is sealed, so the shift has primitive integer semantics. We reduce the
+        // number of bits needed to represent the shifted value by `SHIFT`, and just asserted that
+        // `RES >= N - SHIFT`.
         unsafe { Bounded::__new(self.0 >> SHIFT) }
     }
 
@@ -550,8 +554,9 @@ pub fn shr_exact<const SHIFT: u32, const RES: u32>(self) -> Option<Bounded<T, RE
     pub fn shl<const SHIFT: u32, const RES: u32>(self) -> Bounded<T, RES> {
         const_assert!(RES >= N + SHIFT);
 
-        // SAFETY: We shift the value left by `SHIFT`, augmenting the number of bits needed to
-        // represent the shifted value by as much, and just asserted that `RES >= N + SHIFT`.
+        // SAFETY: `Integer` is sealed, so the shift has primitive integer semantics. We augment
+        // the number of bits needed to represent the shifted value by `SHIFT`, and just asserted
+        // that `RES >= N + SHIFT`.
         unsafe { Bounded::__new(self.0 << SHIFT) }
     }
 }
@@ -1028,8 +1033,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 {
-                // SAFETY: The trait bound on `Self` guarantees that `N` bits is
-                // enough to hold any value of the source type.
+                // SAFETY: `Integer` is sealed, so the bit widths and signedness are correct. The
+                // trait bound on `Self` guarantees that `N` bits is enough to hold any value of
+                // the source type.
                 unsafe { Self::__new(T::from(value)) }
             }
         }
@@ -1104,9 +1110,9 @@ impl<T, const N: u32> From<Bounded<T, N>> for $type
             Bounded<T, N>: FitsInXBits<{ <$type as Integer>::BITS as usize }>,
         {
             fn from(value: Bounded<T, N>) -> $type {
-                // SAFETY: The trait bound on `Bounded` ensures that any value it holds (which
-                // is constrained to `N` bits) can fit into the destination type, so this
-                // conversion cannot fail.
+                // SAFETY: `Integer` is sealed, so the bit widths and signedness are correct. The
+                // trait bound on `Bounded` ensures that any value it holds (which is constrained
+                // to `N` bits) can fit into the destination type, so this conversion cannot fail.
                 unsafe { <$type>::try_from(value.get()).unwrap_unchecked() }
             }
         }
@@ -1137,8 +1143,8 @@ impl<T, const N: u32> From<bool> for Bounded<T, N>
     T: Integer<Signedness = Unsigned> + From<bool>,
 {
     fn from(value: bool) -> Self {
-        // SAFETY: A boolean is represented by `0` or `1`, so it fits within any valid unsigned
-        // `Bounded` width.
+        // SAFETY: `Integer` is sealed, so `T` is a primitive unsigned integer. A boolean is
+        // represented by `0` or `1`, so it fits within any valid unsigned `Bounded` width.
         unsafe { Self::__new(T::from(value)) }
     }
 }

---
base-commit: c6709d5e14072d0e3d02f291daee46a199e5dad3
change-id: 20260906-docs-rust-num-integer-sealing-safety-3a0e2967a948

Best regards,
--  
Younes Akhouayri <git@younes.io>



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

* Re: [PATCH v3] rust: num: document why Integer is sealed
  2026-09-16 15:10 [PATCH v3] rust: num: document why Integer is sealed Younes Akhouayri via B4 Relay
@ 2026-09-22 15:04 ` Alexandre Courbot
  0 siblings, 0 replies; 2+ messages in thread
From: Alexandre Courbot @ 2026-09-22 15:04 UTC (permalink / raw)
  To: Younes Akhouayri via B4 Relay, Miguel Ojeda
  Cc: git, Yury Norov, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
	Onur Özkan, rust-for-linux, linux-kernel

On Thu Sep 17, 2026 at 12:10 AM JST, Younes Akhouayri via B4 Relay wrote:
> From: Younes Akhouayri <git@younes.io>
>
> Bounded relies on Integer implementations to provide primitive integer
> semantics when justifying unchecked operations.
>
> Explain why Integer is sealed next to its private supertrait. Document
> the dependency inside fits_within, so callers can rely on its result
> without repeating that explanation. Mention sealing in safety comments
> that directly rely on integer metadata, shifts or conversions.
>
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Suggested-by: Gary Guo <gary@garyguo.net>
> Suggested-by: Alexandre Courbot <acourbot@nvidia.com>
> Link: https://lore.kernel.org/all/CANiq72m8kycbfQ1teyne-OtB5d5TsUcX_WW0FCkWB3Ayyg-qWw@mail.gmail.com/
> Link: https://lore.kernel.org/all/DL88SQWYU15W.2CVZB5NVSSJGK@garyguo.net/
> Link: https://lore.kernel.org/all/CANiq72kx-YPPEruOFdu-Dp7GX+8=Et6svG+sQtqEmmF7kpnVyQ@mail.gmail.com/
> Link: https://lore.kernel.org/all/DLDSZ09SM8HI.3PHPYGLV2YZBX@nvidia.com/
> Signed-off-by: Younes Akhouayri <git@younes.io>
> ---
> Changes in v3:
> - Document the sealing dependency inside fits_within and restore its
>   callers' original safety comments, following Alexandre's feedback.
> - Restore extend's invariant-based comment and remove the repeated
>   sealing explanation from the second cast safety comment.
> - Link to v2: https://patch.msgid.link/20260908-docs-rust-num-integer-sealing-safety-v2-1-e8c65234db82@younes.io
>
> Changes in v2:
> - Shorten the comment explaining why `Integer` is sealed.
> - Mention the seal in the `SAFETY` comments that rely on it.
> - Link to v1: https://patch.msgid.link/20260906-docs-rust-num-integer-sealing-safety-v1-1-78057391302c@younes.io
>
> To: Alexandre Courbot <acourbot@nvidia.com>
> To: Yury Norov <yury.norov@gmail.com>
> To: Miguel Ojeda <ojeda@kernel.org>
> To: Boqun Feng <boqun@kernel.org>
> To: Gary Guo <gary@garyguo.net>
> To: Björn Roy Baron <bjorn3_gh@protonmail.com>
> To: Benno Lossin <lossin@kernel.org>
> To: Andreas Hindborg <a.hindborg@kernel.org>
> To: Alice Ryhl <aliceryhl@google.com>
> To: Trevor Gross <tmgross@umich.edu>
> To: Danilo Krummrich <dakr@kernel.org>
> To: Daniel Almeida <daniel.almeida@collabora.com>
> To: Tamir Duberstein <tamird@kernel.org>
> To: Onur Özkan <work@onurozkan.dev>
> Cc: rust-for-linux@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  rust/kernel/num.rs         |  1 +
>  rust/kernel/num/bounded.rs | 32 +++++++++++++++++++-------------
>  2 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs
> index de589792a77a..0449e84a384a 100644
> --- a/rust/kernel/num.rs
> +++ b/rust/kernel/num.rs
> @@ -21,6 +21,7 @@ pub trait Sealed {}
>  
>  /// Describes core properties of integer types.
>  pub trait Integer:
> +    // Sealed so that unsafe code can rely on the correctness of its implementations.
>      private::Sealed
>      + Sized
>      + Copy
> diff --git a/rust/kernel/num/bounded.rs b/rust/kernel/num/bounded.rs
> index 2a2b0a4bca5e..aff35e2eb616 100644
> --- a/rust/kernel/num/bounded.rs
> +++ b/rust/kernel/num/bounded.rs
> @@ -40,6 +40,8 @@ macro_rules! fits_within {
>  /// Returns `true` if `value` can be represented with at most `N` bits in a `T`.
>  #[inline(always)]
>  fn fits_within<T: Integer>(value: T, num_bits: u32) -> bool {
> +    // `Integer` is sealed, so the bit width, shifts and equality have primitive integer semantics.
> +    // Unsafe code relies on this function correctly checking whether `value` fits.
>      fits_within!(value, T, num_bits)
>  }

So while I think these two hunks are indeed required...

>  
> @@ -472,8 +474,9 @@ pub fn cast<U>(self) -> Bounded<U, N>
>          T: Integer,
>          U: Integer<Signedness = T::Signedness>,
>      {
> -        // SAFETY: The converted value is represented using `N` bits, `U` can contain `N` bits, and
> -        // `U` and `T` have the same sign, hence this conversion cannot fail.
> +        // SAFETY: `Integer` is sealed, so the bit widths and signedness are correct. The converted
> +        // value is represented using `N` bits, `U` can contain `N` bits, and `U` and `T` have the
> +        // same sign, hence this conversion cannot fail.
>          let value = unsafe { U::try_from(self.get()).unwrap_unchecked() };
>  
>          // SAFETY: Although the backing type has changed, the value is still represented within
> @@ -498,8 +501,9 @@ pub fn shr<const SHIFT: u32, const RES: u32>(self) -> Bounded<T, RES> {
>          const_assert!(SHIFT < T::BITS);
>          const_assert!(RES + SHIFT >= N);
>  
> -        // SAFETY: We shift the value right by `SHIFT`, reducing the number of bits needed to
> -        // represent the shifted value by as much, and just asserted that `RES >= N - SHIFT`.
> +        // SAFETY: `Integer` is sealed, so the shift has primitive integer semantics. We reduce the
> +        // number of bits needed to represent the shifted value by `SHIFT`, and just asserted that
> +        // `RES >= N - SHIFT`.
>          unsafe { Bounded::__new(self.0 >> SHIFT) }
>      }
>  
> @@ -550,8 +554,9 @@ pub fn shr_exact<const SHIFT: u32, const RES: u32>(self) -> Option<Bounded<T, RE
>      pub fn shl<const SHIFT: u32, const RES: u32>(self) -> Bounded<T, RES> {
>          const_assert!(RES >= N + SHIFT);
>  
> -        // SAFETY: We shift the value left by `SHIFT`, augmenting the number of bits needed to
> -        // represent the shifted value by as much, and just asserted that `RES >= N + SHIFT`.
> +        // SAFETY: `Integer` is sealed, so the shift has primitive integer semantics. We augment
> +        // the number of bits needed to represent the shifted value by `SHIFT`, and just asserted
> +        // that `RES >= N + SHIFT`.
>          unsafe { Bounded::__new(self.0 << SHIFT) }
>      }
>  }
> @@ -1028,8 +1033,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 {
> -                // SAFETY: The trait bound on `Self` guarantees that `N` bits is
> -                // enough to hold any value of the source type.
> +                // SAFETY: `Integer` is sealed, so the bit widths and signedness are correct. The
> +                // trait bound on `Self` guarantees that `N` bits is enough to hold any value of
> +                // the source type.
>                  unsafe { Self::__new(T::from(value)) }
>              }
>          }
> @@ -1104,9 +1110,9 @@ impl<T, const N: u32> From<Bounded<T, N>> for $type
>              Bounded<T, N>: FitsInXBits<{ <$type as Integer>::BITS as usize }>,
>          {
>              fn from(value: Bounded<T, N>) -> $type {
> -                // SAFETY: The trait bound on `Bounded` ensures that any value it holds (which
> -                // is constrained to `N` bits) can fit into the destination type, so this
> -                // conversion cannot fail.
> +                // SAFETY: `Integer` is sealed, so the bit widths and signedness are correct. The
> +                // trait bound on `Bounded` ensures that any value it holds (which is constrained
> +                // to `N` bits) can fit into the destination type, so this conversion cannot fail.
>                  unsafe { <$type>::try_from(value.get()).unwrap_unchecked() }
>              }
>          }
> @@ -1137,8 +1143,8 @@ impl<T, const N: u32> From<bool> for Bounded<T, N>
>      T: Integer<Signedness = Unsigned> + From<bool>,
>  {
>      fn from(value: bool) -> Self {
> -        // SAFETY: A boolean is represented by `0` or `1`, so it fits within any valid unsigned
> -        // `Bounded` width.
> +        // SAFETY: `Integer` is sealed, so `T` is a primitive unsigned integer. A boolean is
> +        // represented by `0` or `1`, so it fits within any valid unsigned `Bounded` width.

... here I am not sure the remaining mentions give us much. The seal is
a property of `Integer` itself, and is documented there. So an `Integer`
bound already tells the reader that primitive semantics are guaranteed,
just as we don't justify `u32::BITS == 32` in a SAFETY comment.
Repeating that `Integer` is sealed at every site does not add new
information, it just makes the comments more complex.

The comment on `fits_within` is legit though, since it is safe code
whose correctness the unsafe code depends on.

That being said, I know Miguel asked for these mentions on v1, and I
don't want to send you back and forth between two reviewers. Miguel,
would you be OK with keeping this to the trait comment and
`fits_within`?

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

end of thread, other threads:[~2026-09-22 15:04 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-16 15:10 [PATCH v3] rust: num: document why Integer is sealed Younes Akhouayri via B4 Relay
2026-09-22 15:04 ` Alexandre Courbot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®