Message-ID: <baee63d9-273a-48aa-b3cc-f15e3782156b@proton.me>
Date: Sat, 16 Mar 2024 20:39:34 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Alice Ryhl <aliceryhl@...gle.com>, Miguel Ojeda <ojeda@...nel.org>, Matthew Wilcox <willy@...radead.org>, Al Viro <viro@...iv.linux.org.uk>, Andrew Morton <akpm@...ux-foundation.org>, Kees Cook <keescook@...omium.org>
Cc: 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>, Andreas Hindborg <a.hindborg@...sung.com>, 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 v3 4/4] rust: add abstraction for `struct page`

On 3/11/24 11:47, Alice Ryhl wrote:
> +/// A pointer to a page that owns the page allocation.
> +///
> +/// # Invariants
> +///
> +/// The pointer points at a page, and has ownership over the page.

Why not "`page` is valid"?
Do you mean by ownership of the page that `page` has ownership of the
allocation, or does that entail any other property/privilege?

> +pub struct Page {
> +    page: NonNull<bindings::page>,
> +}
> +
> +// SAFETY: It is safe to transfer page allocations between threads.

Why?

> +unsafe impl Send for Page {}
> +
> +// SAFETY: As long as the safety requirements for `&self` methods on this type
> +// are followed, there is no problem with calling them in parallel.

Why?

> +unsafe impl Sync for Page {}
> +
> +impl Page {
> +    /// Allocates a new page.
> +    pub fn alloc_page(gfp_flags: flags::gfp_t) -> Result<Self, AllocError> {
> +        // SAFETY: The specified order is zero and we want one page.

This doesn't explain why it is sound to call the function. I expect that
it is always sound to call this function with valid arguments.

> +        let page = unsafe { bindings::alloc_pages(gfp_flags, 0) };
> +        let page = NonNull::new(page).ok_or(AllocError)?;
> +        // INVARIANT: We checked that the allocation succeeded.

Doesn't mention ownership.

> +        Ok(Self { page })
> +    }
> +
> +    /// Returns a raw pointer to the page.
> +    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.
> +    ///
> +    /// It is up to the caller to use the provided raw pointer correctly.

This says nothing about what 'correctly' means. What I gathered from the
implementation is that the supplied pointer is valid for the execution
of `f` for `PAGE_SIZE` bytes.
What other things are you allowed to rely upon?

Is it really OK for this function to be called from multiple threads?
Could that not result in the same page being mapped multiple times? If
that is fine, what about potential data races when two threads write to
the pointer given to `f`?

> +    pub fn with_page_mapped<T>(&self, f: impl FnOnce(*mut u8) -> T) -> T {
> +        // SAFETY: `page` is valid due to the type invariants on `Page`.
> +        let mapped_addr = unsafe { bindings::kmap_local_page(self.as_ptr()) };
> +
> +        let res = f(mapped_addr.cast());
> +
> +        // SAFETY: This unmaps the page mapped above.

This doesn't explain why it is sound.

> +        //
> +        // Since this API takes the user code as a closure, it can only be used
> +        // in a manner where the pages are unmapped in reverse order. This is as
> +        // required by `kunmap_local`.
> +        //
> +        // In other words, if this call to `kunmap_local` happens when a
> +        // different page should be unmapped first, then there must necessarily
> +        // be a call to `kmap_local_page` other than the call just above in
> +        // `with_page_mapped` that made that possible. In this case, it is the
> +        // unsafe block that wraps that other call that is incorrect.
> +        unsafe { bindings::kunmap_local(mapped_addr) };
> +
> +        res
> +    }
> +
> +    /// 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.

This information about the pointer only being valid on this task should
also apply to `with_page_mapped`, right?

> +    ///
> +    /// If `off` and `len` refers to a region outside of this page, then this
> +    /// method returns `EINVAL` and does not call `f`.
> +    ///
> +    /// It is up to the caller to use the provided raw pointer correctly.

Again, please specify what 'correctly' means.

-- 
Cheers,
Benno

> +    pub fn with_pointer_into_page<T>(
> +        &self,
> +        off: usize,
> +        len: usize,
> +        f: impl FnOnce(*mut u8) -> Result<T>,
> +    ) -> Result<T> {
> +        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.add(off) })
> +            })
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }

Powered by blists - more mailing lists