Message-ID: <20240416095333.1108884-1-aliceryhl@google.com>
Date: Tue, 16 Apr 2024 09:53:32 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: tmgross@...ch.edu
Cc: a.hindborg@...sung.com, akpm@...ux-foundation.org, alex.gaynor@...il.com, 
	aliceryhl@...gle.com, arnd@...db.de, arve@...roid.com, benno.lossin@...ton.me, 
	bjorn3_gh@...tonmail.com, boqun.feng@...il.com, brauner@...nel.org, 
	cmllamas@...gle.com, gary@...yguo.net, gregkh@...uxfoundation.org, 
	joel@...lfernandes.org, keescook@...omium.org, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, maco@...roid.com, ojeda@...nel.org, 
	rust-for-linux@...r.kernel.org, surenb@...gle.com, tkjos@...roid.com, 
	viro@...iv.linux.org.uk, wedsonaf@...il.com, willy@...radead.org
Subject: Re: [PATCH v5 4/4] rust: add abstraction for `struct page`

Trevor Gross <tmgross@...ch.edu> writes:
> On Mon, Apr 15, 2024 at 3:15 AM Alice Ryhl <aliceryhl@...gle.com> wrote:
>>
>> Adds a new struct called `Page` that wraps a pointer to `struct page`.
>> This struct is assumed to hold ownership over the page, so that Rust
>> code can allocate and manage pages directly.
>>
>> The page type has various methods for reading and writing into the page.
>> These methods will temporarily map the page to allow the operation. All
>> of these methods use a helper that takes an offset and length, performs
>> bounds checks, and returns a pointer to the given offset in the page.
>>
>> This patch only adds support for pages of order zero, as that is all
>> Rust Binder needs. However, it is written to make it easy to add support
>> for higher-order pages in the future. To do that, you would add a const
>> generic parameter to `Page` that specifies the order. Most of the
>> methods do not need to be adjusted, as the logic for dealing with
>> mapping multiple pages at once can be isolated to just the
>> `with_pointer_into_page` method.
>>
>> Rust Binder needs to manage pages directly as that is how transactions
>> are delivered: Each process has an mmap'd region for incoming
>> transactions. When an incoming transaction arrives, the Binder driver
>> will choose a region in the mmap, allocate and map the relevant pages
>> manually, and copy the incoming transaction directly into the page. This
>> architecture allows the driver to copy transactions directly from the
>> address space of one process to another, without an intermediate copy
>> to a kernel buffer.
>>
>> This code is based on Wedson's page abstractions from the old rust
>> branch, but it has been modified by Alice by removing the incomplete
>> support for higher-order pages, by introducing the `with_*` helpers
>> to consolidate the bounds checking logic into a single place, and by
>> introducing gfp flags.
>>
>> Co-developed-by: Wedson Almeida Filho <wedsonaf@...il.com>
>> Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
>> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> 
> I have a couple questions about naming, and think an example would be
> good for the functions that are trickier to use correctly. But I
> wouldn't block on this, implementation looks good to me.
> 
> Reviewed-by: Trevor Gross <tmgross@...ch.edu>

Thanks for taking a look!

>> +/// Flags for the "get free page" function that underlies all memory allocations.
>> +pub mod flags {
>> +    /// gfp flags.
> 
> Uppercase acronym, maybe with a description:
> 
>     GFP (Get Free Page) flags.
> 
>> +    #[allow(non_camel_case_types)]
>> +    pub type gfp_t = bindings::gfp_t;
> 
> Why not GfpFlags, do we do this elsewhere?
> 
>> +    /// `GFP_KERNEL` is typical for kernel-internal allocations. The caller requires `ZONE_NORMAL`
>> +    /// or a lower zone for direct access but can direct reclaim.
>> +    pub const GFP_KERNEL: gfp_t = bindings::GFP_KERNEL;
>> +    /// `GFP_ZERO` returns a zeroed page on success.
>> +    pub const __GFP_ZERO: gfp_t = bindings::__GFP_ZERO;
>> +    /// `GFP_HIGHMEM` indicates that the allocated memory may be located in high memory.
>> +    pub const __GFP_HIGHMEM: gfp_t = bindings::__GFP_HIGHMEM;
> 
> It feels a bit weird to have dunder constants on the rust side that
> aren't also `#[doc(hidden)]` or just nonpublic. Makes me think they
> are an implementation detail or not really meant to be used - could
> you update the docs if this is the case?

All of this is going away in the next version because it will be based
on [1], which defines the gfp flags type for us.

[1]: https://lore.kernel.org/rust-for-linux/20240328013603.206764-1-wedsonaf@gmail.com/

>> +
>> +impl Page {
>> +    /// Allocates a new page.
> 
> Could you add a small example here?

I can add an example that shows how to pass gfp flags.

>> +    pub fn alloc_page(gfp_flags: flags::gfp_t) -> Result<Self, AllocError> {
>> [...]
>> +    }
>> +
>> +    /// Returns a raw pointer to the page.
> 
> Could you add a note about how the pointer needs to be used correctly,
> if it is for anything more than interfacing with kernel APIs?

I can clarify that it's a pointer to the `struct page` and not the
actual PAGE_SIZE bytes stored in the page, and that it's for use with
the raw C apis. I won't go into more details than that.

>> +    pub fn as_ptr(&self) -> *mut bindings::page {
>> +        self.page.as_ptr()
>> +    }
>> +
>> +    /// Runs a piece of code with this page mapped to an address.
>> +    ///
>> +    /// The page is unmapped when this call returns.
>> +    ///
>> +    /// # Using the raw pointer
>> +    ///
>> +    /// It is up to the caller to use the provided raw pointer correctly. The pointer is valid for
>> +    /// `PAGE_SIZE` bytes and for the duration in which the closure is called. The pointer might
>> +    /// only be mapped on the current thread, and when that is the case, dereferencing it on other
>> +    /// threads is UB. Other than that, the usual rules for dereferencing a raw pointer apply: don't
>> +    /// cause data races, the memory may be uninitialized, and so on.
>> +    ///
>> +    /// If multiple threads map the same page at the same time, then they may reference with
>> +    /// different addresses. However, even if the addresses are different, the underlying memory is
>> +    /// still the same for these purposes (e.g., it's still a data race if they both write to the
>> +    /// same underlying byte at the same time).
>> +    fn with_page_mapped<T>(&self, f: impl FnOnce(*mut u8) -> T) -> T {
>> [...]
>> +    }
> 
> Could you add an example of how to use this correctly?

This is a private function, you're not supposed to use it directly.
Anyone who is modifying this file directly can look at the existing
users for examples.

>> +    /// Runs a piece of code with a raw pointer to a slice of this page, with bounds checking.
>> +    ///
>> +    /// If `f` is called, then it will be called with a pointer that points at `off` bytes into the
>> +    /// page, and the pointer will be valid for at least `len` bytes. The pointer is only valid on
>> +    /// this task, as this method uses a local mapping.
>> +    ///
>> +    /// If `off` and `len` refers to a region outside of this page, then this method returns
>> +    /// `EINVAL` and does not call `f`.
>> +    ///
>> +    /// # Using the raw pointer
>> +    ///
>> +    /// It is up to the caller to use the provided raw pointer correctly. The pointer is valid for
>> +    /// `len` bytes and for the duration in which the closure is called The pointer might only be
>> +    /// mapped on the current thread, and when that is the case, dereferencing it on other threads
>> +    /// is UB. Other than that, the usual rules for dereferencing a raw pointer apply: don't cause
>> +    /// data races, the memory may be uninitialized, and so on.
>> +    ///
>> +    /// If multiple threads map the same page at the same time, then they may reference with
>> +    /// different addresses. However, even if the addresses are different, the underlying memory is
>> +    /// still the same for these purposes (e.g., it's still a data race if they both write to the
>> +    /// same underlying byte at the same time).
> 
> This could probably also use an example. A note about how to select
> between with_pointer_into_page and with_page_mapped would also be nice
> to guide usage, e.g. "prefer with_pointer_into_page for all cases
> except when..."

Same as above. This is a private function.

>> +    fn with_pointer_into_page<T>(
>> +        &self,
>> +        off: usize,
>> +        len: usize,
>> +        f: impl FnOnce(*mut u8) -> Result<T>,
>> +    ) -> Result<T> {
>> [...]
>> +    /// Maps the page and zeroes the given slice.
>> +    ///
>> +    /// This method will perform bounds checks on the page offset. If `offset .. offset+len` goes
>> +    /// outside ot the page, then this call returns `EINVAL`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that this call does not race with a read or write to the same page that
>> +    /// overlaps with this write.
>> +    pub unsafe fn fill_zero(&self, offset: usize, len: usize) -> Result {
>> +        self.with_pointer_into_page(offset, len, move |dst| {
>> +            // SAFETY: If `with_pointer_into_page` calls into this closure, then it has performed a
>> +            // bounds check and guarantees that `dst` is valid for `len` bytes.
>> +            //
>> +            // There caller guarantees that there is no data race.
>> +            unsafe { ptr::write_bytes(dst, 0u8, len) };
>> +            Ok(())
>> +        })
>> +    }
> 
> Could this be named `fill_zero_raw` to leave room for a safe
> `fill_zero(&mut self, ...)`?

I suppose I can rename these _raw as well.

>> +    /// Copies data from userspace into this page.
>> +    ///
>> +    /// This method will perform bounds checks on the page offset. If `offset .. offset+len` goes
>> +    /// outside ot the page, then this call returns `EINVAL`.
>> +    ///
>> +    /// Like the other `UserSliceReader` methods, data races are allowed on the userspace address.
>> +    /// However, they are not allowed on the page you are copying into.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that this call does not race with a read or write to the same page that
>> +    /// overlaps with this write.
>> +    pub unsafe fn copy_from_user_slice(
>> +        &self,
>> +        reader: &mut UserSliceReader,
>> +        offset: usize,
>> +        len: usize,
>> +    ) -> Result {
>> +        self.with_pointer_into_page(offset, len, move |dst| {
>> +            // SAFETY: If `with_pointer_into_page` calls into this closure, then it has performed a
>> +            // bounds check and guarantees that `dst` is valid for `len` bytes. Furthermore, we have
>> +            // exclusive access to the slice since the caller guarantees that there are no races.
>> +            reader.read_raw(unsafe { core::slice::from_raw_parts_mut(dst.cast(), len) })
>> +        })
>> +    }
>> +}
> 
> Same as above, `copy_from_user_slice_raw` would leave room for a safe API.

Okay

Alice

Powered by blists - more mailing lists