Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust: check range and add type invariant to Error (issue #295) #324

Merged
merged 1 commit into from
Jun 3, 2021
Merged

rust: check range and add type invariant to Error (issue #295) #324

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions rust/kernel/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ use core::str::{self, Utf8Error};
///
/// The kernel defines a set of integer generic error codes based on C and
/// POSIX ones. These codes may have a more specific meaning in some contexts.
///
/// # Invariants
///
/// The value is a valid `errno` (i.e. `>= -MAX_ERRNO && < 0`).
#[derive(Clone, Copy, PartialEq, Eq)]
pub struct Error(c_types::c_int);

Expand Down Expand Up @@ -57,7 +61,32 @@ impl Error {
pub const EBADF: Self = Error(-(bindings::EBADF as i32));

/// Creates an [`Error`] from a kernel error code.
pub fn from_kernel_errno(errno: c_types::c_int) -> Error {
///
/// It is a bug to pass an out-of-range `errno`. `EINVAL` would
/// be returned in such a case.
pub(crate) fn from_kernel_errno(errno: c_types::c_int) -> Error {
if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
Copy link
Collaborator

@TheSven73 TheSven73 May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this implementation of #295 is not zero-cost, is that correct? I am wondering if a blanket runtime check to enforce a type invariant is overkill in this context?

The kernel people can do this in C, at runtime, if they wanted to, e.g. by adding an if/pr_warn (or even if/WARN) in the PTR_ERR() macro. But they don't. Does anyone know if this is a conscious choice, or an oversight? Did anyone check if the kernel people care? What happens in the kernel if you do return an negative error int outside -MAX_ERRNO:-1?

Is it a good use of "The Power of Rust" to insert non-zero-cost runtime checks that are possibly too paranoid?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTR_ERR() usually works in tandem with IS_ERR which does a runtime check like this. Because it's for a different semantic (either pointer or error), they don't print warnings.

But I think your point is valid. It may be overkill in some situations, so we have an unchecked version that relies on users to guarantee validity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens in the kernel if you do return an negative error int outside -MAX_ERRNO:-1?

This could lead to a memory safety issue if not careful. If we have a Result<SomePointer>::Err(e) and e is outside MAX_ERRNO range, when we convert this Result into pointer, this will be recognized as a success, and the pointer points to some random kernel memory location.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could lead to a memory safety issue if not careful. If we have a Result<SomePointer>::Err(e) and e is outside MAX_ERRNO range, when we convert this Result into pointer, this will be recognized as a success, and the pointer points to some random kernel memory location.

Kernel C has exactly the same issue: if you call ERR_PTR(x) where x is "negative-er" than -MAX_ERRNO, then you end up with a random pointer that's not IS_ERR(). My point is: it's perfectly possible to check/warn/fix these issues at runtime in C, yet this has not been done by the kernel community.

So my question is: are we "fixing" things that are not seen as a problem upstream? And are we adding non-zero cost runtime checks to accomplish this? How does that help us, or the upstream community?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really a contract between the caller and callee doesn't it? In C there are no simple ways to represent this invariant in the type system, so pretty much the caller just assumes that the return value is sane, so there're no checks. In Rust we have the opportunity to represent this in the type system.

However, I do think that in many cases this check may not be desirable. In that case the unsafe unchecked constructor should be used:

pub fn getrandom(dest: &mut [u8]) -> error::Result {
    let res = unsafe { bindings::wait_for_random_bytes() };
    if res != 0 {
        return Err(error::Error::from_kernel_errno(res));
    }
	/* ... */
}

would be:

pub fn getrandom(dest: &mut [u8]) -> error::Result {
    let res = unsafe { bindings::wait_for_random_bytes() };
    if res != 0 {
		// SAFETY: `wait_for_random_bytes` returns 0 on success and -errno on failure.
        return Err(unsafe { error::Error::from_kernel_errno_unchecked(res) });
    }
	/* ... */
}

I think this is actually good. It would prevent people from just try to convert an arbitrary int to errno, and force the caller to reason about whether the value is indeed an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-topic: @TheSven73 do we need the extern "C" calls to IS_PTR/ERR_PTR? Since we will be having checks against MAX_ERRNO anyway we can just cast them to usize and check them?

Copy link
Member

@ojeda ojeda May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Replying to @TheSven73 -- again GitHub did not update with the new replies :)

Those are good questions. Let me write some context (which you already know, but I am saving these explanations for future discussions) first.

There are two ways we can introduce Rust into the kernel. One is as a C-like replacement, keeping semantics as close as possible (e.g. in the case here). This is what many people may expect, but that would mean we need to go the unsoundness-way, and therefore one of the major advantages of Rust is gone. There are other advantages to Rust, of course, but many may wonder whether "just a nicer language" is worth it.

The other way is to be fully sound. Of course, this means we cannot take for granted even "trivial" things (e.g. like the case here), where the caller is "supposed to do the right thing". Thus, to remain sound, we need to either take a runtime hit or have unsafe blocks (plus their proofs, plus *_unchecked() functions). Nobody likes either, but the upside is huge: it is what leads us to the "holy grail" of being able to write kernel-space memory-safe code; i.e. without the risk of exploits and without moving things to user-space.

So, with that context in mind, the answers would be:

are we "fixing" things that are not seen as a problem upstream?

It is not so much about fixing, but about being a requirement for us in order to remain sound.

And are we adding non-zero cost runtime checks to accomplish this?

That depends: in each instance, we have to decide whether to take a runtime hit or go the unsafe/*_unchecked() way.

I think, in general, the latter is best kept within rust/kernel/ code (since we reap the benefits across many modules) and/or when performance is critical, while the former is best for "leaf" modules (to be able to write them in the safe subset as much as possible) and when performance does not matter much (e.g. initialization of a driver).

In some cases, though, the only way is the runtime hit, because there may be no way to prove the unsafe call is sound.

How does that help us, or the upstream community?

For us, it is what enables us to claim that Rust allows us to write UB-free and memory-safe code/drivers.

For the upstream community, it gives Linux the ability to have kernel-space memory-safe drivers without having to move everything into user-space (or into a multi-rings model). This should allow Linux to improve its security & reliability (due to no UB), maintainability (due to easier reviews of "leaf" modules), etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-topic: @TheSven73 do we need the extern "C" calls to IS_PTR/ERR_PTR? Since we will be having checks against MAX_ERRNO anyway we can just cast them to usize and check them?

Calling the C macros from Rust is probably more robust - if the "error pointer" implementation changes in C, things will continue to work.

Also, the "error pointer" mechanism could differ per-architecture:

linux/include/linux/err.h

Lines 10 to 18 in 7884043

/*
* Kernel pointers have redundant information, so we can use a
* scheme where we can return either an error code or a normal
* pointer with the same return value.
*
* This should be a per-architecture thing, to allow different
* error and pointer decisions.
*/
#define MAX_ERRNO 4095

// TODO: make it a `WARN_ONCE` once available.
crate::pr_warn!(
ojeda marked this conversation as resolved.
Show resolved Hide resolved
"attempted to create `Error` with out of range `errno`: {}",
errno
);
return Error::EINVAL;
}

// INVARIANT: the check above ensures the type invariant
// will hold.
Error(errno)
}

/// Creates an [`Error`] from a kernel error code.
///
/// # Safety
///
/// `errno` must be within error code range (i.e. `>= -MAX_ERRNO && < 0`).
pub(crate) unsafe fn from_kernel_errno_unchecked(errno: c_types::c_int) -> Error {
// INVARIANT: the contract ensures the type invariant
// will hold.
Error(errno)
}

Expand Down Expand Up @@ -227,7 +256,10 @@ pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
// which always fits in an `i16`, as per the invariant above.
// And an `i16` always fits in an `i32`. So casting `err` to
// an `i32` can never overflow, and is always valid.
return Err(Error::from_kernel_errno(err as i32));
//
// SAFETY: `rust_helper_is_err()` ensures `err` is a
// negative value greater-or-equal to `-bindings::MAX_ERRNO`
return Err(unsafe { Error::from_kernel_errno_unchecked(err as i32) });
}
Ok(ptr)
}