| 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
| ||
|
Message-ID: <CAH5fLggb=_Vtk74RONc+pQUSc1XrVi=jdD1=mHNs6-67ZCYB0A@mail.gmail.com>
Date: Wed, 10 Sep 2025 13:05:42 +0200
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 Wed, Sep 10, 2025 at 12:59 PM Onur Özkan <work@...rozkan.dev> wrote:
>
> On Wed, 10 Sep 2025 06:26:27 +0000
> Alice Ryhl <aliceryhl@...gle.com> wrote:
>
> > 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)
> > }
> >
>
> `Error::from_errno` is limited to i32, that's why I followed the
> `TryInto<i32>` approach, but I like this design too.
If you pass an i32 that is not a valid errno, then it becomes EINVAL.
In the case of `isize`, I would say that if a negative isize does not
fit in i32, then that should fall into the same scenario.
> > > 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
>
>
> I left that part intentionally and explained the reason in v2 changelog.
I missed that, ok. I still prefer Ok::<...>()
Powered by blists - more mailing lists