linux-kernel - Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

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: <aMEaE2NEU2FctgH2@google.com>
Date: Wed, 10 Sep 2025 06:26:27 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: "Onur Özkan" <work@...rozkan.dev>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	daniel@...lak.dev, dirk.behme@...bosch.com, felipe_life@...e.com, 
	tamird@...il.com, dakr@...nel.org, tmgross@...ch.edu, a.hindborg@...nel.org, 
	lossin@...nel.org, bjorn3_gh@...tonmail.com, gary@...yguo.net, 
	boqun.feng@...il.com, alex.gaynor@...il.com, ojeda@...nel.org
Subject: Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

On Tue, Sep 09, 2025 at 08:00:13PM +0300, Onur Özkan wrote:
> Current `to_result` helper takes a `c_int` and returns `Ok(())` on
> success and this has some issues like:
> 
>     - Callers lose the original return value and often have to store
> 	it in a temporary variable before calling `to_result`.
> 
>     - It only supports `c_int`, which makes callers to unnecessarily
> 	cast when working with other types (e.g. `u16` in phy
> 	abstractions). We even have some places that ignore to use
> 	`to_result` helper because the input doesn't fit in `c_int`
> 	(see [0]).
> 
> [0]: https://lore.kernel.org/all/20250822080252.773d6f54@nimda.home/
> 
> This patch changes `to_result` to be generic and also return the
> original value on success.
> 
> So that the code that previously looked like:
> 
>     let ret = unsafe { bindings::some_ffi_call() };
>     to_result(ret).map(|()| SomeType::new(ret))
> 
> can now be written more directly as:
> 
>     to_result(unsafe { bindings::some_ffi_call() })
> 	.map(|ret| SomeType::new(ret))
> 
> Similarly, code such as:
> 
>     let res: isize = $some_ffi_call();
>     if res < 0 {
> 	    return Err(Error::from_errno(res as i32));
>     }
> 
> can now be used with `to_result` as:
> 
>     to_result($some_ffi_call())?;
> 
> This patch only fixes the callers that broke after the changes on `to_result`.
> I haven't included all the improvements made possible by the new design since
> that could conflict with other ongoing patches [1]. Once this patch is approved
> and applied, I am planning to follow up with creating a "good first issue" on [2]
> for those additional changes.
> 
> [1]: https://lore.kernel.org/rust-for-linux/?q=to_result
> [2]: https://github.com/Rust-for-Linux/linux
> 
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089/topic/x/near/536374456
> Signed-off-by: Onur Özkan <work@...rozkan.dev>

> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index a41de293dcd1..6563ea71e203 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -376,12 +376,19 @@ fn from(e: core::convert::Infallible) -> Error {
>  pub type Result<T = (), E = Error> = core::result::Result<T, E>;
> 
>  /// Converts an integer as returned by a C kernel function to an error if it's negative, and
> -/// `Ok(())` otherwise.
> -pub fn to_result(err: crate::ffi::c_int) -> Result {
> -    if err < 0 {
> -        Err(Error::from_errno(err))
> +/// returns the original value otherwise.
> +pub fn to_result<T>(code: T) -> Result<T>
> +where
> +    T: Copy + TryInto<i32>,
> +{
> +    // Try casting into `i32`.
> +    let casted: crate::ffi::c_int = code.try_into().unwrap_or(0);
> +
> +    if casted < 0 {
> +        Err(Error::from_errno(casted))
>      } else {
> -        Ok(())
> +        // Return the original input value.
> +        Ok(code)
>      }
>  }

I don't think this is the best way to declare this function. The
conversions I would want are:

* i32 -> Result<u32>
* isize -> Result<usize>
* i64 -> Result<u64>

Your commit messages mentions i16, but does the error types even fit in
16 bits? Maybe. But they don't fit in i8. That is to say, I think it
should support all the types larger than i32 (the errors fit in those
types too), but for the ones that are smaller, it might not make sense
if the type is too small. That's the reverse of what you have now.

We probably need a new trait. E.g.:

trait ToResult {
    type Unsigned;
    fn to_result(self) -> Result<Self::Unsigned, Error>;
}

impl ToResult for i32 {
    type Unsigned = u32;
    fn to_result(self) -> Result<u32, Error> {
        ...
    }
}

impl ToResult for isize {
    type Unsigned = usize;
    fn to_result(self) -> Result<usize, Error> {
        ...
    }
}

pub fn to_result<T: ToResult>(code: T) -> Result<T::Unsigned> {
    T::to_result(code)
}

> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 6373fe183b27..22b72ae84c03 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -79,7 +79,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>                  // the destructor of this type deallocates the memory.
>                  // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>                  // misc device.
> -                to_result(unsafe { bindings::misc_register(slot) })
> +                to_result(unsafe { bindings::misc_register(slot) }).map(|_| ())

This still uses the `map` pattern. Please change it too.

Alice