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(())
> + }
> +
[...]
