Message-ID: <CAH5fLgi_iU3nDE-gJ56s8CPznWvC0T4P5M0dVx1zO61kmVGNgQ@mail.gmail.com>
Date: Thu, 8 Feb 2024 14:46:13 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Trevor Gross <tmgross@...ch.edu>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...sung.com>, 
	Kees Cook <keescook@...omium.org>, Al Viro <viro@...iv.linux.org.uk>, 
	Andrew Morton <akpm@...ux-foundation.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Arve Hjønnevåg <arve@...roid.com>, 
	Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>, 
	Joel Fernandes <joel@...lfernandes.org>, Carlos Llamas <cmllamas@...gle.com>, 
	Suren Baghdasaryan <surenb@...gle.com>, Arnd Bergmann <arnd@...db.de>, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH 3/3] rust: add abstraction for `struct page`

On Thu, Feb 1, 2024 at 7:02 AM Trevor Gross <tmgross@...ch.edu> wrote:
>
> On Wed, Jan 24, 2024 at 6:22 AM Alice Ryhl <aliceryhl@...gle.com> wrote:
> > +/// A pointer to a page that owns the page allocation.
> > +///
> > +/// # Invariants
> > +///
> > +/// The pointer points at a page, and has ownership over the page.
> > +pub struct Page {
> > +    page: NonNull<bindings::page>,
> > +}
>
> Shouldn't this be UnsafeCell / Opaque? Since `struct page` contains locks.

That only matters when we use a reference. Here, it's behind a raw pointer.

> > +// SAFETY: It is safe to transfer page allocations between threads.
> > +unsafe impl Send for Page {}
> > +
> > +// SAFETY: Calling `&self` methods on this type in parallel is safe. It might
> > +// allow you to perform a data race on bytes stored in the page, but we treat
> > +// this like data races on user pointers.
> > +unsafe impl Sync for Page {}
>
> These races should probably be in the Page docs, rather than pointing
> to user pointers.

New safety comment:

SAFETY: As long as the safety requirements for `&self` methods on this
type are followed, there is no problem with calling them in parallel.

> > +impl Page {
> > +    /// Allocates a new set of contiguous pages.
>
> "set of contiguous page" -> "page"?

Thanks, done.

> > +    pub fn new() -> Result<Self, AllocError> {
> > +        // SAFETY: These are the correct arguments to allocate a single page.
> > +        let page = unsafe {
> > +            bindings::alloc_pages(
> > +                bindings::GFP_KERNEL | bindings::__GFP_ZERO | bindings::__GFP_HIGHMEM,
> > +                0,
> > +            )
> > +        };
> > +
> > +        match NonNull::new(page) {
> > +            // INVARIANT: We checked that the allocation above succeeded.
> > +            Some(page) => Ok(Self { page }),
> > +            None => Err(AllocError),
> > +        }
>
> Optionally:
>
>     let page = NonNull::new(page).ok_or(AllocError)?;
>     Ok(Self { page })

Done.

> > +    }
> > +
> > +    /// Returns a raw pointer to the page.
>
> Maybe add ", valid for PAGE_SIZE" or similar to make this obvious.

This is a pointer to the `struct page`, not the actual page data.

> > +    pub fn as_ptr(&self) -> *mut bindings::page {
> > +        self.page.as_ptr()
> > +    }
> > +
> > +    /// Runs a piece of code with this page mapped to an address.
>
> Maybe ", then immediately unmaps the page" to make the entire operation clear.

Ok.

> > +    /// It is up to the caller to use the provided raw pointer correctly.
> > +    pub fn with_page_mapped<T>(&self, f: impl FnOnce(*mut c_void) -> T) -> T {
>
> If there is exclusive access into the page, this signature could be:
>
>     FnOnce(&mut [u8; PAGE_SIZE]) -> T
>
> Otherwise possibly
>
>     FnOnce(*mut [u8; PAGE_SIZE]) -> T
>
> But based on the thread with Boqun it seems there is no synchronized
> access here. In this case, "use the provided raw pointer correctly" or
> the type level docs should clarify what you can and can't rely on with
> pointers into a page.
>
> E.g. if I'm understanding correctly, you can never construct a &T or
> &mut T anywhere in this page unless T is Sync.

We discussed this in the meeting and concluded that we should use *mut u8 here.

> > +    /// 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`.
> > +    pub fn with_pointer_into_page<T>(
> > +        &self,
> > +        off: usize,
> > +        len: usize,
> > +        f: impl FnOnce(*mut u8) -> Result<T>,
> > +    ) -> Result<T> {
>
> Same question about exclusive access
>
>     impl FnOnce(&mut [u8]) -> Result<T>

We discussed this in the meeting. Slices raise all sorts of cans of
worms with uninit and exclusivity, so the raw methods won't use them.

> > +        let bounds_ok = off <= PAGE_SIZE && len <= PAGE_SIZE && (off + len) <= PAGE_SIZE;
> > +
> > +        if bounds_ok {
> > +            self.with_page_mapped(move |page_addr| {
> > +                // SAFETY: The `off` integer is at most `PAGE_SIZE`, so this pointer offset will
> > +                // result in a pointer that is in bounds or one off the end of the page.
> > +                f(unsafe { page_addr.cast::<u8>().add(off) })
> > +            })
> > +        } else {
> > +            Err(EINVAL)
> > +        }
> > +    }
> > +
> > +    /// Maps the page and reads from it into the given buffer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that `dest` is valid for writing `len` bytes.
> > +    pub unsafe fn read(&self, dest: *mut u8, offset: usize, len: usize) -> Result {
>
> Is there a reason not to use a slice just for a destination to read into?

Ditto.

> > +        self.with_pointer_into_page(offset, len, move |from_ptr| {
>
> Nit: do the names from_ptr/to_ptr come from existing binder? src/dst
> seems more common (also dst vs. dest).

Renamed everything to use src/dst

> > +        self.with_pointer_into_page(offset, len, move |to_ptr| {
> > +            // SAFETY: If `with_pointer_into_page` calls into this closure, then
> > +            // it has performed a bounds check and guarantees that `to_ptr` is
> > +            // valid for `len` bytes.
> > +            unsafe { ptr::copy(src, to_ptr, len) };
> > +            Ok(())
> > +        })
> > +    }
> > +
> > +    /// Maps the page and zeroes the given slice.
>
> Mention that this will error with the same conditions as with_pointer_into_page.

That method is private. I will add documentation for this that doesn't
reference with_pointer_into_page.

> > +    pub fn fill_zero(&self, offset: usize, len: usize) -> Result {
> > +        self.with_pointer_into_page(offset, len, move |to_ptr| {
> > +            // SAFETY: If `with_pointer_into_page` calls into this closure, then
> > +            // it has performed a bounds check and guarantees that `to_ptr` is
> > +            // valid for `len` bytes.
> > +            unsafe { ptr::write_bytes(to_ptr, 0u8, len) };
> > +            Ok(())
> > +        })
> > +    }
> > +
> > +    /// Copies data from userspace into this page.
> > +    pub fn copy_into_page(
> > +        &self,
> > +        reader: &mut UserSlicePtrReader,
> > +        offset: usize,
> > +        len: usize,
> > +    ) -> Result {
>
> Maybe copy_from_user_slice or something that includes "user", since
> as-is it sounds like copying a page into another page.
>
> Also, docs should point out the error condition.

Done.

Thanks,
Alice

Powered by blists - more mailing lists