Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions

["Benno Lossin" <lossin@kernel.org>]

On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
> I'm having a difficult time parsing. Are you suggesting that we guard
> against implementations of `TryInto<u64>` that misbehave?

Let me try a different explanation:

The safety requirement for implementing the `FromStrRadix`:

    /// The member functions of this trait must be implemented according to
    /// their documentation.

Together with the functions of the trait:

    /// Parse `src` to [`Self`] using radix `radix`.
    fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
    
    /// Return the absolute value of [`Self::MIN`].
    fn abs_min() -> u64;
    
    /// Perform bitwise 2's complement on `self`.
    ///
    /// Note: This function does not make sense for unsigned integers.
    fn complement(self) -> Self;

Doesn't make sense. What does it mean to return the "absolute value of
[`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary
type. Similarly for `complement` and `from_str_radix`, what does "Parse
`src` to [`Self`] using radex `radix`" mean? It's not well-defined.

You use this safety requirement in the parsing branch for negative
numbers (the `unsafe` call at the bottom):

    [b'-', rest @ ..] => {
        let (radix, digits) = strip_radix(rest.as_ref());
        // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
        // So if we want to parse negative numbers as positive and
        // later multiply by -1, we have to parse into a larger
        // integer. We choose `u64` as sufficiently large.
        //
        // NOTE: 128 bit integers are not available on all
        // platforms, hence the choice of 64 bits.
        let val =
            u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
                .map_err(|_| EINVAL)?;
    
        if val > Self::abs_min() {
            return Err(EINVAL);
        }
    
        if val == Self::abs_min() {
            return Ok(Self::MIN);
        }
    
        // SAFETY: We checked that `val` will fit in `Self` above.
        let val: Self = unsafe { val.try_into().unwrap_unchecked() };
    
        Ok(val.complement())
    }

But you don't mention that the check is valid due to the safety
requirements of implementing `FromStrRadix`. But even if you did, that
wouldn't mean anything as I explained above.

So let's instead move all of this negation & u64 conversion logic into
the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str`
function doesn't use `unsafe` (there still will be `unsafe` in the
macro, but that is fine, as it's more local and knows the concrete
types).

---
Cheers,
Benno

Here is what I have in mind:

diff --git a/rust/kernel/str/parse_int.rs b/rust/kernel/str/parse_int.rs
index 0754490aec4b..9d6e146c5ea7 100644
--- a/rust/kernel/str/parse_int.rs
+++ b/rust/kernel/str/parse_int.rs
@@ -13,32 +13,16 @@
 // `ParseInt`, that is, prevents downstream users from implementing the
 // trait.
 mod private {
+    use crate::prelude::*;
     use crate::str::BStr;
 
     /// Trait that allows parsing a [`&BStr`] to an integer with a radix.
-    ///
-    /// # Safety
-    ///
-    /// The member functions of this trait must be implemented according to
-    /// their documentation.
-    ///
-    /// [`&BStr`]: kernel::str::BStr
-    // This is required because the `from_str_radix` function on the primitive
-    // integer types is not part of any trait.
-    pub unsafe trait FromStrRadix: Sized {
-        /// The minimum value this integer type can assume.
-        const MIN: Self;
-
+    pub trait FromStrRadix: Sized {
         /// Parse `src` to [`Self`] using radix `radix`.
-        fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
-
-        /// Return the absolute value of [`Self::MIN`].
-        fn abs_min() -> u64;
+        fn from_str_radix(src: &BStr, radix: u32) -> Result<Self>;
 
-        /// Perform bitwise 2's complement on `self`.
-        ///
-        /// Note: This function does not make sense for unsigned integers.
-        fn complement(self) -> Self;
+        /// Tries to convert `value` into [`Self`] and negates the resulting value.
+        fn from_u64_negated(value: u64) -> Result<Self>;
     }
 }
 
@@ -108,19 +92,7 @@ fn from_str(src: &BStr) -> Result<Self> {
                 let val =
                     u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
                         .map_err(|_| EINVAL)?;
-
-                if val > Self::abs_min() {
-                    return Err(EINVAL);
-                }
-
-                if val == Self::abs_min() {
-                    return Ok(Self::MIN);
-                }
-
-                // SAFETY: We checked that `val` will fit in `Self` above.
-                let val: Self = unsafe { val.try_into().unwrap_unchecked() };
-
-                Ok(val.complement())
+                Self::from_u64_negated(val)
             }
             _ => {
                 let (radix, digits) = strip_radix(src);
@@ -131,41 +103,49 @@ fn from_str(src: &BStr) -> Result<Self> {
 }
 
 macro_rules! impl_parse_int {
-    ($ty:ty) => {
-        // SAFETY: We implement the trait according to the documentation.
-        unsafe impl private::FromStrRadix for $ty {
-            const MIN: Self = <$ty>::MIN;
-
-            fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error> {
-                <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
-                    .map_err(|_| EINVAL)
-            }
-
-            fn abs_min() -> u64 {
-                #[allow(unused_comparisons)]
-                if Self::MIN < 0 {
-                    1u64 << (Self::BITS - 1)
-                } else {
-                    0
+    ($($ty:ty),*) => {
+        $(
+            impl private::FromStrRadix for $ty {
+                fn from_str_radix(src: &BStr, radix: u32) -> Result<Self> {
+                    <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
+                        .map_err(|_| EINVAL)
                 }
-            }
 
-            fn complement(self) -> Self {
-                (!self).wrapping_add((1 as $ty))
+                fn from_u64_negated(value: u64) -> Result<Self> {
+                    const ABS_MIN: u64 = {
+                        #[allow(unused_comparisons)]
+                        if <$ty>::MIN < 0 {
+                            1u64 << (Self::BITS - 1)
+                        } else {
+                            0
+                        }
+                    };
+
+                    fn complement(self) -> Self {
+                        (!self).wrapping_add((1 as $ty))
+                    }
+                    if val > ABS_MIN {
+                        return Err(EINVAL);
+                    }
+
+                    if val == ABS_MIN {
+                        return Ok(<$ty>::MIN);
+                    }
+
+                    // SAFETY: The above checks guarantee that `val` fits into `Self`:
+                    // - if `Self` is unsigned, then `ABS_MIN == 0` and thus we have returned above
+                    //   (either `EINVAL` or `MIN`).
+                    // - if `Self` is signed, then we have that `0 <= val < ABS_MIN`. And since
+                    //   `ABS_MIN - 1` fits into `Self` by construction, `val` also does.
+                    let val: Self = unsafe { val.try_into().unwrap_unchecked() };
+
+                    Ok((!val).wrapping_add(1))
+                }
             }
-        }
 
-        impl ParseInt for $ty {}
+            impl ParseInt for $ty {}
+        )*
     };
 }
 
-impl_parse_int!(i8);
-impl_parse_int!(u8);
-impl_parse_int!(i16);
-impl_parse_int!(u16);
-impl_parse_int!(i32);
-impl_parse_int!(u32);
-impl_parse_int!(i64);
-impl_parse_int!(u64);
-impl_parse_int!(isize);
-impl_parse_int!(usize);
+impl_parse_int![i8, u8, i16, u16, i32, u32, i64, u64, isize, usize];