Re: [PATCH v3 1/4] rust: uaccess: add userspace pointers

[Boqun Feng]

Hi Alice,

I was trying to work on a patch for UserSlice::read_slice(), and I found
a few place that may need some documentation improvements. Please see
below:

On Mon, Mar 11, 2024 at 10:47:13AM +0000, Alice Ryhl wrote:
[...]
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> new file mode 100644
> index 000000000000..020f3847683f
> --- /dev/null
> +++ b/rust/kernel/uaccess.rs
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! User pointers.

Since the type is renamed as UserSlice, maybe:

//! Slices to user space memory regions.

?

> +//!
> +//! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccess.h)
> +
> +use crate::{bindings, error::code::*, error::Result};
> +use alloc::vec::Vec;
> +use core::ffi::{c_ulong, c_void};
> +
> +/// A pointer to an area in userspace memory, which can be either read-only or
> +/// read-write.
> +///
> +/// All methods on this struct are safe: attempting to read or write invalid
> +/// pointers will return `EFAULT`. Concurrent access, *including data races

Probably reword this a little bit:

"All methods on this struct are safe: attempting to read or write on bad
addresses (either out of the bound of the slice or unmapped addresses)
will return `EFAULT`."

, please see below for the reason.

> +/// to/from userspace memory*, is permitted, because fundamentally another
> +/// userspace thread/process could always be modifying memory at the same time
> +/// (in the same way that userspace Rust's [`std::io`] permits data races with
> +/// the contents of files on disk). In the presence of a race, the exact byte
> +/// values read/written are unspecified but the operation is well-defined.
> +/// Kernelspace code should validate its copy of data after completing a read,
> +/// and not expect that multiple reads of the same address will return the same
> +/// value.
> +///
> +/// These APIs are designed to make it difficult to accidentally write TOCTOU
> +/// (time-of-check to time-of-use) bugs. Every time a memory location is read,
> +/// the reader's position is advanced by the read length and the next read will
> +/// start from there. This helps prevent accidentally reading the same location
> +/// twice and causing a TOCTOU bug.
> +///
> +/// Creating a [`UserSliceReader`] and/or [`UserSliceWriter`] consumes the
> +/// `UserSlice`, helping ensure that there aren't multiple readers or writers to
> +/// the same location.
> +///
> +/// If double-fetching a memory location is necessary for some reason, then that
> +/// is done by creating multiple readers to the same memory location, e.g. using
> +/// [`clone_reader`].
> +///

[...]

> +    /// Reads raw data from the user slice into a raw kernel buffer.
> +    ///
> +    /// Fails with `EFAULT` if the read encounters a page fault.

Technically, this is not correct, since normal page faults can happen
during copy_from_user() (for example, userspace memory gets swapped). So
returning `EFAULT` really means the read happens on a bad address, which
also matches `EFAULT`'s definition:

	EFAULT          Bad address (POSIX.1-2001).

so maybe reword this and the similar ones below into something like:

	/// Fails with `EFAULT` if the read happens on a bad address.

Otherwise, people may think that this function just abort whenever there
is a page fault. Thoughts?

Regards,
Boqun

> +    ///
> +    /// # Safety
> +    ///
> +    /// The `out` pointer must be valid for writing `len` bytes.
> +    pub unsafe fn read_raw(&mut self, out: *mut u8, len: usize) -> Result {
> +        if len > self.length {
> +            return Err(EFAULT);
> +        }
> +        let Ok(len_ulong) = c_ulong::try_from(len) else {
> +            return Err(EFAULT);
> +        };
> +        // SAFETY: The caller promises that `out` is valid for writing `len` bytes.
> +        let res = unsafe { bindings::copy_from_user(out.cast::<c_void>(), self.ptr, len_ulong) };
> +        if res != 0 {
> +            return Err(EFAULT);
> +        }
> +        // Userspace pointers are not directly dereferencable by the kernel, so
> +        // we cannot use `add`, which has C-style rules for defined behavior.
> +        self.ptr = self.ptr.wrapping_byte_add(len);
> +        self.length -= len;
> +        Ok(())
> +    }
> +
[...]