Received-SPF: pass (google.com: domain of linux-kernel+bounces-37795-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Valentin Obst <kernel@valentinobst.de>
To: aliceryhl@google.com
Cc: a.hindborg@samsung.com,
	akpm@linux-foundation.org,
	alex.gaynor@gmail.com,
	arnd@arndb.de,
	arve@android.com,
	benno.lossin@proton.me,
	bjorn3_gh@protonmail.com,
	boqun.feng@gmail.com,
	brauner@kernel.org,
	cmllamas@google.com,
	gary@garyguo.net,
	gregkh@linuxfoundation.org,
	joel@joelfernandes.org,
	keescook@chromium.org,
	linux-kernel@vger.kernel.org,
	linux-mm@kvack.org,
	maco@android.com,
	ojeda@kernel.org,
	rust-for-linux@vger.kernel.org,
	surenb@google.com,
	tkjos@android.com,
	viro@zeniv.linux.org.uk,
	wedsonaf@gmail.com,
	Valentin Obst <kernel@valentinobst.de>
Subject: Re: [PATCH 1/3] rust: add userspace pointers
Date: Thu, 25 Jan 2024 00:12:35 +0100
Message-ID: <20240124231235.6183-1-kernel@valentinobst.de>
In-Reply-To: <20240124-alice-mm-v1-1-d1abcec83c44@google.com>
References: <20240124-alice-mm-v1-1-d1abcec83c44@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
UI-OutboundReport: notjunk:1;M01:P0:f29DWwXpfyE=;I1MCVoVH7iehyuAN6gh7kinA6E6
 zjgAy4XiAQLA6OLLraBfnkxJnnybDI9hU5PYprLRiQEt5kCLLKKfmafd6vmx9o4cuSPrERkjI
 IuB1J7gngdEebE+a9fQnbxCC/k3EeIs84qM9lfUCG52x4Vw1fRRrFH+Ys8QdopqxVz8wx0Kmx
 IzfO4vWPyL2cVWdabWJ+CuGDQZWQewpBmXuzPhFQFODC9nZNRt06i538Yz7TperCIXt1BrVPb
 QpCgpOMSBlZDwSAeDfIwRKuvDEt1P6cpu3EkGFvoIZaP71pYg0iB4mpJMDZ3f4qIrE84FdwLN
 BQ+tx6XAomHSQ+zy91x0lxW40h7BpPnZ6E9VwcrYg3/igunWFeVT7LXvpfOgLG7LwZAksCcgQ
 s7Mlx+TUdsUjuKqnIIh8CJ7ZM/p5zqDTF0YVG+dAPIYLwt+ovbLoleWUG55p8lSytJ4FYO1gp
 /BfIIMfpWgSLhHyKXo0WxtM8dEYE8bFTROgfTT0dYjcYoFNkxQyoiMw7ADGyQm0h0qZZ6gPN9
 sKBpHWTJjmSpibHSCUJVe5DpBFwBZEr92BnPjBEjjrse+ih+pXe4QGVYOB4yJ6h7MZNMj9I+p
 TrIjVTUnowSIniRPpr1E1I8avxUFbz+Pvk9/ItrFLivsBhvKvOUf/6g0qd/wGw3xHkZa23OVN
 fS97fzgJV9h9vYN9u+DP/jSm6uuhwkFZITe3lb6w0PVJFIZKKExDVDfBJSvjLbCUZbyh0o2CZ
 itM44KLUls6x5F1x6g5mkRS1x8L+5rP5hNvupATjJQBIEotaHDbCuSUclxXkwSvbE+OMUEPqs
 oNc+kYtCCqt8DwrStyDfKaoG/GjkC1gOXJbSD3P21LtRvDz1fys6Kk+KmShoGYjlE+RR4w6XF
 KxOFDSgJ2E8C8NWOQRKmoctRsER+QV/HNZNg=

> +//! User pointers.
> +//!
> +//! C header: [`include/linux/uaccess.h`](../../../../include/linux/uaccess.h)
> +

nit: could this be using srctree-relative links?

> +/// The maximum length of a operation using `copy_[from|to]_user`.

nit: 'a' -> 'an'

> +///
> +/// If a usize is not greater than this constant, then casting it to `c_ulong`
> +/// is guaranteed to be lossless.

nit: could this be `usize` or [`usize`]. Maybe would also be clearer to
say "... a value of type [`usize`] is smaller than ..."

> +///
> +/// These APIs are designed to make it difficult to accidentally write TOCTOU
> +/// bugs. Every time you read from a memory location, the pointer is advanced by

Maybe makes sense to also introduce the abbreviation TOCTOU in the type
documentation when it is first used.

> +    /// Reads the entirety of the user slice.
> +    ///
> +    /// Returns `EFAULT` if the address does not currently point to
> +    /// mapped, readable memory.
> +    pub fn read_all(self) -> Result<Vec<u8>> {
> +        self.reader().read_all()
> +    }

If I understand it correctly, the function will return `EFAULT` if _any_
address in the interval `[self.0, self.0 + self.1)` does not point to
mapped, readable memory. Maybe the docs could be more explicit.

> +        // Since this is not a pointer to a valid object in our program,
> +        // we cannot use `add`, which has C-style rules for defined
> +        // behavior.
> +        self.0 = self.0.wrapping_add(len);

If I understand it correctly, you are using 'valid object' to refer to
an 'allocated object' [1] as this is what the `add` method's docs
refer to [2]. In that case it might be better to use the latter term as
it has a defined meaning. Also see [3] and [4] which are about making it
more precise.

[1]: https://doc.rust-lang.org/core/ptr/index.html#allocated-object
[2]: https://doc.rust-lang.org/core/primitive.pointer.html#method.add
[3]: https://github.com/rust-lang/rust/pull/116675
[4]: https://github.com/rust-lang/unsafe-code-guidelines/issues/465