[PATCH 0/3] Error improvements

[PATCH 0/3] Error improvements

[Miguel Ojeda]

A couple improvements from the past that I rebased prompted by the
discussion at [1].

Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/x/near/535616940 [1]

Miguel Ojeda (3):
  rust: error: improve `Error::from_errno` documentation
  rust: error: improve `to_result` documentation
  rust: error: replace `WARN_ON_ONCE` comment with `debug_assert!`

 rust/kernel/error.rs | 62 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 5 deletions(-)

--
2.51.0

[PATCH 1/3] rust: error: improve `Error::from_errno` documentation

[Miguel Ojeda]

This constructor is public since commit 5ed147473458 ("rust: error:
make conversion functions public"), and we will refer to it from the
documentation of `to_result` in a later commit.

Thus improve its documentation, including adding examples.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/kernel/error.rs | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index a41de293dcd1..c415c3d3a3b6 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -101,8 +101,23 @@ macro_rules! declare_err {
 impl Error {
     /// Creates an [`Error`] from a kernel error code.
     ///
-    /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
-    /// be returned in such a case.
+    /// `errno` must be within error code range (i.e. `>= -MAX_ERRNO && < 0`).
+    ///
+    /// It is a bug to pass an out-of-range `errno`. [`code::EINVAL`] is returned in such a case.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// assert_eq!(Error::from_errno(-1), EPERM);
+    /// assert_eq!(Error::from_errno(-2), ENOENT);
+    /// ```
+    ///
+    /// The following calls are considered a bug:
+    ///
+    /// ```
+    /// assert_eq!(Error::from_errno(0), EINVAL);
+    /// assert_eq!(Error::from_errno(-1000000), EINVAL);
+    /// ```
     pub fn from_errno(errno: crate::ffi::c_int) -> Error {
         if let Some(error) = Self::try_from_errno(errno) {
             error
-- 
2.51.0

Re: [PATCH 1/3] rust: error: improve `Error::from_errno` documentation

[Alexandre Courbot]

On Sat Aug 30, 2025 at 4:22 AM JST, Miguel Ojeda wrote:
> This constructor is public since commit 5ed147473458 ("rust: error:
> make conversion functions public"), and we will refer to it from the
> documentation of `to_result` in a later commit.
>
> Thus improve its documentation, including adding examples.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

This reads very smoothly!

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

> ---
>  rust/kernel/error.rs | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index a41de293dcd1..c415c3d3a3b6 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -101,8 +101,23 @@ macro_rules! declare_err {
>  impl Error {
>      /// Creates an [`Error`] from a kernel error code.
>      ///
> -    /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
> -    /// be returned in such a case.
> +    /// `errno` must be within error code range (i.e. `>= -MAX_ERRNO && < 0`).
> +    ///
> +    /// It is a bug to pass an out-of-range `errno`. [`code::EINVAL`] is returned in such a case.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// assert_eq!(Error::from_errno(-1), EPERM);
> +    /// assert_eq!(Error::from_errno(-2), ENOENT);

Small nit: this leaves a tiny chance of misunderstanding that this
function actually expects literal negative integers. How about using
e.g. `bindings::EPERM` instead of `-1`?

Re: [PATCH 1/3] rust: error: improve `Error::from_errno` documentation

[Miguel Ojeda]

On Sat, Aug 30, 2025 at 2:18 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> Small nit: this leaves a tiny chance of misunderstanding that this
> function actually expects literal negative integers. How about using
> e.g. `bindings::EPERM` instead of `-1`?

Sounds good -- or perhaps `EPERM.to_errno()`?

Thanks!

Cheers,
Miguel

Re: [PATCH 1/3] rust: error: improve `Error::from_errno` documentation

[Alexandre Courbot]

On Sat Aug 30, 2025 at 10:00 PM JST, Miguel Ojeda wrote:
> On Sat, Aug 30, 2025 at 2:18 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> Small nit: this leaves a tiny chance of misunderstanding that this
>> function actually expects literal negative integers. How about using
>> e.g. `bindings::EPERM` instead of `-1`?
>
> Sounds good -- or perhaps `EPERM.to_errno()`?

Looking good to me!

Re: [PATCH 1/3] rust: error: improve `Error::from_errno` documentation

[Benno Lossin]

On Fri Aug 29, 2025 at 9:22 PM CEST, Miguel Ojeda wrote:
> This constructor is public since commit 5ed147473458 ("rust: error:
> make conversion functions public"), and we will refer to it from the
> documentation of `to_result` in a later commit.
>
> Thus improve its documentation, including adding examples.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  rust/kernel/error.rs | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)

[PATCH 2/3] rust: error: improve `to_result` documentation

[Miguel Ojeda]

Core functions like `to_result` should have good documentation.

Thus improve it, including adding an example of how to perform early
returns with it.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/kernel/error.rs | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index c415c3d3a3b6..1ebdb798fd5d 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -390,8 +390,43 @@ fn from(e: core::convert::Infallible) -> Error {
 /// [Rust documentation]: https://doc.rust-lang.org/book/ch09-02-recoverable-errors-with-result.html
 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.
+/// Converts an integer as returned by a C kernel function to a [`Result`].
+///
+/// If the integer is negative, an [`Err`] with an [`Error`] as given by [`Error::from_errno`] is
+/// returned. This means the integer must be `>= -MAX_ERRNO`.
+///
+/// Otherwise, it returns [`Ok`].
+///
+/// It is a bug to pass an out-of-range negative integer. `Err(EINVAL)` is returned in such a case.
+///
+/// # Examples
+///
+/// This function may be used to easily perform early returns with the [`?`] operator when working
+/// with C APIs within Rust abstractions:
+///
+/// ```
+/// # use kernel::error::to_result;
+/// # mod bindings {
+/// #     #![expect(clippy::missing_safety_doc)]
+/// #     use kernel::prelude::*;
+/// #     pub(super) unsafe fn f1() -> c_int { 0 }
+/// #     pub(super) unsafe fn f2() -> c_int { EINVAL.to_errno() }
+/// # }
+/// fn f() -> Result {
+///     // SAFETY: ...
+///     to_result(unsafe { bindings::f1() })?;
+///
+///     // SAFETY: ...
+///     to_result(unsafe { bindings::f2() })?;
+///
+///     // ...
+///
+///     Ok(())
+/// }
+/// # assert_eq!(f(), Err(EINVAL));
+/// ```
+///
+/// [`?`]: https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator
 pub fn to_result(err: crate::ffi::c_int) -> Result {
     if err < 0 {
         Err(Error::from_errno(err))
-- 
2.51.0

Re: [PATCH 2/3] rust: error: improve `to_result` documentation

[Alexandre Courbot]

On Sat Aug 30, 2025 at 4:22 AM JST, Miguel Ojeda wrote:
> Core functions like `to_result` should have good documentation.
>
> Thus improve it, including adding an example of how to perform early
> returns with it.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 2/3] rust: error: improve `to_result` documentation

[Benno Lossin]

On Fri Aug 29, 2025 at 9:22 PM CEST, Miguel Ojeda wrote:
> Core functions like `to_result` should have good documentation.
>
> Thus improve it, including adding an example of how to perform early
> returns with it.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  rust/kernel/error.rs | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)

[PATCH 3/3] rust: error: replace `WARN_ON_ONCE` comment with `debug_assert!`

[Miguel Ojeda]

`warn_on!` support landed recently, and we had a very old comment
about using it when supported to catch invalid inputs passed to
`Error::from_errno`.

However, the kernel policy is that reaching a `WARN_ON` by user
interactions is a CVE, e.g. [1].

Since `from_errno` and other functions that use it such as `to_result`
will be used everywhere, sooner or later a caller may pass an invalid
value due to a user interaction.

Thus, instead, use a debug assertion -- this assumes hitting one of them
is not going to be considered a CVE (which requires
`CONFIG_RUST_DEBUG_ASSERTIONS=y`).

We don't want to potentially panic when testing the examples, thus
convert those to a build-test.

Cc: Greg KH <gregkh@linuxfoundation.org>
Link: https://lore.kernel.org/all/2024092340-renovate-cornflake-4b5e@gregkh/ [1]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/kernel/error.rs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 1ebdb798fd5d..7b9892a46505 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -115,18 +115,20 @@ impl Error {
     /// The following calls are considered a bug:
     ///
     /// ```
+    /// # fn no_run() {
     /// assert_eq!(Error::from_errno(0), EINVAL);
     /// assert_eq!(Error::from_errno(-1000000), EINVAL);
+    /// # }
     /// ```
     pub fn from_errno(errno: crate::ffi::c_int) -> Error {
         if let Some(error) = Self::try_from_errno(errno) {
             error
         } else {
-            // TODO: Make it a `WARN_ONCE` once available.
             crate::pr_warn!(
                 "attempted to create `Error` with out of range `errno`: {}\n",
                 errno
             );
+            debug_assert!(false);
             code::EINVAL
         }
     }
-- 
2.51.0

Re: [PATCH 3/3] rust: error: replace `WARN_ON_ONCE` comment with `debug_assert!`

[Miguel Ojeda]

On Fri, Aug 29, 2025 at 9:23 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Thus, instead, use a debug assertion -- this assumes hitting one of them
> is not going to be considered a CVE (which requires
> `CONFIG_RUST_DEBUG_ASSERTIONS=y`).

Greg: RFC on this -- this is the usual conundrum around `WARN`. I
would like to have an assertion or `WARN`-like entity for developers
that doesn't imply CVEs when hit by user interactions. More generally,
to know that such a config option is OK as long as it is labeled
clearly a debug one like this one (we can document the CVE bit
explicitly if needed).

Thanks!

Cheers,
Miguel

Re: [PATCH 3/3] rust: error: replace `WARN_ON_ONCE` comment with `debug_assert!`

[Greg KH]

On Fri, Aug 29, 2025 at 09:43:24PM +0200, Miguel Ojeda wrote:
> On Fri, Aug 29, 2025 at 9:23 PM Miguel Ojeda <ojeda@kernel.org> wrote:
> >
> > Thus, instead, use a debug assertion -- this assumes hitting one of them
> > is not going to be considered a CVE (which requires
> > `CONFIG_RUST_DEBUG_ASSERTIONS=y`).
> 
> Greg: RFC on this -- this is the usual conundrum around `WARN`. I
> would like to have an assertion or `WARN`-like entity for developers
> that doesn't imply CVEs when hit by user interactions. More generally,
> to know that such a config option is OK as long as it is labeled
> clearly a debug one like this one (we can document the CVE bit
> explicitly if needed).

I do not understand, if there is ANY way that a user can trigger a
WARN() call, then that is considered a "vulnerability" as far as CVE is
concerned and so I need to issue one when it is fixed.  This is entirely
because the panic-on-warn option could have also been enabled.

If you wish to state that CONFIG_RUST_DEBUG_ASSERTIONS=y should NEVER be
used in ANY shipping Linux system, then yes, we can carve out an
exception for this (we do that if lockdep is enabled as that should
never be in a running system, only a development one).  But I thought
that some groups wanted to have that option enabled in their running
systems to provide additional security overall?

And yes, the fact that enabling an option for safety can actually cause
a CVE to be issued is not lost on me as being very odd :)

thanks,

greg k-h

Re: [PATCH 3/3] rust: error: replace `WARN_ON_ONCE` comment with `debug_assert!`

[Miguel Ojeda]

On Sat, Aug 30, 2025 at 8:28 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> If you wish to state that CONFIG_RUST_DEBUG_ASSERTIONS=y should NEVER be
> used in ANY shipping Linux system, then yes, we can carve out an
> exception for this (we do that if lockdep is enabled as that should
> never be in a running system, only a development one).

The config option is meant for development purposes ("debug"). We
don't control all its behavior anyway, because the compiler/stdlib
will also add many assertions (e.g. for unsafe preconditions). So, for
instance, it could easily have a non-trivial performance impact.

For the same reason, it will also change behavior depending on the
compiler version. So, for instance, new assertions in new compiler
versions could have an impact that is not seen in previous versions.

Thus, for this particular config option, we cannot guarantee much, and
the help text already states "This can be used to enable extra
debugging code in development but not in production.".

Having said that, I regularly CI-test our main branches with the
option enabled, and it has worked fine so far.

So if a user actually run with such kind of asserts in production,
because they really want to crash on anything and everything, I don't
see why they couldn't. It may really be that it actually stops an
important exploit from going on. Of course, it may also be that it
elevates a trivial bug into a denial of service elsewhere, but that
risk may be worth it for certain users.

In fact, I would say it is a good thing that certain specialized users
run with it enabled, because then it means they may find potential
bugs for others, and that makes everyone safer in practice.

But I don't know what exact constraints the CVE system puts on you, so
it is hard to assess what the best wording for such an option would
be.

As an addendum: the fact that the compiler is involved is a bit
tangential -- we could have our own "debug" asserts (or "extra
paranoid" asserts) that are independent of the compiler, and we could
have a separate config option etc. But, of course, in this case with
the compiler/stdlib involved it means it is harder to
blanket-recommend for production.

Thanks!

Cheers,
Miguel

Re: [PATCH 3/3] rust: error: replace `WARN_ON_ONCE` comment with `debug_assert!`

[Greg KH]

On Sat, Aug 30, 2025 at 01:07:52PM +0200, Miguel Ojeda wrote:
> On Sat, Aug 30, 2025 at 8:28 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > If you wish to state that CONFIG_RUST_DEBUG_ASSERTIONS=y should NEVER be
> > used in ANY shipping Linux system, then yes, we can carve out an
> > exception for this (we do that if lockdep is enabled as that should
> > never be in a running system, only a development one).
> 
> The config option is meant for development purposes ("debug"). We
> don't control all its behavior anyway, because the compiler/stdlib
> will also add many assertions (e.g. for unsafe preconditions). So, for
> instance, it could easily have a non-trivial performance impact.
> 
> For the same reason, it will also change behavior depending on the
> compiler version. So, for instance, new assertions in new compiler
> versions could have an impact that is not seen in previous versions.
> 
> Thus, for this particular config option, we cannot guarantee much, and
> the help text already states "This can be used to enable extra
> debugging code in development but not in production.".
> 
> Having said that, I regularly CI-test our main branches with the
> option enabled, and it has worked fine so far.
> 
> So if a user actually run with such kind of asserts in production,
> because they really want to crash on anything and everything, I don't
> see why they couldn't. It may really be that it actually stops an
> important exploit from going on. Of course, it may also be that it
> elevates a trivial bug into a denial of service elsewhere, but that
> risk may be worth it for certain users.
> 
> In fact, I would say it is a good thing that certain specialized users
> run with it enabled, because then it means they may find potential
> bugs for others, and that makes everyone safer in practice.
> 
> But I don't know what exact constraints the CVE system puts on you, so
> it is hard to assess what the best wording for such an option would
> be.

There are no "constraints" only a definition of a vulnerability that we
must follow.  And for that, any way that a user could cause a reboot or
panic, without having root privileges, gets assigned a CVE.

One exception being if lockdep or a few other "debugging only" options
are enabled.  Those are explicitly stated by their maintainers that they
should NEVER be enabled in a real system.  For those we do not assign
CVEs as they should never be actually triggered by a user.

So I don't know what to recommend here.  I strongly advise against
adding code to the kernel that can cause users to reboot their boxes if
they do something.  But hey, if developers want to do that, I'll gladly
assign CVEs for when it happens :)

thanks,

greg k-h

Re: [PATCH 3/3] rust: error: replace `WARN_ON_ONCE` comment with `debug_assert!`

[Miguel Ojeda]

On Wed, Sep 3, 2025 at 11:46 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> There are no "constraints" only a definition of a vulnerability that we
> must follow.  And for that, any way that a user could cause a reboot or
> panic, without having root privileges, gets assigned a CVE.
>
> One exception being if lockdep or a few other "debugging only" options
> are enabled.  Those are explicitly stated by their maintainers that they
> should NEVER be enabled in a real system.  For those we do not assign
> CVEs as they should never be actually triggered by a user.
>
> So I don't know what to recommend here.  I strongly advise against
> adding code to the kernel that can cause users to reboot their boxes if
> they do something.  But hey, if developers want to do that, I'll gladly
> assign CVEs for when it happens :)

Sounds good to me, thanks.

These are meant to be debug assertions, so it should be fine. We can
be more explicit in the wording of the config option.

Cheers,
Miguel