From 5e932f07100f7e7cdb32c8b209b96457fb5303f6 Mon Sep 17 00:00:00 2001 From: Sven Van Asbroeck Date: Thu, 3 Jun 2021 10:59:51 -0400 Subject: [PATCH 1/3] rust/kernel: introduce zero-cost `from_kernel_int_result()` In a previous PR, the type invariant for `Error` is enforced using a runtime check. This is non-zero cost. However we may decide to trust the return value of certain kernel C functions. In such cases, no runtime check is required to enforce the type invariant. So we can return to zero-cost. This patch removes invariant checks from kernel C functions that return 0 on success, or a non-zero errno on failure. Signed-off-by: Sven Van Asbroeck --- rust/kernel/chrdev.rs | 15 ++++++++------- rust/kernel/error.rs | 33 +++++++++++++++++++++++++++++++++ rust/kernel/miscdev.rs | 8 +++++--- rust/kernel/pages.rs | 12 +++++------- rust/kernel/platdev.rs | 8 +++++--- rust/kernel/random.rs | 11 ++++++++--- 6 files changed, 64 insertions(+), 23 deletions(-) diff --git a/rust/kernel/chrdev.rs b/rust/kernel/chrdev.rs index 631405920c29c7..6fbaa2b517c9fe 100644 --- a/rust/kernel/chrdev.rs +++ b/rust/kernel/chrdev.rs @@ -15,7 +15,7 @@ use core::pin::Pin; use crate::bindings; use crate::c_types; -use crate::error::{Error, Result}; +use crate::error::{from_kernel_int_result, Error, Result}; use crate::file_operations; use crate::str::CStr; @@ -60,10 +60,9 @@ impl Cdev { // - [`(*self.0).owner`] will live at least as long as the // module, which is an implicit requirement. let rc = unsafe { bindings::cdev_add(self.0, dev, count) }; - if rc != 0 { - return Err(Error::from_kernel_errno(rc)); - } - Ok(()) + // SAFETY: `bindings::cdev_add()` returns zero on success, + // or a valid negative `errno` on error. + unsafe { from_kernel_int_result(rc) } } } @@ -149,8 +148,10 @@ impl Registration<{ N }> { this.name.as_char_ptr(), ) }; - if res != 0 { - return Err(Error::from_kernel_errno(res)); + // SAFETY: `bindings::alloc_chrdev_region()` returns zero on + // success, or a valid negative `errno` on error. + unsafe { + from_kernel_int_result(res)?; } const NONE: Option = None; this.inner = Some(RegistrationInner { diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 6ac8630c8fb203..cf7184b946c4e2 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -263,3 +263,36 @@ pub(crate) fn from_kernel_err_ptr(ptr: *mut T) -> Result<*mut T> { } Ok(ptr) } + +/// Transform a kernel integer result to a [`Result`]. +/// +/// Some kernel C API functions return a result in the form of an integer: +/// zero if ok, a negative errno on error. This function converts such a +/// return value into an idiomatic [`Result`]. +/// +/// Use this function when the C function only returns 0 on success or +/// negative on error. If the C function returns a useful value on the +/// happy path, use [`from_kernel_int_result_uint`] instead. +/// +/// # Safety +/// +/// `retval` must be non-negative or a valid negative errno (i.e. `retval` must +/// be in `[-MAX_ERRNO..]`). +/// +/// # Examples +/// +/// ```rust,no_run +/// let ret = unsafe { bindings::misc_register(&mut this.mdev) }; +/// // SAFETY: `misc_register` returns zero on success, or a valid +/// // negative errno on failure. +/// unsafe { from_kernel_int_result(ret)?; } +/// ``` +pub(crate) unsafe fn from_kernel_int_result(retval: c_types::c_int) -> Result { + if retval < 0 { + // SAFETY: This condition together with the function precondition + // guarantee that `errno` is a valid negative `errno`. + return Err(unsafe { Error::from_kernel_errno_unchecked(retval) }); + } + Ok(()) +} +} diff --git a/rust/kernel/miscdev.rs b/rust/kernel/miscdev.rs index 3f66202cb66ddf..c98556a2142eeb 100644 --- a/rust/kernel/miscdev.rs +++ b/rust/kernel/miscdev.rs @@ -7,7 +7,7 @@ //! Reference: use crate::bindings; -use crate::error::{Error, Result}; +use crate::error::{from_kernel_int_result, Error, Result}; use crate::file_operations::{FileOpenAdapter, FileOpener, FileOperationsVtable}; use crate::str::CStr; use alloc::boxed::Box; @@ -73,8 +73,10 @@ impl Registration { this.mdev.minor = minor.unwrap_or(bindings::MISC_DYNAMIC_MINOR as i32); let ret = unsafe { bindings::misc_register(&mut this.mdev) }; - if ret < 0 { - return Err(Error::from_kernel_errno(ret)); + // SAFETY: `bindings::alloc_chrdev_region()` returns zero on + // success, or a valid negative `errno` on error. + unsafe { + from_kernel_int_result(ret)?; } this.registered = true; Ok(()) diff --git a/rust/kernel/pages.rs b/rust/kernel/pages.rs index bccacb6d1feb0d..a0f787009ce8cc 100644 --- a/rust/kernel/pages.rs +++ b/rust/kernel/pages.rs @@ -5,8 +5,8 @@ //! TODO: This module is a work in progress. use crate::{ - bindings, c_types, io_buffer::IoBufferReader, user_ptr::UserSlicePtrReader, Error, Result, - PAGE_SIZE, + bindings, c_types, error::from_kernel_int_result, io_buffer::IoBufferReader, + user_ptr::UserSlicePtrReader, Error, Result, PAGE_SIZE, }; use core::{marker::PhantomData, ptr}; @@ -65,11 +65,9 @@ impl Pages { // SAFETY: We check above that the allocation is of order 0. The range of `address` is // already checked by `vm_insert_page`. let ret = unsafe { bindings::vm_insert_page(vma, address as _, self.pages) }; - if ret != 0 { - Err(Error::from_kernel_errno(ret)) - } else { - Ok(()) - } + // SAFETY: `bindings::vm_insert_page()` returns zero on success, + // or a valid negative `errno` on error. + unsafe { from_kernel_int_result(ret) } } /// Copies data from the given [`UserSlicePtrReader`] into the pages. diff --git a/rust/kernel/platdev.rs b/rust/kernel/platdev.rs index 1b79445c04df3c..1322e0e55bf745 100644 --- a/rust/kernel/platdev.rs +++ b/rust/kernel/platdev.rs @@ -8,7 +8,7 @@ use crate::{ bindings, c_types, - error::{Error, Result}, + error::{from_kernel_int_result, Error, Result}, from_kernel_result, of::OfMatchTable, str::CStr, @@ -83,8 +83,10 @@ impl Registration { // `bindings::platform_driver_unregister()`, or // - null. let ret = unsafe { bindings::__platform_driver_register(&mut this.pdrv, module.0) }; - if ret < 0 { - return Err(Error::from_kernel_errno(ret)); + // SAFETY: `bindings::__platform_driver_register()` returns zero on + // success, or a valid negative `errno` on error. + unsafe { + from_kernel_int_result(ret)?; } this.registered = true; Ok(()) diff --git a/rust/kernel/random.rs b/rust/kernel/random.rs index 723a89829f6619..c8c85dd3ae7005 100644 --- a/rust/kernel/random.rs +++ b/rust/kernel/random.rs @@ -6,7 +6,10 @@ use core::convert::TryInto; -use crate::{bindings, c_types, error}; +use crate::{ + bindings, c_types, + error::{self, from_kernel_int_result}, +}; /// Fills a byte slice with random bytes generated from the kernel's CSPRNG. /// @@ -14,8 +17,10 @@ use crate::{bindings, c_types, error}; /// and will block until it is ready. 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)); + // SAFETY: `bindings::wait_for_random_bytes()` returns zero on success, + // or a valid negative `errno` on error. + unsafe { + from_kernel_int_result(res)?; } unsafe { From 79fcf6b0772500fa04bab6a8e647e422a7871268 Mon Sep 17 00:00:00 2001 From: Sven Van Asbroeck Date: Thu, 3 Jun 2021 11:22:43 -0400 Subject: [PATCH 2/3] rust/kernel: introduce zero-cost `from_kernel_int_result_uint()` In a previous PR, the type invariant for `Error` is enforced using a runtime check. This is non-zero cost. However we may decide to trust the return value of certain kernel C functions. In such cases, no runtime check is required to enforce the type invariant. So we can return to zero-cost. This patch removes invariant checks from kernel C functions that return a positive value on success, or a non-zero errno on failure. Signed-off-by: Sven Van Asbroeck --- rust/kernel/error.rs | 34 ++++++++++++++++++++++++++++++++++ rust/kernel/file.rs | 12 ++++++++---- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index cf7184b946c4e2..7d059e2152757a 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -295,4 +295,38 @@ pub(crate) unsafe fn from_kernel_int_result(retval: c_types::c_int) -> Result { } Ok(()) } + +/// Transform a kernel integer result to a [`Result`]. +/// +/// Some kernel C API functions return a result in the form of an integer: +/// zero or positive if ok, a negative errno on error. This function converts +/// such a return value into an idiomatic [`Result`]. +/// +/// Use this function when the C function returns a useful, positive value +/// on success, or negative on error. If the C function only returns 0 on +/// success, use [`from_kernel_int_result`] instead. +/// +/// # Safety +/// +/// `retval` must be non-negative or a valid negative errno (i.e. `retval` must +/// be in `[-MAX_ERRNO..]`). +/// +/// # Examples +/// +/// ```rust,no_run +/// let fd = unsafe { bindings::get_unused_fd_flags(flags) }; +/// // SAFETY: `bindings::get_unused_fd_flags()` returns a non-negative +/// // `fd` on success, or a valid negative `errno` on error. +/// let fd = unsafe { from_kernel_int_result_uint(fd)? }; +/// ``` +pub(crate) unsafe fn from_kernel_int_result_uint( + retval: c_types::c_int, +) -> Result { + if retval < 0 { + // SAFETY: This condition together with the function precondition + // guarantee that `errno` is a valid negative `errno`. + return Err(unsafe { Error::from_kernel_errno_unchecked(retval) }); + } + // CAST: a non-negative `c_types::c_int` always fits in a `c_types::c_uint`. + Ok(retval as c_types::c_uint) } diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs index 262a856fc4910f..70ab2abe9dee6b 100644 --- a/rust/kernel/file.rs +++ b/rust/kernel/file.rs @@ -5,7 +5,11 @@ //! C headers: [`include/linux/fs.h`](../../../../include/linux/fs.h) and //! [`include/linux/file.h`](../../../../include/linux/file.h) -use crate::{bindings, error::Error, Result}; +use crate::{ + bindings, + error::{from_kernel_int_result_uint, Error}, + Result, +}; use core::{mem::ManuallyDrop, ops::Deref}; /// Wraps the kernel's `struct file`. @@ -96,9 +100,9 @@ impl FileDescriptorReservation { /// Creates a new file descriptor reservation. pub fn new(flags: u32) -> Result { let fd = unsafe { bindings::get_unused_fd_flags(flags) }; - if fd < 0 { - return Err(Error::from_kernel_errno(fd)); - } + // SAFETY: `bindings::get_unused_fd_flags()` returns a non-negative + // `fd` on success, or a valid negative `errno` on error. + let fd = unsafe { from_kernel_int_result_uint(fd)? }; Ok(Self { fd: fd as _ }) } From ec1faf3ac524b352e8ba52199a449e117afc04a4 Mon Sep 17 00:00:00 2001 From: Sven Van Asbroeck Date: Thu, 3 Jun 2021 11:26:17 -0400 Subject: [PATCH 3/3] rust/kernel: remove `Error::from_kernel_errno()` dead code warning It turns out that we don't need an `Error` constructor that checks the `errno` invariant at runtime. This is because we trust return values originating from kernel C. And any return values originating from Rust code can be constucted using `Error::` constants, which also do not need a runtime check: ```rust return Err(Error::EBUSY); ``` However, there is still ongoing discussion about the merits and purpose of this function. To facilitate this discussion, keep the function for the time being, and remove the "dead code" warning. Signed-off-by: Sven Van Asbroeck --- rust/kernel/error.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 7d059e2152757a..930f6144617eab 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -64,6 +64,8 @@ impl Error { /// /// It is a bug to pass an out-of-range `errno`. `EINVAL` would /// be returned in such a case. + // TODO: remove `dead_code` marker once an in-kernel client is available. + #[allow(dead_code)] pub(crate) fn from_kernel_errno(errno: c_types::c_int) -> Error { if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 { // TODO: make it a `WARN_ONCE` once available.