Re: [PATCH v3 4/4] rust: add abstraction for `struct page`

[Alice Ryhl]

> 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?

I can add "at a valid page".

By ownership I mean that we are allowed to pass it to __free_page and
that until we do, we can access the page. If you want me to reword this,
please tell me what you want it to say.

> > +// SAFETY: It is safe to transfer page allocations between threads.
> 
> Why?
> 
> > +unsafe impl Send for Page {}

How about:

// SAFETY: Pages have no logic that relies on them staying on a given
// thread, so moving them across threads is safe.

> > +// 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 {}

How about:

// SAFETY: Pages have no logic that relies on them not being accessed
// concurrently, so accessing them concurrently is safe.

> > +        // 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) };

How about:

// SAFETY: Depending on the value of `gfp_flags`, this call may sleep.
// Other than that, it is always safe to call this method.

> > +        // INVARIANT: We checked that the allocation succeeded.
> 
> Doesn't mention ownership.
> 
> > +        Ok(Self { page })

How about:

// INVARIANT: We just successfully allocated a page, so we now have
// ownership of the newly allocated page. We transfer that ownership to
// the new `Page` object.

> > +    /// 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 {

I will say:

/// 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. Depending on the gfp flags and kernel
/// configuration, the pointer may only be mapped on the current thread,
/// and in those cases, dereferencing it on other threads is UB. Other
/// than that, the usual rules for dereferencing a raw pointer apply.
/// (E.g., don't cause data races, the memory may be uninitialized, and
/// so on.)

It's okay to map it multiple times from different threads.

> > +        // 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) };

Why do you say that? The kunmap_local method requires that the address
being unmapped is currently mapped, and that pages are unmapped in
reverse order. The safety comment explains that the page is currently
mapped and that this method cannot be used to unmap them in anything
other than reverse order.

> > +    /// 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.

I will remove the "The pointer is only valid on this task, as this
method uses a local mapping." sentence and copy the same paragraph as
previously (without the `PAGE_SIZE` remark).

Alice