Re: [PATCH v2 3/5] rust: page: Extend support to vmalloc_to_page

[Danilo Krummrich <dakr@kernel.org>]

On Wed, Oct 23, 2024 at 01:44:47AM +0300, Abdiel Janulgue wrote:
> Extend Page to support pages that are not allocated by the constructor, for
> example, those returned by vmalloc_to_page(). Since we don't own those pages
> we shouldn't Drop them either. Hence we take advantage of the switch to Opaque
> so we can cast to a Page pointer from a struct page pointer and be able to
> retrieve the reference on an existing struct page mapping. In this case
> no destructor will be called since we are not instantiating a new Page instance.
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/page.rs | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> index a8288c15b860..465928986f4b 100644
> --- a/rust/kernel/page.rs
> +++ b/rust/kernel/page.rs
> @@ -31,11 +31,12 @@ pub const fn page_align(addr: usize) -> usize {
>      (addr + (PAGE_SIZE - 1)) & PAGE_MASK
>  }
>  
> -/// A pointer to a page that owns the page allocation.
> +/// A pointer to a page that may own the page allocation.
>  ///
>  /// # Invariants
>  ///
> -/// The pointer is valid, and has ownership over the page.
> +/// The pointer is valid, and has ownership over the page if the page is allocated by this
> +/// abstraction.
>  #[repr(transparent)]
>  pub struct Page {
>      page: Opaque<bindings::page>,
> @@ -88,6 +89,33 @@ pub fn alloc_page(flags: Flags) -> Result<Owned<Self>, AllocError> {
>          Ok(unsafe { Owned::to_owned(ptr) })
>      }
>  
> +    /// This is just a wrapper to vmalloc_to_page which returns an existing page mapping, hence

In documentation, try to avoid filler words, such as "just". Better say
something like:

"This is an abstraction around the C `vmalloc_to_page()` function. Note that by
a call to this function the caller doesn't take ownership of the returned `Page`
[...]."

> +    /// we don't take ownership of the page. Returns an error if the pointer is null or if it
> +    /// is not returned by vmalloc().
> +    pub fn vmalloc_to_page<'a>(
> +        cpu_addr: *const core::ffi::c_void

When you have a raw pointer argument in your function it becomes unsafe by
definition.

I also think it would also be better to pass a `NonNull<u8>` instead.

> +    ) -> Result<&'a Self, AllocError>

Please don't use `AllocError`. We're not allocating anything here.

Anyway, do we need this as a separate function at all?

> +    {
> +        if cpu_addr.is_null() {
> +            return Err(AllocError);
> +        }
> +        // SAFETY: We've checked that the pointer is not null, so it is safe to call this method.
> +        if unsafe { !bindings::is_vmalloc_addr(cpu_addr) } {
> +            return Err(AllocError);
> +        }
> +        // SAFETY: We've initially ensured the pointer argument to this function is not null and
> +        // checked for the requirement the the buffer passed to it should be allocated by vmalloc,
> +        // so it is safe to call this method.
> +        let page = unsafe { bindings::vmalloc_to_page(cpu_addr) };
> +        if page.is_null() {
> +            return Err(AllocError);
> +        }

I think those should all return `EINVAL` instead.

> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::page`.
> +        // SAFETY: We just successfully allocated a page, therefore dereferencing
> +        // the page pointer is valid.
> +        Ok(unsafe { &*page.cast() })
> +    }
> +
>      /// Returns a raw pointer to the page.
>      pub fn as_ptr(&self) -> *mut bindings::page {
>          self.page.get()
> -- 
> 2.43.0
>