Trevor Gross <tmgross@xxxxxxxxx> writes:
> On Mon, Apr 15, 2024 at 3:15 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> 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@xxxxxxxxx>
>> Signed-off-by: Wedson Almeida Filho <wedsonaf@xxxxxxxxx>
>> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
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@xxxxxxxxx/
>> +
>> +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
