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
+    /// 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
+    ) -> Result<&'a Self, AllocError>
+    {
+        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);
+        }
+        // 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

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
>

On 23/10/2024 11:42, Danilo Krummrich wrote:
>> +    ) -> 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?
Thanks. Would it make sense to squash this function into 
`Page::page_slice_to_page` instead?

/Abdiel

On Wed, Oct 23, 2024 at 01:26:37PM +0300, Abdiel Janulgue wrote:
> On 23/10/2024 11:42, Danilo Krummrich wrote:
> > > +    ) -> 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?
> Thanks. Would it make sense to squash this function into
> `Page::page_slice_to_page` instead?

Probably, though in the future we might also want to add `virt_to_page()` if
to `Page::page_slice_to_page` if it's not a Vmalloc address.

But I think it should be fine to handle both cases in `Page::page_slice_to_page`
directly.

> 
> /Abdiel
>

On Wed, Oct 23, 2024 at 10:42:27AM +0200, Danilo Krummrich wrote:
> 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.

Actually, this was phrased badly, the pointer must also be dereferenced by the
function in some way to become unsafe (which `vmalloc_to_page` does).

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

More specifically, `is_vmalloc_addr()` only checks that the address is between
`VMALLOC_START` and `VMALLOC_END`, but not whether it's pointing to a valid
allocation.

So, this isn't safe unless you make it a safety requirement of your function.

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