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

[Benno Lossin]

On 3/20/24 09:46, Alice Ryhl wrote:
>> 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".

I don't think that helps, what you need as an invariant is that the
pointer is valid.

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

I see, no need to change it.

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

Sounds good.

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

Sounds good.

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

Sounds good.

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

Sounds good.

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

I would simplify and drop "depending on the gfp flags and kernel..." and
just say that the pointer is only valid on the current thread.

Also would it make sense to make the pointer type *mut [u8; PAGE_SIZE]?

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

Do you still need to take care of data races?
So would it be fine to execute this code on two threads in parallel?

     static PAGE: Page = ...; // assume we have a page accessible by both threads
     
     PAGE.with_page_mapped(|ptr| {
         loop {
             unsafe { ptr.write(0) };
             pr_info!("{}", unsafe { ptr.read() });
         }
     });

If this is not allowed, I don't really like the API. As a raw version it
would be fine, but I think we should have a safer version (eg by taking
`&mut self`).

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

Sorry it seems I thought that the safety comment ended after the first
sentence. Can you (re)move that first sentence, since it is not part of
a justification?
The rest is fine.

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

Sounds good.

-- 
Cheers,
Benno