Received-SPF: pass (google.com: domain of linux-kernel+bounces-108358-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Feedback-ID: iad51458e:Fastmail
Date: Tue, 19 Mar 2024 19:27:38 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Alice Ryhl <aliceryhl@google.com>
Cc: Miguel Ojeda <ojeda@kernel.org>, Matthew Wilcox <willy@infradead.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	Kees Cook <keescook@chromium.org>,
	Alex Gaynor <alex.gaynor@gmail.com>,
	Wedson Almeida Filho <wedsonaf@gmail.com>,
	Gary Guo <gary@garyguo.net>,
	=?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
	Benno Lossin <benno.lossin@proton.me>,
	Andreas Hindborg <a.hindborg@samsung.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Arve =?iso-8859-1?B?SGr4bm5lduVn?= <arve@android.com>,
	Todd Kjos <tkjos@android.com>, Martijn Coenen <maco@android.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	Carlos Llamas <cmllamas@google.com>,
	Suren Baghdasaryan <surenb@google.com>,
	Arnd Bergmann <arnd@arndb.de>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
	Christian Brauner <brauner@kernel.org>
Subject: Re: [PATCH v3 1/4] rust: uaccess: add userspace pointers
Message-ID: <ZfpJmmO2h4PxGshH@boqun-archlinux>
References: <20240311-alice-mm-v3-0-cdf7b3a2049c@google.com>
 <20240311-alice-mm-v3-1-cdf7b3a2049c@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240311-alice-mm-v3-1-cdf7b3a2049c@google.com>

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