Message-ID: <Zxi7-Gaf0U3xRdQt@pollux>
Date: Wed, 23 Oct 2024 11:03:52 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Abdiel Janulgue <abdiel.janulgue@...il.com>
Cc: rust-for-linux@...r.kernel.org, aliceryhl@...gle.com, dakr@...hat.com,
	linux-kernel@...r.kernel.org, airlied@...hat.com,
	miguel.ojeda.sandonis@...il.com, boqun.feng@...il.com
Subject: Re: [PATCH v2 3/5] rust: page: Extend support to vmalloc_to_page

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

Powered by blists - more mailing lists