linux-kernel - Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

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: <aE2P3OBo1Ufjctxy@Mac.home>
Date: Sat, 14 Jun 2025 08:06:04 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Daniel Almeida <daniel.almeida@...labora.com>,
	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 <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-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

On Sat, Jun 14, 2025 at 10:38:11PM +0900, Alexandre Courbot wrote:
[...]
> > +macro_rules! impl_genmask_fn {
> > +    (
> > +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
> > +        $(#[$genmask_ex:meta])*
> > +    ) => {
> > +        /// Creates a compile-time contiguous bitmask for the given range by
> > +        /// validating the range at runtime.
> > +        ///
> > +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
> > +        /// greater than or equal to the end.
> > +        #[inline]
> > +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
> > +            if range.start >= range.end || range.end > <$ty>::BITS {
> > +                return None;
> > +            }
> 
> From this check I assumed that you interpret `range` as non-inclusive,
> since `range.end == 32` is valid on u32...
> 
> > +            let high = $checked_bit(range.end)?;
> 
> ... however IIUC `checked_bit` will return `None` here in such a case.
> Should the argument be `range.end - 1`?
> 
> Your examples do seem to interpret the range as inclusive though, so
> probably the check should be `|| range.end >= <$ty>::BITS`. But that
> triggers the question, is it ok to use `Range` that way, when its
> documentation specifically states that it is bounded exclusively above?
> We could use `RangeInclusive` to match the semantics, which would
> require us to write the ranges as `0..=7`. At least it is clear that the
> upper bound is inclusive.
> 
> ... or we make the methods generic against `RangeBounds` and allow both
> `Range` and `RangeInclusive` to be used. But I'm concerned that callers
> might use `0..1` thinking it is inclusive while it is not.
> 

I think generic over `RangeBounds` is a good idea, and we should
.is_emtpy() or .contains() instead of comparison + boolean operation
when possible. Seems we need a function to check whether one range
contains another range, which is not available currently?

I would not be worried about callers treating `0..1` as inclusive: this
is a Rust project anyway, we need to learn the correct semantics of
expressions eventually ;-)

Regards,
Boqun

> Thoughts?
> 
> > +            let low = $checked_bit(range.start)?;
> > +            Some((high | (high - 1)) & !(low - 1))
> > +        }
> > +
> > +        /// Creates a compile-time contiguous bitmask for the given range by
> > +        /// validating the range at runtime.
> > +        ///
> > +        /// Returns `0` if the range is invalid, i.e.: if the start is greater
> > +        /// than or equal to the end.
> > +        #[inline]
> > +        pub fn $genmask_unbounded(range: Range<u32>) -> $ty {
> > +            match $genmask_checked(range) {
> > +                Some(v) => v,
> > +                None => 0,
> > +            }
[...]