[PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Abdiel Janulgue]

This series aims to add support for pages that are not constructed by an
instance of the rust Page abstraction, for example those returned by
vmalloc_to_page() or virt_to_page().

Changes sinve v3:
- Use the struct page's reference count to decide when to free the
  allocation (Alice Ryhl, Boqun Feng).
- Make Page::page_slice_to_page handle virt_to_page cases as well
  (Danilo Krummrich).
- Link to v2: https://lore.kernel.org/lkml/20241022224832.1505432-1-abdiel.janulgue@gmail.com/

Changes since v2:
- Use Owned and Ownable types for constructing Page as suggested in
  instad of using ptr::read().
- Link to v1: https://lore.kernel.org/rust-for-linux/20241007202752.3096472-1-abdiel.janulgue@gmail.com/

Abdiel Janulgue (2):
  rust: page: use the page's reference count to decide when to free the
    allocation
  rust: page: Extend support to existing struct page mappings

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/page.c             |  20 +++++
 rust/kernel/page.rs             | 135 ++++++++++++++++++++++++++++----
 3 files changed, 142 insertions(+), 14 deletions(-)


base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb
-- 
2.43.0

[PATCH v3 1/2] rust: page: use the page's reference count to decide when to free the allocation

[Abdiel Janulgue]

Ensure pages returned by the constructor are always reference counted.
This requires that we replace the page pointer wrapper with Opaque instead
of NonNull to make it possible to cast to a Page pointer from a raw struct
page pointer which is needed to create an ARef instance.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers/page.c             | 10 +++++++++
 rust/kernel/page.rs             | 38 ++++++++++++++++++++++-----------
 3 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index a80783fcbe04..daa3225a185f 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -15,6 +15,7 @@
 #include <linux/firmware.h>
 #include <linux/jiffies.h>
 #include <linux/mdio.h>
+#include <linux/mm.h>
 #include <linux/phy.h>
 #include <linux/refcount.h>
 #include <linux/sched.h>
diff --git a/rust/helpers/page.c b/rust/helpers/page.c
index b3f2b8fbf87f..48d4481c1e33 100644
--- a/rust/helpers/page.c
+++ b/rust/helpers/page.c
@@ -17,3 +17,13 @@ void rust_helper_kunmap_local(const void *addr)
 {
 	kunmap_local(addr);
 }
+
+void rust_helper_put_page(struct page *page)
+{
+	put_page(page);
+}
+
+void rust_helper_get_page(struct page *page)
+{
+	get_page(page);
+}
diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index fdac6c375fe4..fdf7ee203597 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -8,6 +8,7 @@
     error::code::*,
     error::Result,
     uaccess::UserSliceReader,
+    types::{Opaque, ARef},
 };
 use core::ptr::{self, NonNull};
 
@@ -30,13 +31,14 @@ 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 reference-counted page.
 ///
 /// # Invariants
 ///
-/// The pointer is valid, and has ownership over the page.
+/// The pointer is valid.
+#[repr(transparent)]
 pub struct Page {
-    page: NonNull<bindings::page>,
+    page: Opaque<bindings::page>,
 }
 
 // SAFETY: Pages have no logic that relies on them staying on a given thread, so moving them across
@@ -71,19 +73,23 @@ impl Page {
     /// let page = Page::alloc_page(GFP_KERNEL | __GFP_ZERO)?;
     /// # Ok(()) }
     /// ```
-    pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
+    pub fn alloc_page(flags: Flags) -> Result<ARef<Self>, AllocError> {
         // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
         // is always safe to call this method.
         let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
-        let page = NonNull::new(page).ok_or(AllocError)?;
-        // 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.
-        Ok(Self { page })
+        if page.is_null() {
+            return Err(AllocError);
+        }
+        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
+        let ptr = page.cast::<Self>();
+        // INVARIANT: We just successfully allocated a page, ptr points to the new `Page` object.
+        // SAFETY: According to invariant above ptr is valid.
+        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) })
     }
 
     /// Returns a raw pointer to the page.
     pub fn as_ptr(&self) -> *mut bindings::page {
-        self.page.as_ptr()
+        self.page.get()
     }
 
     /// Runs a piece of code with this page mapped to an address.
@@ -252,9 +258,15 @@ pub unsafe fn copy_from_user_slice_raw(
     }
 }
 
-impl Drop for Page {
-    fn drop(&mut self) {
-        // SAFETY: By the type invariants, we have ownership of the page and can free it.
-        unsafe { bindings::__free_pages(self.page.as_ptr(), 0) };
+// SAFETY: Instances of `Page` are always reference-counted.
+unsafe impl crate::types::AlwaysRefCounted for Page {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+        unsafe { bindings::get_page(self.as_ptr()) };
+    }
+
+    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::put_page(obj.cast().as_ptr()) }
     }
 }
-- 
2.43.0

[PATCH v3 2/2] rust: page: Extend support to existing struct page mappings

[Abdiel Janulgue]

Extend Page to support pages that are not allocated by the constructor,
for example, those returned by vmalloc_to_page() or virt_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.

The new page_slice_to_page wrapper ensures that it explicity accepts
only page-sized chunks.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/helpers/page.c | 10 +++++
 rust/kernel/page.rs | 97 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/rust/helpers/page.c b/rust/helpers/page.c
index 48d4481c1e33..784563924b83 100644
--- a/rust/helpers/page.c
+++ b/rust/helpers/page.c
@@ -27,3 +27,13 @@ void rust_helper_get_page(struct page *page)
 {
 	get_page(page);
 }
+
+struct page *rust_helper_virt_to_page(const void *kaddr)
+{
+	return virt_to_page(kaddr);
+}
+
+bool rust_helper_virt_addr_valid(const void *kaddr)
+{
+	return virt_addr_valid(kaddr);
+}
diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index fdf7ee203597..d0a896f53afb 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -3,7 +3,7 @@
 //! Kernel page allocation and management.
 
 use crate::{
-    alloc::{AllocError, Flags},
+    alloc::{AllocError, Allocator, Flags, VVec, KVec, KVVec, Vec, flags::*},
     bindings,
     error::code::*,
     error::Result,
@@ -87,6 +87,49 @@ pub fn alloc_page(flags: Flags) -> Result<ARef<Self>, AllocError> {
         Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) })
     }
 
+    /// Create a page object from a buffer which is associated with an existing C `struct page`.
+    ///
+    /// This function ensures it takes a page-sized buffer as represented by `PageSlice`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::page::*;
+    ///
+    /// let somedata: [u8; PAGE_SIZE * 2] = [0; PAGE_SIZE * 2];
+    /// let buf: &[u8] = &somedata;
+    /// let pages: VVec<PageSlice> = buf.try_into()?;
+    /// let page = Page::page_slice_to_page(&pages[0])?;
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self>
+    {
+        let ptr: *const core::ffi::c_void = page.0.as_ptr() as _;
+        if ptr.is_null() {
+            return Err(EINVAL)
+        }
+        // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method.
+        let page = if unsafe { bindings::is_vmalloc_addr(ptr) } {
+            // SAFETY: We've checked that `ptr` is non-null and within the vmalloc range, hence
+            // it's safe to call this method.
+            unsafe { bindings::vmalloc_to_page(ptr) }
+        // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method.
+        } else if unsafe { bindings::virt_addr_valid(ptr) } {
+            // SAFETY: We've checked that `ptr` is non-null and a valid virtual address, hence
+            // it's safe to call this method.
+            unsafe { bindings::virt_to_page(ptr) }
+        } else {
+            ptr::null_mut()
+        };
+        if page.is_null() {
+            return Err(EINVAL);
+        }
+        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::page`.
+        // SAFETY: We just successfully retrieved an existing `bindings::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()
@@ -270,3 +313,55 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
         unsafe { bindings::put_page(obj.cast().as_ptr()) }
     }
 }
+
+/// A page-aligned, page-sized object.
+///
+/// This is used for convenience to convert a large buffer into an array of page-sized chunks
+/// allocated with the kernel's allocators which can then be used in the
+/// `Page::page_slice_to_page` wrapper.
+///
+// FIXME: This should be `PAGE_SIZE`, but the compiler rejects everything except a literal
+// integer argument for the `repr(align)` attribute.
+#[repr(align(4096))]
+pub struct PageSlice([u8; PAGE_SIZE]);
+
+fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> {
+    let mut k = Vec::<PageSlice, A>::new();
+    let pages = page_align(val.len()) >> PAGE_SHIFT;
+    match k.reserve(pages, GFP_KERNEL) {
+        Ok(()) => {
+            // SAFETY: from above, the length should be equal to the vector's capacity
+            unsafe { k.set_len(pages); }
+            // SAFETY: src buffer sized val.len() does not overlap with dst buffer since
+            // the dst buffer's size is val.len() padded up to a multiple of PAGE_SIZE.
+            unsafe { ptr::copy_nonoverlapping(val.as_ptr(), k.as_mut_ptr() as *mut u8,
+                                              val.len()) };
+            Ok(k)
+        },
+        Err(_) => Err(AllocError),
+    }
+}
+
+impl TryFrom<&[u8]> for VVec<PageSlice> {
+    type Error = AllocError;
+
+    fn try_from(val: &[u8]) -> Result<Self, AllocError> {
+        to_vec_with_allocator(val)
+    }
+}
+
+impl TryFrom<&[u8]> for KVec<PageSlice> {
+    type Error = AllocError;
+
+    fn try_from(val: &[u8]) -> Result<Self, AllocError> {
+        to_vec_with_allocator(val)
+    }
+}
+
+impl TryFrom<&[u8]> for KVVec<PageSlice> {
+    type Error = AllocError;
+
+    fn try_from(val: &[u8]) -> Result<Self, AllocError> {
+        to_vec_with_allocator(val)
+    }
+}
-- 
2.43.0

Re: [PATCH v3 1/2] rust: page: use the page's reference count to decide when to free the allocation

[Alice Ryhl]

On Tue, Nov 19, 2024 at 12:24 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> Ensure pages returned by the constructor are always reference counted.
> This requires that we replace the page pointer wrapper with Opaque instead
> of NonNull to make it possible to cast to a Page pointer from a raw struct
> page pointer which is needed to create an ARef instance.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>

> -/// A pointer to a page that owns the page allocation.
> +/// A pointer to a reference-counted page.
>  ///
>  /// # Invariants
>  ///
> -/// The pointer is valid, and has ownership over the page.
> +/// The pointer is valid.
> +#[repr(transparent)]
>  pub struct Page {
> -    page: NonNull<bindings::page>,
> +    page: Opaque<bindings::page>,

With this change, Page is no longer a pointer, nor does it contain a
pointer. The documentation should be updated to reflect this.

>  // SAFETY: Pages have no logic that relies on them staying on a given thread, so moving them across
> @@ -71,19 +73,23 @@ impl Page {
>      /// let page = Page::alloc_page(GFP_KERNEL | __GFP_ZERO)?;
>      /// # Ok(()) }
>      /// ```
> -    pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
> +    pub fn alloc_page(flags: Flags) -> Result<ARef<Self>, AllocError> {
>          // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
>          // is always safe to call this method.
>          let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
> -        let page = NonNull::new(page).ok_or(AllocError)?;
> -        // 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.
> -        Ok(Self { page })
> +        if page.is_null() {
> +            return Err(AllocError);
> +        }
> +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
> +        let ptr = page.cast::<Self>();
> +        // INVARIANT: We just successfully allocated a page, ptr points to the new `Page` object.
> +        // SAFETY: According to invariant above ptr is valid.
> +        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) })

Why did you change the null check? You should be able to avoid
changing anything but the last line.

Alice

Re: [PATCH v3 1/2] rust: page: use the page's reference count to decide when to free the allocation

[Abdiel Janulgue]

On 19/11/2024 13:45, Alice Ryhl wrote:
>> +    pub fn alloc_page(flags: Flags) -> Result<ARef<Self>, AllocError> {
>>           // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
>>           // is always safe to call this method.
>>           let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
>> -        let page = NonNull::new(page).ok_or(AllocError)?;
>> -        // 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.
>> -        Ok(Self { page })
>> +        if page.is_null() {
>> +            return Err(AllocError);
>> +        }
>> +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
>> +        let ptr = page.cast::<Self>();
>> +        // INVARIANT: We just successfully allocated a page, ptr points to the new `Page` object.
>> +        // SAFETY: According to invariant above ptr is valid.
>> +        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) })
> 
> Why did you change the null check? You should be able to avoid
> changing anything but the last line.

Changing only the line, it complains:

86  |         Ok(unsafe { ARef::from_raw(page) })
     |                     -------------- ^^^^ expected `NonNull<Page>`, 
found `NonNull<page>`

Unless this is what you mean?

         let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
         let page = page.cast::<Self>();
         let page = NonNull::new(page).ok_or(AllocError)?;
         Ok(unsafe { ARef::from_raw(page) })

But what if alloc_pages returns null in the place? Would that be a valid 
cast still?

Regards,
Abdiel

Re: [PATCH v3 1/2] rust: page: use the page's reference count to decide when to free the allocation

[Alice Ryhl]

On Tue, Nov 19, 2024 at 1:06 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
>
> On 19/11/2024 13:45, Alice Ryhl wrote:
> >> +    pub fn alloc_page(flags: Flags) -> Result<ARef<Self>, AllocError> {
> >>           // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
> >>           // is always safe to call this method.
> >>           let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
> >> -        let page = NonNull::new(page).ok_or(AllocError)?;
> >> -        // 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.
> >> -        Ok(Self { page })
> >> +        if page.is_null() {
> >> +            return Err(AllocError);
> >> +        }
> >> +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::page`.
> >> +        let ptr = page.cast::<Self>();
> >> +        // INVARIANT: We just successfully allocated a page, ptr points to the new `Page` object.
> >> +        // SAFETY: According to invariant above ptr is valid.
> >> +        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) })
> >
> > Why did you change the null check? You should be able to avoid
> > changing anything but the last line.
>
> Changing only the line, it complains:
>
> 86  |         Ok(unsafe { ARef::from_raw(page) })
>      |                     -------------- ^^^^ expected `NonNull<Page>`,
> found `NonNull<page>`
>
> Unless this is what you mean?
>
>          let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
>          let page = page.cast::<Self>();
>          let page = NonNull::new(page).ok_or(AllocError)?;
>          Ok(unsafe { ARef::from_raw(page) })

You can put the cast here:

let page = unsafe { bindings::alloc_pages(flags.as_raw(), 0) };
let page = NonNull::new(page).ok_or(AllocError)?;
Ok(unsafe { ARef::from_raw(page.cast()) })

> But what if alloc_pages returns null in the place? Would that be a valid
> cast still?

The cast is correct both before and after the null check. The above
code returns Err(AllocError) when the pointer is null.

Alice

Re: [PATCH v3 2/2] rust: page: Extend support to existing struct page mappings

[Jann Horn]

Commenting as someone who understands kernel MM decently but only
knows a tiny bit about Rust:

On Tue, Nov 19, 2024 at 12:24 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
> Extend Page to support pages that are not allocated by the constructor,
> for example, those returned by vmalloc_to_page() or virt_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.
>
> The new page_slice_to_page wrapper ensures that it explicity accepts
> only page-sized chunks.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
[...]
> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> index fdf7ee203597..d0a896f53afb 100644
> --- a/rust/kernel/page.rs
> +++ b/rust/kernel/page.rs
> @@ -3,7 +3,7 @@
>  //! Kernel page allocation and management.
>
>  use crate::{
> -    alloc::{AllocError, Flags},
> +    alloc::{AllocError, Allocator, Flags, VVec, KVec, KVVec, Vec, flags::*},
>      bindings,
>      error::code::*,
>      error::Result,
> @@ -87,6 +87,49 @@ pub fn alloc_page(flags: Flags) -> Result<ARef<Self>, AllocError> {
>          Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) })
>      }
>
> +    /// Create a page object from a buffer which is associated with an existing C `struct page`.
> +    ///
> +    /// This function ensures it takes a page-sized buffer as represented by `PageSlice`.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::page::*;
> +    ///
> +    /// let somedata: [u8; PAGE_SIZE * 2] = [0; PAGE_SIZE * 2];
> +    /// let buf: &[u8] = &somedata;
> +    /// let pages: VVec<PageSlice> = buf.try_into()?;
> +    /// let page = Page::page_slice_to_page(&pages[0])?;
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self>

Sorry, can you explain to me what the semantics of this are? Does this
create a Page reference that is not lifetime-bound to the PageSlice?

> +    {
> +        let ptr: *const core::ffi::c_void = page.0.as_ptr() as _;
> +        if ptr.is_null() {
> +            return Err(EINVAL)
> +        }
> +        // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method.
> +        let page = if unsafe { bindings::is_vmalloc_addr(ptr) } {
> +            // SAFETY: We've checked that `ptr` is non-null and within the vmalloc range, hence
> +            // it's safe to call this method.
> +            unsafe { bindings::vmalloc_to_page(ptr) }
> +        // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method.
> +        } else if unsafe { bindings::virt_addr_valid(ptr) } {
> +            // SAFETY: We've checked that `ptr` is non-null and a valid virtual address, hence
> +            // it's safe to call this method.
> +            unsafe { bindings::virt_to_page(ptr) }
> +        } else {
> +            ptr::null_mut()
> +        };
> +        if page.is_null() {
> +            return Err(EINVAL);
> +        }
> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::page`.
> +        // SAFETY: We just successfully retrieved an existing `bindings::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()
> @@ -270,3 +313,55 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>          unsafe { bindings::put_page(obj.cast().as_ptr()) }
>      }
>  }
> +
> +/// A page-aligned, page-sized object.
> +///
> +/// This is used for convenience to convert a large buffer into an array of page-sized chunks
> +/// allocated with the kernel's allocators which can then be used in the
> +/// `Page::page_slice_to_page` wrapper.
> +///
> +// FIXME: This should be `PAGE_SIZE`, but the compiler rejects everything except a literal
> +// integer argument for the `repr(align)` attribute.
> +#[repr(align(4096))]
> +pub struct PageSlice([u8; PAGE_SIZE]);
> +
> +fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> {
> +    let mut k = Vec::<PageSlice, A>::new();
> +    let pages = page_align(val.len()) >> PAGE_SHIFT;
> +    match k.reserve(pages, GFP_KERNEL) {

Do I understand correctly that this can be used to create a kmalloc
allocation whose pages can then basically be passed to
page_slice_to_page()?

FYI, the page refcount does not protect against UAF of slab
allocations through new slab allocations of the same size. In other
words: The slab allocator can internally recycle memory without going
through the page allocator, and the slab allocator itself does not
care about page refcounts.

If the Page returned from calling page_slice_to_page() on the slab
memory pages returned from to_vec_with_allocator() is purely usable as
a borrow and there is no way to later grab a refcounted reference to
it or pass it into a C function that assumes it can grab a reference
to the page, I guess that works. But if I understand correctly what's
going on here, and you can grab references to slab pages returned from
page_slice_to_page(to_vec_with_allocator()), that'd be bad.

> +        Ok(()) => {
> +            // SAFETY: from above, the length should be equal to the vector's capacity
> +            unsafe { k.set_len(pages); }
> +            // SAFETY: src buffer sized val.len() does not overlap with dst buffer since
> +            // the dst buffer's size is val.len() padded up to a multiple of PAGE_SIZE.
> +            unsafe { ptr::copy_nonoverlapping(val.as_ptr(), k.as_mut_ptr() as *mut u8,
> +                                              val.len()) };
> +            Ok(k)
> +        },
> +        Err(_) => Err(AllocError),
> +    }
> +}

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Matthew Wilcox]

On Tue, Nov 19, 2024 at 01:24:01PM +0200, Abdiel Janulgue wrote:
> This series aims to add support for pages that are not constructed by an
> instance of the rust Page abstraction, for example those returned by
> vmalloc_to_page() or virt_to_page().
> 
> Changes sinve v3:
> - Use the struct page's reference count to decide when to free the
>   allocation (Alice Ryhl, Boqun Feng).

Bleh, this is going to be "exciting".  We're in the middle of a multi-year
project to remove refcounts from struct page.  The lifetime of a page
will be controlled by the memdesc that it belongs to.  Some of those
memdescs will have refcounts, but others will not.

We don't have a fully formed destination yet, so I can't give you a
definite answer to a lot of questions.  Obviously I don't want to hold
up the Rust project in any way, but I need to know that what we're trying
to do will be expressible in Rust.

Can we avoid referring to a page's refcount?

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Alice Ryhl]

On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Nov 19, 2024 at 01:24:01PM +0200, Abdiel Janulgue wrote:
> > This series aims to add support for pages that are not constructed by an
> > instance of the rust Page abstraction, for example those returned by
> > vmalloc_to_page() or virt_to_page().
> >
> > Changes sinve v3:
> > - Use the struct page's reference count to decide when to free the
> >   allocation (Alice Ryhl, Boqun Feng).
>
> Bleh, this is going to be "exciting".  We're in the middle of a multi-year
> project to remove refcounts from struct page.  The lifetime of a page
> will be controlled by the memdesc that it belongs to.  Some of those
> memdescs will have refcounts, but others will not.
>
> We don't have a fully formed destination yet, so I can't give you a
> definite answer to a lot of questions.  Obviously I don't want to hold
> up the Rust project in any way, but I need to know that what we're trying
> to do will be expressible in Rust.
>
> Can we avoid referring to a page's refcount?

I don't think this patch needs the refcount at all, and the previous
version did not expose it. This came out of the advice to use put_page
over free_page. Does this mean that we should switch to put_page but
not use get_page?

Alice

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Boqun Feng]

On Wed, Nov 20, 2024 at 10:10:44AM +0100, Alice Ryhl wrote:
> On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Tue, Nov 19, 2024 at 01:24:01PM +0200, Abdiel Janulgue wrote:
> > > This series aims to add support for pages that are not constructed by an
> > > instance of the rust Page abstraction, for example those returned by
> > > vmalloc_to_page() or virt_to_page().
> > >
> > > Changes sinve v3:
> > > - Use the struct page's reference count to decide when to free the
> > >   allocation (Alice Ryhl, Boqun Feng).
> >
> > Bleh, this is going to be "exciting".  We're in the middle of a multi-year
> > project to remove refcounts from struct page.  The lifetime of a page
> > will be controlled by the memdesc that it belongs to.  Some of those
> > memdescs will have refcounts, but others will not.
> >

One question: will the page that doesn't have refcounts has an exclusive
owner? I.e. there is one owner that's responsible to free the page and
make sure other references to the page get properly invalidated (maybe
via RCU?)

> > We don't have a fully formed destination yet, so I can't give you a
> > definite answer to a lot of questions.  Obviously I don't want to hold
> > up the Rust project in any way, but I need to know that what we're trying
> > to do will be expressible in Rust.
> >
> > Can we avoid referring to a page's refcount?
> 
> I don't think this patch needs the refcount at all, and the previous
> version did not expose it. This came out of the advice to use put_page
> over free_page. Does this mean that we should switch to put_page but
> not use get_page?
> 

I think the point is finding the exact lifetime model for pages, if it's
not a simple refcounting, then what it is? Besides, we can still
represent refcounting pages with `struct Page` and other pages with a
different type name. So as far as I can see, this patch is OK for now.

Regards,
Boqun

> Alice

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Matthew Wilcox]

On Wed, Nov 20, 2024 at 08:20:16AM -0800, Boqun Feng wrote:
> On Wed, Nov 20, 2024 at 10:10:44AM +0100, Alice Ryhl wrote:
> > On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Tue, Nov 19, 2024 at 01:24:01PM +0200, Abdiel Janulgue wrote:
> > > > This series aims to add support for pages that are not constructed by an
> > > > instance of the rust Page abstraction, for example those returned by
> > > > vmalloc_to_page() or virt_to_page().
> > > >
> > > > Changes sinve v3:
> > > > - Use the struct page's reference count to decide when to free the
> > > >   allocation (Alice Ryhl, Boqun Feng).
> > >
> > > Bleh, this is going to be "exciting".  We're in the middle of a multi-year
> > > project to remove refcounts from struct page.  The lifetime of a page
> > > will be controlled by the memdesc that it belongs to.  Some of those
> > > memdescs will have refcounts, but others will not.
> > >
> 
> One question: will the page that doesn't have refcounts has an exclusive
> owner? I.e. there is one owner that's responsible to free the page and
> make sure other references to the page get properly invalidated (maybe
> via RCU?)

It's up to the owner of the page how they want to manage freeing it.
They can use a refcount (folios will still have a refcount, for example),
or they can know when there are no more users of the page (eg slab knows
when all objects in a slab are freed).  RCU is a possibility, but would
be quite unusual I would think.  The model I'm looking for here is that
'page' is too low-level an object to have its own lifecycle; it's always
defined by a higher level object.

> > > We don't have a fully formed destination yet, so I can't give you a
> > > definite answer to a lot of questions.  Obviously I don't want to hold
> > > up the Rust project in any way, but I need to know that what we're trying
> > > to do will be expressible in Rust.
> > >
> > > Can we avoid referring to a page's refcount?
> > 
> > I don't think this patch needs the refcount at all, and the previous
> > version did not expose it. This came out of the advice to use put_page
> > over free_page. Does this mean that we should switch to put_page but
> > not use get_page?

Did I advise using put_page() over free_page()?  I hope I didn't say
that.  I don't see a reason why binder needs to refcount its pages (nor
use a mapcount on them), but I don't fully understand binder so maybe
it does need a refcount.

> I think the point is finding the exact lifetime model for pages, if it's
> not a simple refcounting, then what it is? Besides, we can still
> represent refcounting pages with `struct Page` and other pages with a
> different type name. So as far as I can see, this patch is OK for now.

I don't want Page to have a refcount.  If you need something with a
refcount, it needs to be called something else.

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Boqun Feng]

On Wed, Nov 20, 2024 at 05:02:14PM +0000, Matthew Wilcox wrote:
> On Wed, Nov 20, 2024 at 08:20:16AM -0800, Boqun Feng wrote:
> > On Wed, Nov 20, 2024 at 10:10:44AM +0100, Alice Ryhl wrote:
> > > On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Tue, Nov 19, 2024 at 01:24:01PM +0200, Abdiel Janulgue wrote:
> > > > > This series aims to add support for pages that are not constructed by an
> > > > > instance of the rust Page abstraction, for example those returned by
> > > > > vmalloc_to_page() or virt_to_page().
> > > > >
> > > > > Changes sinve v3:
> > > > > - Use the struct page's reference count to decide when to free the
> > > > >   allocation (Alice Ryhl, Boqun Feng).
> > > >
> > > > Bleh, this is going to be "exciting".  We're in the middle of a multi-year
> > > > project to remove refcounts from struct page.  The lifetime of a page
> > > > will be controlled by the memdesc that it belongs to.  Some of those
> > > > memdescs will have refcounts, but others will not.
> > > >
> > 
> > One question: will the page that doesn't have refcounts has an exclusive
> > owner? I.e. there is one owner that's responsible to free the page and
> > make sure other references to the page get properly invalidated (maybe
> > via RCU?)
> 
> It's up to the owner of the page how they want to manage freeing it.
> They can use a refcount (folios will still have a refcount, for example),
> or they can know when there are no more users of the page (eg slab knows
> when all objects in a slab are freed).  RCU is a possibility, but would
> be quite unusual I would think.  The model I'm looking for here is that
> 'page' is too low-level an object to have its own lifecycle; it's always
> defined by a higher level object.
> 

Ok, that makes sense. That's actually aligned with the direction we are
heading in this patch: make `struct Page` itself independent on how the
lifetime is maintained. Conceptually, say we can define folio in pure
Rust, it could be:

    struct Folio {
        head: Page, /* or a union of page */
	...
    }

and we can `impl AlwaysRefcounted for Folio`, which implies there is a
refcount inside. And we can also have a `Foo` being:

    struct Foo {
        inner: Page,
    }

which doesn't implement `AlwaysRefcounted`, and that suggests a
different way the page lifetime will be maintained.

> > > > We don't have a fully formed destination yet, so I can't give you a
> > > > definite answer to a lot of questions.  Obviously I don't want to hold
> > > > up the Rust project in any way, but I need to know that what we're trying
> > > > to do will be expressible in Rust.
> > > >
> > > > Can we avoid referring to a page's refcount?
> > > 
> > > I don't think this patch needs the refcount at all, and the previous
> > > version did not expose it. This came out of the advice to use put_page
> > > over free_page. Does this mean that we should switch to put_page but
> > > not use get_page?
> 
> Did I advise using put_page() over free_page()?  I hope I didn't say

We have some off-list discussion about free_page() doesn't always free
the page if you could remember.

> that.  I don't see a reason why binder needs to refcount its pages (nor
> use a mapcount on them), but I don't fully understand binder so maybe
> it does need a refcount.

I don't think binder needs it either, but I think Abdiel here has a
different usage than binder.

> 
> > I think the point is finding the exact lifetime model for pages, if it's
> > not a simple refcounting, then what it is? Besides, we can still
> > represent refcounting pages with `struct Page` and other pages with a
> > different type name. So as far as I can see, this patch is OK for now.
> 
> I don't want Page to have a refcount.  If you need something with a
> refcount, it needs to be called something else.

So if I understand correctly, what Abdiel needs here is a way to convert
a virtual address to the corresponding page, would it make sense to just
use folio in this case? Abdiel, what's the operation you are going to
call on the page you get?

Regards,
Boqun

Re: [PATCH v3 2/2] rust: page: Extend support to existing struct page mappings

[Abdiel Janulgue]

Hi,

Thanks for the feedback.

On 19/11/2024 19:07, Jann Horn wrote:
>> +    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self>
> 
> Sorry, can you explain to me what the semantics of this are? Does this
> create a Page reference that is not lifetime-bound to the PageSlice?

This creates a Page reference that is tied to the lifetime of the `C 
struct page` behind the PageSlice buffer. Basically, it's just a cast 
from the struct page pointer and does not own that resource.

>> +fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> {
> Do I understand correctly that this can be used to create a kmalloc
> allocation whose pages can then basically be passed to
> page_slice_to_page()?
> 
> FYI, the page refcount does not protect against UAF of slab
> allocations through new slab allocations of the same size. In other
> words: The slab allocator can internally recycle memory without going
> through the page allocator, and the slab allocator itself does not
> care about page refcounts.
> 
> If the Page returned from calling page_slice_to_page() on the slab
> memory pages returned from to_vec_with_allocator() is purely usable as
> a borrow and there is no way to later grab a refcounted reference to
> it or pass it into a C function that assumes it can grab a reference
> to the page, I guess that works. 

Yes, I think that is the intent. I appreciate your help in pointing out 
the issues with using refcounts in slab memory pages. As you can see, 
page_slice_to_page() only returns a Page reference (not a refcounted 
Page). Hopefully that addresses your concern?

Regards,
Abdiel

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Abdiel Janulgue]

On 20/11/2024 19:25, Boqun Feng wrote:
> On Wed, Nov 20, 2024 at 05:02:14PM +0000, Matthew Wilcox wrote:
>> On Wed, Nov 20, 2024 at 08:20:16AM -0800, Boqun Feng wrote:
>>> On Wed, Nov 20, 2024 at 10:10:44AM +0100, Alice Ryhl wrote:
>>>> On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
>>>>>
>>>>> On Tue, Nov 19, 2024 at 01:24:01PM +0200, Abdiel Janulgue wrote:
>>>>>> This series aims to add support for pages that are not constructed by an
>>>>>> instance of the rust Page abstraction, for example those returned by
>>>>>> vmalloc_to_page() or virt_to_page().
>>>>>>
>>>>>> Changes sinve v3:
>>>>>> - Use the struct page's reference count to decide when to free the
>>>>>>    allocation (Alice Ryhl, Boqun Feng).
>>>>>
>>>>> Bleh, this is going to be "exciting".  We're in the middle of a multi-year
>>>>> project to remove refcounts from struct page.  The lifetime of a page
>>>>> will be controlled by the memdesc that it belongs to.  Some of those
>>>>> memdescs will have refcounts, but others will not.
>>>>>
>>>
>>> One question: will the page that doesn't have refcounts has an exclusive
>>> owner? I.e. there is one owner that's responsible to free the page and
>>> make sure other references to the page get properly invalidated (maybe
>>> via RCU?)
>>
>> It's up to the owner of the page how they want to manage freeing it.
>> They can use a refcount (folios will still have a refcount, for example),
>> or they can know when there are no more users of the page (eg slab knows
>> when all objects in a slab are freed).  RCU is a possibility, but would
>> be quite unusual I would think.  The model I'm looking for here is that
>> 'page' is too low-level an object to have its own lifecycle; it's always
>> defined by a higher level object.
>>
> 
> Ok, that makes sense. That's actually aligned with the direction we are
> heading in this patch: make `struct Page` itself independent on how the
> lifetime is maintained. Conceptually, say we can define folio in pure
> Rust, it could be:
> 
>      struct Folio {
>          head: Page, /* or a union of page */
> 	...
>      }
> 
> and we can `impl AlwaysRefcounted for Folio`, which implies there is a
> refcount inside. And we can also have a `Foo` being:
> 
>      struct Foo {
>          inner: Page,
>      }
> 
> which doesn't implement `AlwaysRefcounted`, and that suggests a
> different way the page lifetime will be maintained.
> 
>>>>> We don't have a fully formed destination yet, so I can't give you a
>>>>> definite answer to a lot of questions.  Obviously I don't want to hold
>>>>> up the Rust project in any way, but I need to know that what we're trying
>>>>> to do will be expressible in Rust.
>>>>>
>>>>> Can we avoid referring to a page's refcount?
>>>>
>>>> I don't think this patch needs the refcount at all, and the previous
>>>> version did not expose it. This came out of the advice to use put_page
>>>> over free_page. Does this mean that we should switch to put_page but
>>>> not use get_page?
>>
>> Did I advise using put_page() over free_page()?  I hope I didn't say
> 
> We have some off-list discussion about free_page() doesn't always free
> the page if you could remember.
> 
>> that.  I don't see a reason why binder needs to refcount its pages (nor
>> use a mapcount on them), but I don't fully understand binder so maybe
>> it does need a refcount.
> 
> I don't think binder needs it either, but I think Abdiel here has a
> different usage than binder.
> 
>>
>>> I think the point is finding the exact lifetime model for pages, if it's
>>> not a simple refcounting, then what it is? Besides, we can still
>>> represent refcounting pages with `struct Page` and other pages with a
>>> different type name. So as far as I can see, this patch is OK for now.
>>
>> I don't want Page to have a refcount.  If you need something with a
>> refcount, it needs to be called something else.
> 
> So if I understand correctly, what Abdiel needs here is a way to convert
> a virtual address to the corresponding page, would it make sense to just
> use folio in this case? Abdiel, what's the operation you are going to
> call on the page you get?

Yes that's basically it. The goal here is represent those existing 
struct page within this rust Page abstraction but at the same time to 
avoid taking over its ownership.

Boqun, Alice, should we reconsider Ownable and Owned trait again? :)

Regards,
Abdiel

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Boqun Feng]

On Thu, Nov 21, 2024 at 12:56:38AM +0200, Abdiel Janulgue wrote:
> On 20/11/2024 19:25, Boqun Feng wrote:
> > On Wed, Nov 20, 2024 at 05:02:14PM +0000, Matthew Wilcox wrote:
> > > On Wed, Nov 20, 2024 at 08:20:16AM -0800, Boqun Feng wrote:
> > > > On Wed, Nov 20, 2024 at 10:10:44AM +0100, Alice Ryhl wrote:
> > > > > On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > 
> > > > > > On Tue, Nov 19, 2024 at 01:24:01PM +0200, Abdiel Janulgue wrote:
> > > > > > > This series aims to add support for pages that are not constructed by an
> > > > > > > instance of the rust Page abstraction, for example those returned by
> > > > > > > vmalloc_to_page() or virt_to_page().
> > > > > > > 
> > > > > > > Changes sinve v3:
> > > > > > > - Use the struct page's reference count to decide when to free the
> > > > > > >    allocation (Alice Ryhl, Boqun Feng).
> > > > > > 
> > > > > > Bleh, this is going to be "exciting".  We're in the middle of a multi-year
> > > > > > project to remove refcounts from struct page.  The lifetime of a page
> > > > > > will be controlled by the memdesc that it belongs to.  Some of those
> > > > > > memdescs will have refcounts, but others will not.
> > > > > > 
> > > > 
> > > > One question: will the page that doesn't have refcounts has an exclusive
> > > > owner? I.e. there is one owner that's responsible to free the page and
> > > > make sure other references to the page get properly invalidated (maybe
> > > > via RCU?)
> > > 
> > > It's up to the owner of the page how they want to manage freeing it.
> > > They can use a refcount (folios will still have a refcount, for example),
> > > or they can know when there are no more users of the page (eg slab knows
> > > when all objects in a slab are freed).  RCU is a possibility, but would
> > > be quite unusual I would think.  The model I'm looking for here is that
> > > 'page' is too low-level an object to have its own lifecycle; it's always
> > > defined by a higher level object.
> > > 
> > 
> > Ok, that makes sense. That's actually aligned with the direction we are
> > heading in this patch: make `struct Page` itself independent on how the
> > lifetime is maintained. Conceptually, say we can define folio in pure
> > Rust, it could be:
> > 
> >      struct Folio {
> >          head: Page, /* or a union of page */
> > 	...
> >      }
> > 
> > and we can `impl AlwaysRefcounted for Folio`, which implies there is a
> > refcount inside. And we can also have a `Foo` being:
> > 
> >      struct Foo {
> >          inner: Page,
> >      }
> > 
> > which doesn't implement `AlwaysRefcounted`, and that suggests a
> > different way the page lifetime will be maintained.
> > 
> > > > > > We don't have a fully formed destination yet, so I can't give you a
> > > > > > definite answer to a lot of questions.  Obviously I don't want to hold
> > > > > > up the Rust project in any way, but I need to know that what we're trying
> > > > > > to do will be expressible in Rust.
> > > > > > 
> > > > > > Can we avoid referring to a page's refcount?
> > > > > 
> > > > > I don't think this patch needs the refcount at all, and the previous
> > > > > version did not expose it. This came out of the advice to use put_page
> > > > > over free_page. Does this mean that we should switch to put_page but
> > > > > not use get_page?
> > > 
> > > Did I advise using put_page() over free_page()?  I hope I didn't say
> > 
> > We have some off-list discussion about free_page() doesn't always free
> > the page if you could remember.
> > 
> > > that.  I don't see a reason why binder needs to refcount its pages (nor
> > > use a mapcount on them), but I don't fully understand binder so maybe
> > > it does need a refcount.
> > 
> > I don't think binder needs it either, but I think Abdiel here has a
> > different usage than binder.
> > 
> > > 
> > > > I think the point is finding the exact lifetime model for pages, if it's
> > > > not a simple refcounting, then what it is? Besides, we can still
> > > > represent refcounting pages with `struct Page` and other pages with a
> > > > different type name. So as far as I can see, this patch is OK for now.
> > > 
> > > I don't want Page to have a refcount.  If you need something with a
> > > refcount, it needs to be called something else.
> > 
> > So if I understand correctly, what Abdiel needs here is a way to convert
> > a virtual address to the corresponding page, would it make sense to just
> > use folio in this case? Abdiel, what's the operation you are going to
> > call on the page you get?
> 
> Yes that's basically it. The goal here is represent those existing struct
> page within this rust Page abstraction but at the same time to avoid taking
> over its ownership.
> 
> Boqun, Alice, should we reconsider Ownable and Owned trait again? :)
> 

Could you use folio in your case? If so, we can provide a simple binding
for folio which should be `AlwaysRefcounted`, and re-investigate how
page should be wrapped.

Regards,
Boqun

> Regards,
> Abdiel

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Alice Ryhl]

On Thu, Nov 21, 2024 at 1:24 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Thu, Nov 21, 2024 at 12:56:38AM +0200, Abdiel Janulgue wrote:
> > On 20/11/2024 19:25, Boqun Feng wrote:
> > > On Wed, Nov 20, 2024 at 05:02:14PM +0000, Matthew Wilcox wrote:
> > > > On Wed, Nov 20, 2024 at 08:20:16AM -0800, Boqun Feng wrote:
> > > > > On Wed, Nov 20, 2024 at 10:10:44AM +0100, Alice Ryhl wrote:
> > > > > > On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > >
> > > > > > > On Tue, Nov 19, 2024 at 01:24:01PM +0200, Abdiel Janulgue wrote:
> > > > > > > > This series aims to add support for pages that are not constructed by an
> > > > > > > > instance of the rust Page abstraction, for example those returned by
> > > > > > > > vmalloc_to_page() or virt_to_page().
> > > > > > > >
> > > > > > > > Changes sinve v3:
> > > > > > > > - Use the struct page's reference count to decide when to free the
> > > > > > > >    allocation (Alice Ryhl, Boqun Feng).
> > > > > > >
> > > > > > > Bleh, this is going to be "exciting".  We're in the middle of a multi-year
> > > > > > > project to remove refcounts from struct page.  The lifetime of a page
> > > > > > > will be controlled by the memdesc that it belongs to.  Some of those
> > > > > > > memdescs will have refcounts, but others will not.
> > > > > > >
> > > > >
> > > > > One question: will the page that doesn't have refcounts has an exclusive
> > > > > owner? I.e. there is one owner that's responsible to free the page and
> > > > > make sure other references to the page get properly invalidated (maybe
> > > > > via RCU?)
> > > >
> > > > It's up to the owner of the page how they want to manage freeing it.
> > > > They can use a refcount (folios will still have a refcount, for example),
> > > > or they can know when there are no more users of the page (eg slab knows
> > > > when all objects in a slab are freed).  RCU is a possibility, but would
> > > > be quite unusual I would think.  The model I'm looking for here is that
> > > > 'page' is too low-level an object to have its own lifecycle; it's always
> > > > defined by a higher level object.
> > > >
> > >
> > > Ok, that makes sense. That's actually aligned with the direction we are
> > > heading in this patch: make `struct Page` itself independent on how the
> > > lifetime is maintained. Conceptually, say we can define folio in pure
> > > Rust, it could be:
> > >
> > >      struct Folio {
> > >          head: Page, /* or a union of page */
> > >     ...
> > >      }
> > >
> > > and we can `impl AlwaysRefcounted for Folio`, which implies there is a
> > > refcount inside. And we can also have a `Foo` being:
> > >
> > >      struct Foo {
> > >          inner: Page,
> > >      }
> > >
> > > which doesn't implement `AlwaysRefcounted`, and that suggests a
> > > different way the page lifetime will be maintained.
> > >
> > > > > > > We don't have a fully formed destination yet, so I can't give you a
> > > > > > > definite answer to a lot of questions.  Obviously I don't want to hold
> > > > > > > up the Rust project in any way, but I need to know that what we're trying
> > > > > > > to do will be expressible in Rust.
> > > > > > >
> > > > > > > Can we avoid referring to a page's refcount?
> > > > > >
> > > > > > I don't think this patch needs the refcount at all, and the previous
> > > > > > version did not expose it. This came out of the advice to use put_page
> > > > > > over free_page. Does this mean that we should switch to put_page but
> > > > > > not use get_page?
> > > >
> > > > Did I advise using put_page() over free_page()?  I hope I didn't say
> > >
> > > We have some off-list discussion about free_page() doesn't always free
> > > the page if you could remember.
> > >
> > > > that.  I don't see a reason why binder needs to refcount its pages (nor
> > > > use a mapcount on them), but I don't fully understand binder so maybe
> > > > it does need a refcount.
> > >
> > > I don't think binder needs it either, but I think Abdiel here has a
> > > different usage than binder.
> > >
> > > >
> > > > > I think the point is finding the exact lifetime model for pages, if it's
> > > > > not a simple refcounting, then what it is? Besides, we can still
> > > > > represent refcounting pages with `struct Page` and other pages with a
> > > > > different type name. So as far as I can see, this patch is OK for now.
> > > >
> > > > I don't want Page to have a refcount.  If you need something with a
> > > > refcount, it needs to be called something else.
> > >
> > > So if I understand correctly, what Abdiel needs here is a way to convert
> > > a virtual address to the corresponding page, would it make sense to just
> > > use folio in this case? Abdiel, what's the operation you are going to
> > > call on the page you get?
> >
> > Yes that's basically it. The goal here is represent those existing struct
> > page within this rust Page abstraction but at the same time to avoid taking
> > over its ownership.
> >
> > Boqun, Alice, should we reconsider Ownable and Owned trait again? :)
> >
>
> Could you use folio in your case? If so, we can provide a simple binding
> for folio which should be `AlwaysRefcounted`, and re-investigate how
> page should be wrapped.

Well, regardless of that, I do think it sounds like Owned / Ownable is
the right way forward for Page.

Alice

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Abdiel Janulgue]

Hi Boqun, Matthew:

On 21/11/2024 02:24, Boqun Feng wrote:
>>> So if I understand correctly, what Abdiel needs here is a way to convert
>>> a virtual address to the corresponding page, would it make sense to just
>>> use folio in this case? Abdiel, what's the operation you are going to
>>> call on the page you get?
>>
>> Yes that's basically it. The goal here is represent those existing struct
>> page within this rust Page abstraction but at the same time to avoid taking
>> over its ownership.
>>
>> Boqun, Alice, should we reconsider Ownable and Owned trait again? :)
>>
> 
> Could you use folio in your case? If so, we can provide a simple binding
> for folio which should be `AlwaysRefcounted`, and re-investigate how
> page should be wrapped.
> 

I'm not sure. Is there a way to get the struct folio from a vmalloc'd 
address, e.g vmalloc_to_folio()?

Regards,
Abdiel

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Boqun Feng]

[Cc Kairui in case he's interested]

On Thu, Nov 21, 2024 at 11:30:13AM +0200, Abdiel Janulgue wrote:
> Hi Boqun, Matthew:
> 
> On 21/11/2024 02:24, Boqun Feng wrote:
> > > > So if I understand correctly, what Abdiel needs here is a way to convert
> > > > a virtual address to the corresponding page, would it make sense to just
> > > > use folio in this case? Abdiel, what's the operation you are going to
> > > > call on the page you get?
> > > 
> > > Yes that's basically it. The goal here is represent those existing struct
> > > page within this rust Page abstraction but at the same time to avoid taking
> > > over its ownership.
> > > 
> > > Boqun, Alice, should we reconsider Ownable and Owned trait again? :)
> > > 
> > 
> > Could you use folio in your case? If so, we can provide a simple binding
> > for folio which should be `AlwaysRefcounted`, and re-investigate how
> > page should be wrapped.
> > 
> 
> I'm not sure. Is there a way to get the struct folio from a vmalloc'd
> address, e.g vmalloc_to_folio()?
> 

I think you can use page_folio(vmalloc_to_page(..)) to get the folio,
but one thing to notice is that folio is guaranteed to be a non-tail
page, so if you want to do something later for the particular page (if
it's a tail page), you will need to know the offset of the that page in
folio. You can do something like below:

    pub fn page_slice_to_folio<'a>(page: &PageSlice) -> Result<(&'a Folio, usize)> {
        ...
	let page = vmalloc_to_page(ptr);

	let folio = page_folio(page);
	let offset = folio_page_idx(folio, page);

	Ok((folio, offset))
    }	

And you have a folio -> page function like:

    pub struct Folio(Opaque<bindings::folio>);

    impl Folio {
        pub fn nth_page(&self, n: usize) -> &Page {
	    &*(nth_page(self.0.get(), n))
	}
    }

Of course, this is me acting as I know MM ;-) but I feel this is the way
to go. And if binder can use folio as well (I don't see a reason why
not, but it's extra work, so defer to Alice), then we would only need
the `pub struct Page { inner: Opaque<bindings::page> }` part in your
patch #1, and can avoid doing `Ownable` or `AlwaysRefcounted` for
`Page`.

Thoughts?

Regards,
Boqun

> Regards,
> Abdiel

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Boqun Feng]

On Thu, Nov 21, 2024 at 11:10:45AM -0800, Boqun Feng wrote:
> [Cc Kairui in case he's interested]
> 

(forgot to cc...)

> On Thu, Nov 21, 2024 at 11:30:13AM +0200, Abdiel Janulgue wrote:
> > Hi Boqun, Matthew:
> > 
> > On 21/11/2024 02:24, Boqun Feng wrote:
> > > > > So if I understand correctly, what Abdiel needs here is a way to convert
> > > > > a virtual address to the corresponding page, would it make sense to just
> > > > > use folio in this case? Abdiel, what's the operation you are going to
> > > > > call on the page you get?
> > > > 
> > > > Yes that's basically it. The goal here is represent those existing struct
> > > > page within this rust Page abstraction but at the same time to avoid taking
> > > > over its ownership.
> > > > 
> > > > Boqun, Alice, should we reconsider Ownable and Owned trait again? :)
> > > > 
> > > 
> > > Could you use folio in your case? If so, we can provide a simple binding
> > > for folio which should be `AlwaysRefcounted`, and re-investigate how
> > > page should be wrapped.
> > > 
> > 
> > I'm not sure. Is there a way to get the struct folio from a vmalloc'd
> > address, e.g vmalloc_to_folio()?
> > 
> 
> I think you can use page_folio(vmalloc_to_page(..)) to get the folio,
> but one thing to notice is that folio is guaranteed to be a non-tail
> page, so if you want to do something later for the particular page (if
> it's a tail page), you will need to know the offset of the that page in
> folio. You can do something like below:
> 
>     pub fn page_slice_to_folio<'a>(page: &PageSlice) -> Result<(&'a Folio, usize)> {
>         ...
> 	let page = vmalloc_to_page(ptr);
> 
> 	let folio = page_folio(page);
> 	let offset = folio_page_idx(folio, page);
> 
> 	Ok((folio, offset))
>     }	
> 
> And you have a folio -> page function like:
> 
>     pub struct Folio(Opaque<bindings::folio>);
> 
>     impl Folio {
>         pub fn nth_page(&self, n: usize) -> &Page {
> 	    &*(nth_page(self.0.get(), n))
> 	}
>     }
> 
> Of course, this is me acting as I know MM ;-) but I feel this is the way
> to go. And if binder can use folio as well (I don't see a reason why
> not, but it's extra work, so defer to Alice), then we would only need
> the `pub struct Page { inner: Opaque<bindings::page> }` part in your
> patch #1, and can avoid doing `Ownable` or `AlwaysRefcounted` for
> `Page`.
> 
> Thoughts?
> 
> Regards,
> Boqun
> 
> > Regards,
> > Abdiel

Re: [PATCH v3 2/2] rust: page: Extend support to existing struct page mappings

[Jann Horn]

On Wed, Nov 20, 2024 at 11:56 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
> On 19/11/2024 19:07, Jann Horn wrote:
> >> +    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self>
> >
> > Sorry, can you explain to me what the semantics of this are? Does this
> > create a Page reference that is not lifetime-bound to the PageSlice?
>
> This creates a Page reference that is tied to the lifetime of the `C
> struct page` behind the PageSlice buffer. Basically, it's just a cast
> from the struct page pointer and does not own that resource.

How is the Page reference tied to the lifetime of the C "struct page"?

I asked some Rust experts to explain to me what this method signature
expands to, and they added the following to the Rust docs:

https://github.com/rust-lang/reference/blob/master/src/lifetime-elision.md
```
fn other_args1<'a>(arg: &str) -> &'a str;             // elided
fn other_args2<'a, 'b>(arg: &'b str) -> &'a str;      // expanded
```

Basically, my understanding is that since you are explicitly
specifying that the result should have lifetime 'a, but you are not
specifying the lifetime of the parameter, the parameter is given a
separate, unrelated lifetime by the compiler? Am I misunderstanding
how this works, or is that a typo in the method signature?

> >> +fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> {
> > Do I understand correctly that this can be used to create a kmalloc
> > allocation whose pages can then basically be passed to
> > page_slice_to_page()?
> >
> > FYI, the page refcount does not protect against UAF of slab
> > allocations through new slab allocations of the same size. In other
> > words: The slab allocator can internally recycle memory without going
> > through the page allocator, and the slab allocator itself does not
> > care about page refcounts.
> >
> > If the Page returned from calling page_slice_to_page() on the slab
> > memory pages returned from to_vec_with_allocator() is purely usable as
> > a borrow and there is no way to later grab a refcounted reference to
> > it or pass it into a C function that assumes it can grab a reference
> > to the page, I guess that works.
>
> Yes, I think that is the intent. I appreciate your help in pointing out
> the issues with using refcounts in slab memory pages. As you can see,
> page_slice_to_page() only returns a Page reference (not a refcounted
> Page). Hopefully that addresses your concern?

Does Rust also prevent safe code from invoking inc_ref() on the
returned Page reference? Normally, the AlwaysRefCounted trait means
that safe code can create an owned reference from a shared reference,
right?

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Matthew Wilcox]

On Thu, Nov 21, 2024 at 11:12:30AM -0800, Boqun Feng wrote:
> On Thu, Nov 21, 2024 at 11:30:13AM +0200, Abdiel Janulgue wrote:
> > Hi Boqun, Matthew:
> > 
> > On 21/11/2024 02:24, Boqun Feng wrote:
> > > > > So if I understand correctly, what Abdiel needs here is a way to convert
> > > > > a virtual address to the corresponding page, would it make sense to just
> > > > > use folio in this case? Abdiel, what's the operation you are going to
> > > > > call on the page you get?
> > > > 
> > > > Yes that's basically it. The goal here is represent those existing struct
> > > > page within this rust Page abstraction but at the same time to avoid taking
> > > > over its ownership.
> > > > 
> > > > Boqun, Alice, should we reconsider Ownable and Owned trait again? :)
> > > > 
> > > 
> > > Could you use folio in your case? If so, we can provide a simple binding
> > > for folio which should be `AlwaysRefcounted`, and re-investigate how
> > > page should be wrapped.
> > > 
> > 
> > I'm not sure. Is there a way to get the struct folio from a vmalloc'd
> > address, e.g vmalloc_to_folio()?
> > 
> 
> I think you can use page_folio(vmalloc_to_page(..)) to get the folio,
> but one thing to notice is that folio is guaranteed to be a non-tail
> page, so if you want to do something later for the particular page (if
> it's a tail page), you will need to know the offset of the that page in
> folio. You can do something like below:

This is one of those things which will work today, but will stop
working in the future, and anyway will only appear to work for some
users.

For example, both vmalloc and slab allocations do not use the refcount
on the struct page for anything.  eg this will be a UAF (please excuse
me writing in C):

	char *a = kmalloc(256, GFP_KERNEL);
	struct page *page = get_page(virt_to_page(a));
	char *b = page_address(page) + offset_in_page(a);
	// a and b will now have the same bit pattern
	kfree(a);
	*b = 1;

Once you've called kfree(), slab is entitled to hand that memory out
to any other user of kmalloc().  This might actually work to protect
vmalloc() memory from going away under you, but I intend to change
vmalloc so that it won't work (nothing to do with this patch series,
rather an approach to make vmalloc more efficient).

One reason you're confused today is that we have a temporary ambiguity
around what "folio" actually means.  The original definition (ie mine) was
simply that it was a non-tail page.  We're moving towards the definition
Johannes wanted, which is that it's only the memdesc for anonymous &
file-backed memory [1].  So while vmalloc_to_folio() makes sense under
the original definition, it's an absurdity under the new definition.

So, Abdiel, why are you trying to add this?  What are you actually
trying to accomplish in terms of "I am writing a device driver for XXX
and I need to ..."?  You've been very evasive up to now.

[1] Actually Johannes wants to split them apart even further so that
anon & file memory have different types, and we may yet get there.
One step at a time.

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Abdiel Janulgue]

On 22/11/2024 00:01, Matthew Wilcox wrote:
> On Thu, Nov 21, 2024 at 11:12:30AM -0800, Boqun Feng wrote:
>> On Thu, Nov 21, 2024 at 11:30:13AM +0200, Abdiel Janulgue wrote:
>>> Hi Boqun, Matthew:
>>>
>>> On 21/11/2024 02:24, Boqun Feng wrote:
>>>>>> So if I understand correctly, what Abdiel needs here is a way to convert
>>>>>> a virtual address to the corresponding page, would it make sense to just
>>>>>> use folio in this case? Abdiel, what's the operation you are going to
>>>>>> call on the page you get?
>>>>>
>>>>> Yes that's basically it. The goal here is represent those existing struct
>>>>> page within this rust Page abstraction but at the same time to avoid taking
>>>>> over its ownership.
>>>>>
>>>>> Boqun, Alice, should we reconsider Ownable and Owned trait again? :)
>>>>>
>>>>
>>>> Could you use folio in your case? If so, we can provide a simple binding
>>>> for folio which should be `AlwaysRefcounted`, and re-investigate how
>>>> page should be wrapped.
>>>>
>>>
>>> I'm not sure. Is there a way to get the struct folio from a vmalloc'd
>>> address, e.g vmalloc_to_folio()?
>>>
>>
>> I think you can use page_folio(vmalloc_to_page(..)) to get the folio,
>> but one thing to notice is that folio is guaranteed to be a non-tail
>> page, so if you want to do something later for the particular page (if
>> it's a tail page), you will need to know the offset of the that page in
>> folio. You can do something like below:
> 
> This is one of those things which will work today, but will stop
> working in the future, and anyway will only appear to work for some
> users.
> 
> For example, both vmalloc and slab allocations do not use the refcount
> on the struct page for anything.  eg this will be a UAF (please excuse
> me writing in C):
> 
> 	char *a = kmalloc(256, GFP_KERNEL);
> 	struct page *page = get_page(virt_to_page(a));
> 	char *b = page_address(page) + offset_in_page(a);
> 	// a and b will now have the same bit pattern
> 	kfree(a);
> 	*b = 1;
> 
> Once you've called kfree(), slab is entitled to hand that memory out
> to any other user of kmalloc().  This might actually work to protect
> vmalloc() memory from going away under you, but I intend to change
> vmalloc so that it won't work (nothing to do with this patch series,
> rather an approach to make vmalloc more efficient).
> 
> One reason you're confused today is that we have a temporary ambiguity
> around what "folio" actually means.  The original definition (ie mine) was
> simply that it was a non-tail page.  We're moving towards the definition
> Johannes wanted, which is that it's only the memdesc for anonymous &
> file-backed memory [1].  So while vmalloc_to_folio() makes sense under
> the original definition, it's an absurdity under the new definition.
> 
> So, Abdiel, why are you trying to add this?  What are you actually
> trying to accomplish in terms of "I am writing a device driver for XXX
> and I need to ..."?  You've been very evasive up to now.

Background behind this is that we need this for the nova rust driver [0].

We need an abstraction of struct page to construct a scatterlist which 
is needed for an internal firmware structure. Now most of pages needed 
there come from vmalloc_to_page() which, unlike the current rust Page 
abstraction, not allocated on demand but is an existing mapping.

Hope that clears things up!

Regards,
Abdiel

[0] https://rust-for-linux.com/nova-gpu-driver

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Matthew Wilcox]

On Fri, Nov 22, 2024 at 01:18:28AM +0200, Abdiel Janulgue wrote:
> We need an abstraction of struct page to construct a scatterlist which is
> needed for an internal firmware structure. Now most of pages needed there
> come from vmalloc_to_page() which, unlike the current rust Page abstraction,
> not allocated on demand but is an existing mapping.
> 
> Hope that clears things up!

That's very helpful!  So the lifetime of the scatterllist must not
outlive the lifetime of the vmalloc allocation.  That means you can call
kmap_local_page() on the page in the scatterlist without worrying about
the refcount of the struct page.  BTW, you can't call page_address() on
vmalloc memory because vmalloc can allocate pages from HIGHMEM.  Unless
you're willing to disable support for 32-bit systems with highmem ...

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[David Airlie]

On Fri, Nov 22, 2024 at 11:24 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Nov 22, 2024 at 01:18:28AM +0200, Abdiel Janulgue wrote:
> > We need an abstraction of struct page to construct a scatterlist which is
> > needed for an internal firmware structure. Now most of pages needed there
> > come from vmalloc_to_page() which, unlike the current rust Page abstraction,
> > not allocated on demand but is an existing mapping.
> >
> > Hope that clears things up!
>
> That's very helpful!  So the lifetime of the scatterllist must not
> outlive the lifetime of the vmalloc allocation.  That means you can call
> kmap_local_page() on the page in the scatterlist without worrying about
> the refcount of the struct page.  BTW, you can't call page_address() on
> vmalloc memory because vmalloc can allocate pages from HIGHMEM.  Unless
> you're willing to disable support for 32-bit systems with highmem ...
>

https://elixir.bootlin.com/linux/v6.11.5/source/drivers/gpu/drm/nouveau/nvkm/core/firmware.c#L266

This is the C code we want to rustify.

Dave.

Re: [PATCH v3 2/2] rust: page: Extend support to existing struct page mappings

[Alice Ryhl]

On Thu, Nov 21, 2024 at 9:18 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Nov 20, 2024 at 11:56 PM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
> > On 19/11/2024 19:07, Jann Horn wrote:
> > >> +    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self>
> > >
> > > Sorry, can you explain to me what the semantics of this are? Does this
> > > create a Page reference that is not lifetime-bound to the PageSlice?
> >
> > This creates a Page reference that is tied to the lifetime of the `C
> > struct page` behind the PageSlice buffer. Basically, it's just a cast
> > from the struct page pointer and does not own that resource.
>
> How is the Page reference tied to the lifetime of the C "struct page"?
>
> I asked some Rust experts to explain to me what this method signature
> expands to, and they added the following to the Rust docs:
>
> https://github.com/rust-lang/reference/blob/master/src/lifetime-elision.md
> ```
> fn other_args1<'a>(arg: &str) -> &'a str;             // elided
> fn other_args2<'a, 'b>(arg: &'b str) -> &'a str;      // expanded
> ```
>
> Basically, my understanding is that since you are explicitly
> specifying that the result should have lifetime 'a, but you are not
> specifying the lifetime of the parameter, the parameter is given a
> separate, unrelated lifetime by the compiler? Am I misunderstanding
> how this works, or is that a typo in the method signature?

No, you are correct. The signature is wrong and lets the caller pick
any lifetime they want, with no relation to the lifetime of the
underlying `struct page`.

From a C perspective, what are the lifetime requirements of vmalloc_to_page?

> > >> +fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> {
> > > Do I understand correctly that this can be used to create a kmalloc
> > > allocation whose pages can then basically be passed to
> > > page_slice_to_page()?
> > >
> > > FYI, the page refcount does not protect against UAF of slab
> > > allocations through new slab allocations of the same size. In other
> > > words: The slab allocator can internally recycle memory without going
> > > through the page allocator, and the slab allocator itself does not
> > > care about page refcounts.
> > >
> > > If the Page returned from calling page_slice_to_page() on the slab
> > > memory pages returned from to_vec_with_allocator() is purely usable as
> > > a borrow and there is no way to later grab a refcounted reference to
> > > it or pass it into a C function that assumes it can grab a reference
> > > to the page, I guess that works.
> >
> > Yes, I think that is the intent. I appreciate your help in pointing out
> > the issues with using refcounts in slab memory pages. As you can see,
> > page_slice_to_page() only returns a Page reference (not a refcounted
> > Page). Hopefully that addresses your concern?
>
> Does Rust also prevent safe code from invoking inc_ref() on the
> returned Page reference? Normally, the AlwaysRefCounted trait means
> that safe code can create an owned reference from a shared reference,
> right?

In principle, no, you could invoke inc_ref() directly. But the correct
way to do it would be to use ARef::from(my_ref) to obtain an actual
refcounted reference.

Alice

Re: [PATCH v3 2/2] rust: page: Extend support to existing struct page mappings

[Abdiel Janulgue]

On 21/11/2024 22:17, Jann Horn wrote:
> 
> Does Rust also prevent safe code from invoking inc_ref() on the
> returned Page reference? Normally, the AlwaysRefCounted trait means
> that safe code can create an owned reference from a shared reference,
> right?

While it is possible for someone to *manually* convert the Page 
reference returned in page_slice_to_page() to a refcounted Page (one 
could wrap it in an ARef). However, by design, page_slice_to_page() 
explicitly returns just an ordinary Page reference. We could add an 
invariant in page_slice_to_page() to warn against such usage just in case.

Anyway seems like the consensus from the other thread is to avoid 
refcounting the rust Page abstraction. If we go with that, then that 
moots this issue.

Regards,
Abdiel

Re: [PATCH v3 2/2] rust: page: Extend support to existing struct page mappings

[Abdiel Janulgue]

On 22/11/2024 09:55, Alice Ryhl wrote:
> On Thu, Nov 21, 2024 at 9:18 PM Jann Horn <jannh@google.com> wrote:
>>
>> On Wed, Nov 20, 2024 at 11:56 PM Abdiel Janulgue
>> <abdiel.janulgue@gmail.com> wrote:
>>> On 19/11/2024 19:07, Jann Horn wrote:
>>>>> +    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self>
>>>>
>>>> Sorry, can you explain to me what the semantics of this are? Does this
>>>> create a Page reference that is not lifetime-bound to the PageSlice?
>>>
>>> This creates a Page reference that is tied to the lifetime of the `C
>>> struct page` behind the PageSlice buffer. Basically, it's just a cast
>>> from the struct page pointer and does not own that resource.
>>
>> How is the Page reference tied to the lifetime of the C "struct page"?
>>
>> I asked some Rust experts to explain to me what this method signature
>> expands to, and they added the following to the Rust docs:
>>
>> https://github.com/rust-lang/reference/blob/master/src/lifetime-elision.md
>> ```
>> fn other_args1<'a>(arg: &str) -> &'a str;             // elided
>> fn other_args2<'a, 'b>(arg: &'b str) -> &'a str;      // expanded
>> ```
>>
>> Basically, my understanding is that since you are explicitly
>> specifying that the result should have lifetime 'a, but you are not
>> specifying the lifetime of the parameter, the parameter is given a
>> separate, unrelated lifetime by the compiler? Am I misunderstanding
>> how this works, or is that a typo in the method signature?
> 
> No, you are correct. The signature is wrong and lets the caller pick
> any lifetime they want, with no relation to the lifetime of the
> underlying `struct page`.

But that could be put in the invariant that the PageSlice buffer must 
last at least the lifetime `'a`?

> 
>  From a C perspective, what are the lifetime requirements of vmalloc_to_page?
> 

If I'm not mistaken, that should be the lifetime of the vmalloc'd buffer 
right?

Re: [PATCH v3 2/2] rust: page: Extend support to existing struct page mappings

[Alice Ryhl]

On Fri, Nov 22, 2024 at 9:36 AM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> On 22/11/2024 09:55, Alice Ryhl wrote:
> > On Thu, Nov 21, 2024 at 9:18 PM Jann Horn <jannh@google.com> wrote:
> >>
> >> On Wed, Nov 20, 2024 at 11:56 PM Abdiel Janulgue
> >> <abdiel.janulgue@gmail.com> wrote:
> >>> On 19/11/2024 19:07, Jann Horn wrote:
> >>>>> +    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self>
> >>>>
> >>>> Sorry, can you explain to me what the semantics of this are? Does this
> >>>> create a Page reference that is not lifetime-bound to the PageSlice?
> >>>
> >>> This creates a Page reference that is tied to the lifetime of the `C
> >>> struct page` behind the PageSlice buffer. Basically, it's just a cast
> >>> from the struct page pointer and does not own that resource.
> >>
> >> How is the Page reference tied to the lifetime of the C "struct page"?
> >>
> >> I asked some Rust experts to explain to me what this method signature
> >> expands to, and they added the following to the Rust docs:
> >>
> >> https://github.com/rust-lang/reference/blob/master/src/lifetime-elision.md
> >> ```
> >> fn other_args1<'a>(arg: &str) -> &'a str;             // elided
> >> fn other_args2<'a, 'b>(arg: &'b str) -> &'a str;      // expanded
> >> ```
> >>
> >> Basically, my understanding is that since you are explicitly
> >> specifying that the result should have lifetime 'a, but you are not
> >> specifying the lifetime of the parameter, the parameter is given a
> >> separate, unrelated lifetime by the compiler? Am I misunderstanding
> >> how this works, or is that a typo in the method signature?
> >
> > No, you are correct. The signature is wrong and lets the caller pick
> > any lifetime they want, with no relation to the lifetime of the
> > underlying `struct page`.
>
> But that could be put in the invariant that the PageSlice buffer must
> last at least the lifetime `'a`?
>
> >
> >  From a C perspective, what are the lifetime requirements of vmalloc_to_page?
> >
>
> If I'm not mistaken, that should be the lifetime of the vmalloc'd buffer
> right?

It seems to me that the signature should look like this:

fn vmalloc_to_page(vec: &VVec<PageSlice>, i: usize) -> &Page

This way, by providing the VVec, you can only use it with memory that
really comes from a vmalloc call.

Alice

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Paolo Bonzini]

On 11/22/24 07:58, David Airlie wrote:
> On Fri, Nov 22, 2024 at 11:24 AM Matthew Wilcox <willy@infradead.org> wrote:
>>
>> On Fri, Nov 22, 2024 at 01:18:28AM +0200, Abdiel Janulgue wrote:
>>> We need an abstraction of struct page to construct a scatterlist which is
>>> needed for an internal firmware structure. Now most of pages needed there
>>> come from vmalloc_to_page() which, unlike the current rust Page abstraction,
>>> not allocated on demand but is an existing mapping.
>>>
>>> Hope that clears things up!
>>
>> That's very helpful!  So the lifetime of the scatterllist must not
>> outlive the lifetime of the vmalloc allocation.  That means you can call
>> kmap_local_page() on the page in the scatterlist without worrying about
>> the refcount of the struct page.  BTW, you can't call page_address() on
>> vmalloc memory because vmalloc can allocate pages from HIGHMEM.  Unless
>> you're willing to disable support for 32-bit systems with highmem ...
>>
> 
> https://elixir.bootlin.com/linux/v6.11.5/source/drivers/gpu/drm/nouveau/nvkm/core/firmware.c#L266
> 
> This is the C code we want to rustify.

I don't think you want to increase/decrease the refcount there.  Instead 
you tie the lifetime of the returned page to the lifetime of the thing 
that provides the page, which would be some kind of NvkmFirmware struct.

pub enum NvkmFirmwareData {
     Ram(KBox<[PageSlice]>,
     Dma(CoherentAllocation<PageSlice>,
     Sgt(VBox<[PageSlice]>,
}

pub struct NvkmFirmware {
     ...,
     img: NvkmFirmwareData,
}

pub struct NvkmFirmwarePages<'a> {
     fw: &'a NvkmFirmware,
     sgt: SgTable,
}

impl NvkmFirmware {
     fn get_sgl(&self) -> NvkmFirmwarePages { ... }
}


Perhaps a trait that is implemented by both {K,V,KV}Vec<PageSlice> and 
{K,V,KV}Box<[PageSlice]>, like

trait ToComponentPage {
     fn to_component_page(&self, i: usize) -> &Page;
}

impl ToComponentPage for KVec<PageSlice> { // same for KBox<[PageSlice]>
     fn to_component_page(&self, i: usize) -> &Page {
         let base = &self[i << PAGE_SHIFT..];
         bindings::virt_to_page(base.as_ptr())
     }
}

impl ToComponentPage for VVec<PageSlice> { // same for VBox<[PageSlice]>
     fn to_component_page(&self, i: usize) -> &Page {
         let base = &self[i << PAGE_SHIFT..];
         bindings::vmalloc_to_page(base.as_ptr())
     }
}

?  And possibly also

trait ToComponentPageMut {
     fn to_component_page_mut(&mut self, i: usize) -> &Page;
}

which would be implemented by the Box types, but not by the Vec types 
because their data is not pinned.

Paolo

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Jann Horn]

On Wed, Nov 20, 2024 at 6:02 PM Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, Nov 20, 2024 at 08:20:16AM -0800, Boqun Feng wrote:
> > On Wed, Nov 20, 2024 at 10:10:44AM +0100, Alice Ryhl wrote:
> > > On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > We don't have a fully formed destination yet, so I can't give you a
> > > > definite answer to a lot of questions.  Obviously I don't want to hold
> > > > up the Rust project in any way, but I need to know that what we're trying
> > > > to do will be expressible in Rust.
> > > >
> > > > Can we avoid referring to a page's refcount?
> > >
> > > I don't think this patch needs the refcount at all, and the previous
> > > version did not expose it. This came out of the advice to use put_page
> > > over free_page. Does this mean that we should switch to put_page but
> > > not use get_page?
>
> Did I advise using put_page() over free_page()?  I hope I didn't say
> that.  I don't see a reason why binder needs to refcount its pages (nor
> use a mapcount on them), but I don't fully understand binder so maybe
> it does need a refcount.

I think that was me, at
<https://lore.kernel.org/all/CAG48ez32zWt4mcfA+y2FnzzNmFe-0ns9XQgp=QYeFpRsdiCAnw@mail.gmail.com/>.
Looking at the C binder version, binder_install_single_page() installs
pages into userspace page tables in a VM_MIXEDMAP mapping using
vm_insert_page(), and when you do that with pages from the page
allocator, userspace can grab references to them through GUP-fast (and
I think also through GUP). (See how vm_insert_page() and
vm_get_page_prot() don't use pte_mkspecial(), which is pretty much the
only thing that can stop GUP-fast on most architectures.)

My understanding is that the combination VM_IO|VM_MIXEDMAP would stop
normal GUP, but currently the only way to block GUP-fast is to use
VM_PFNMAP. (Which, as far as I understand, is also why GPU drivers use
VM_PFNMAP so much.) Maybe we should change that, so that VM_IO and/or
VM_MIXEDMAP blocks GUP in the region and causes installed PTEs to be
marked with pte_mkspecial()?

I am not entirely sure about this stuff, but I was recently looking at
net/packet/af_packet.c, and I tested that vmsplice() can grab
references to the high-order compound pages that
alloc_one_pg_vec_page() allocates with __get_free_pages(GFP_KERNEL |
__GFP_COMP | __GFP_ZERO | __GFP_NOWARN | __GFP_NORETRY, order),
packet_mmap() inserts with vm_insert_page(), and free_pg_vec() drops
with free_pages(). (But that all happens to actually work fine,
free_pages() actually handles refcounted compound pages properly.)

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Jann Horn]

On Tue, Nov 26, 2024 at 9:31 PM Jann Horn <jannh@google.com> wrote:
> On Wed, Nov 20, 2024 at 6:02 PM Matthew Wilcox <willy@infradead.org> wrote:
> > On Wed, Nov 20, 2024 at 08:20:16AM -0800, Boqun Feng wrote:
> > > On Wed, Nov 20, 2024 at 10:10:44AM +0100, Alice Ryhl wrote:
> > > > On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > We don't have a fully formed destination yet, so I can't give you a
> > > > > definite answer to a lot of questions.  Obviously I don't want to hold
> > > > > up the Rust project in any way, but I need to know that what we're trying
> > > > > to do will be expressible in Rust.
> > > > >
> > > > > Can we avoid referring to a page's refcount?
> > > >
> > > > I don't think this patch needs the refcount at all, and the previous
> > > > version did not expose it. This came out of the advice to use put_page
> > > > over free_page. Does this mean that we should switch to put_page but
> > > > not use get_page?
> >
> > Did I advise using put_page() over free_page()?  I hope I didn't say
> > that.  I don't see a reason why binder needs to refcount its pages (nor
> > use a mapcount on them), but I don't fully understand binder so maybe
> > it does need a refcount.
>
> I think that was me, at
> <https://lore.kernel.org/all/CAG48ez32zWt4mcfA+y2FnzzNmFe-0ns9XQgp=QYeFpRsdiCAnw@mail.gmail.com/>.
> Looking at the C binder version, binder_install_single_page() installs
> pages into userspace page tables in a VM_MIXEDMAP mapping using
> vm_insert_page(), and when you do that with pages from the page
> allocator, userspace can grab references to them through GUP-fast (and
> I think also through GUP). (See how vm_insert_page() and
> vm_get_page_prot() don't use pte_mkspecial(), which is pretty much the
> only thing that can stop GUP-fast on most architectures.)
>
> My understanding is that the combination VM_IO|VM_MIXEDMAP would stop
> normal GUP, but currently the only way to block GUP-fast is to use
> VM_PFNMAP. (Which, as far as I understand, is also why GPU drivers use
> VM_PFNMAP so much.) Maybe we should change that, so that VM_IO and/or
> VM_MIXEDMAP blocks GUP in the region and causes installed PTEs to be
> marked with pte_mkspecial()?
>
> I am not entirely sure about this stuff, but I was recently looking at
> net/packet/af_packet.c, and I tested that vmsplice() can grab
> references to the high-order compound pages that
> alloc_one_pg_vec_page() allocates with __get_free_pages(GFP_KERNEL |
> __GFP_COMP | __GFP_ZERO | __GFP_NOWARN | __GFP_NORETRY, order),
> packet_mmap() inserts with vm_insert_page(), and free_pg_vec() drops
> with free_pages(). (But that all happens to actually work fine,
> free_pages() actually handles refcounted compound pages properly.)

And also, the C binder driver wants to free pages in its shrinker
callback, but those pages might still be mapped into userspace. Binder
tries to zap such userspace mappings, but it does that by absolute
virtual address instead of going through the rmap (see
binder_alloc_free_page()), so it will miss page mappings in VMAs that
have been mremap()'d (though legitimate userspace never does that with
binder VMAs) or are concurrently being torn down by munmap(); so
currently the thing that keeps this from falling apart is that if page
mappings are left over somewhere, the page refcount ensures that this
userspace-mapped page doesn't get freed.

(I think the C binder code does its job, but is not exactly a great
model for how to write a clean driver that integrates nicely with the
rest of the kernel.)

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Asahi Lina]

On 11/19/24 8:24 PM, Abdiel Janulgue wrote:
> This series aims to add support for pages that are not constructed by an
> instance of the rust Page abstraction, for example those returned by
> vmalloc_to_page() or virt_to_page().
> 
> Changes sinve v3:
> - Use the struct page's reference count to decide when to free the
>   allocation (Alice Ryhl, Boqun Feng).
> - Make Page::page_slice_to_page handle virt_to_page cases as well
>   (Danilo Krummrich).
> - Link to v2: https://lore.kernel.org/lkml/20241022224832.1505432-1-abdiel.janulgue@gmail.com/
> 
> Changes since v2:
> - Use Owned and Ownable types for constructing Page as suggested in
>   instad of using ptr::read().
> - Link to v1: https://lore.kernel.org/rust-for-linux/20241007202752.3096472-1-abdiel.janulgue@gmail.com/
> 
> Abdiel Janulgue (2):
>   rust: page: use the page's reference count to decide when to free the
>     allocation
>   rust: page: Extend support to existing struct page mappings
> 
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/page.c             |  20 +++++
>  rust/kernel/page.rs             | 135 ++++++++++++++++++++++++++++----
>  3 files changed, 142 insertions(+), 14 deletions(-)
> 
> 
> base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb

Just wanted to comment on an upcoming use case I have that will need
this, to make sure we're aligned. I want to use the page allocator to
manage GPU page tables (currently done via an io-pgtable patch and
abstraction but that's going away because it turned out to be too
intrusive to upstream).

Since I'm dealing with page tables which are their own tree ownership
structure, and I don't want to duplicate management of the page life
cycles, this means I need to be able to:

- Convert a Rust-allocated and owned page *into* its physical address
(page_to_phys()).
- Convert a physical address *into* a Rust-allocated and owned page
(phys_to_page()).
- Borrow a Rust Page from a physical address (so I can do read/write
operations on its data without intending to destroy it).

Conceptually, the first two are like ARef::into_raw() and
ARef::from_raw() (or Box for that matter), while the third would
basically return a &Page with an arbitrary lifetime (up to the caller to
enforce the rules). The latter two would be unsafe functions by nature,
of course.

I think this would work just as well with some kind of Owned/Ownable
solution. Basically, I just need to be able to express the two concepts
of "Page owned and allocated by Rust" and "Page borrowed from a physical
address".

This maps to pagetable management like this:
- On PT allocation, a Page is allocated, cleared, and turned into its
physical address (to be populated in the parent PTE or top-level TTB)
- On PT free, a page physical address is converted back to a Page, its
PTEs are walked to recursively free child PTs or verify they are empty
entries for leaf PTs (invariant: no leaf PTEs, all mappings should be
removed before PT free) and dropped.
- On PT walk/PTE insertion and removal, a physical address is borrowed
as a Page, then `Page::with_page_mapped()` is used to perform R/W
operations on the PTEs contained within.

Tying the lifetime of actual leaf data pages mapped into the page table
to the page table itself is a higher-level concern that isn't relevant
here, drm_gpuvm handles that part and those pages are not allocated
directly via the page allocator, but rather as GEM objects which
ultimately come from shmem)

(Note: this hardware is always 64-bit without highmem so those concerns
don't apply here.)

~~ Lina

Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Alice Ryhl]

On Mon, Dec 2, 2024 at 1:03 PM Asahi Lina <lina@asahilina.net> wrote:
>
> On 11/19/24 8:24 PM, Abdiel Janulgue wrote:
> > This series aims to add support for pages that are not constructed by an
> > instance of the rust Page abstraction, for example those returned by
> > vmalloc_to_page() or virt_to_page().
> >
> > Changes sinve v3:
> > - Use the struct page's reference count to decide when to free the
> >   allocation (Alice Ryhl, Boqun Feng).
> > - Make Page::page_slice_to_page handle virt_to_page cases as well
> >   (Danilo Krummrich).
> > - Link to v2: https://lore.kernel.org/lkml/20241022224832.1505432-1-abdiel.janulgue@gmail.com/
> >
> > Changes since v2:
> > - Use Owned and Ownable types for constructing Page as suggested in
> >   instad of using ptr::read().
> > - Link to v1: https://lore.kernel.org/rust-for-linux/20241007202752.3096472-1-abdiel.janulgue@gmail.com/
> >
> > Abdiel Janulgue (2):
> >   rust: page: use the page's reference count to decide when to free the
> >     allocation
> >   rust: page: Extend support to existing struct page mappings
> >
> >  rust/bindings/bindings_helper.h |   1 +
> >  rust/helpers/page.c             |  20 +++++
> >  rust/kernel/page.rs             | 135 ++++++++++++++++++++++++++++----
> >  3 files changed, 142 insertions(+), 14 deletions(-)
> >
> >
> > base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb
>
> Just wanted to comment on an upcoming use case I have that will need
> this, to make sure we're aligned. I want to use the page allocator to
> manage GPU page tables (currently done via an io-pgtable patch and
> abstraction but that's going away because it turned out to be too
> intrusive to upstream).
>
> Since I'm dealing with page tables which are their own tree ownership
> structure, and I don't want to duplicate management of the page life
> cycles, this means I need to be able to:
>
> - Convert a Rust-allocated and owned page *into* its physical address
> (page_to_phys()).
> - Convert a physical address *into* a Rust-allocated and owned page
> (phys_to_page()).
> - Borrow a Rust Page from a physical address (so I can do read/write
> operations on its data without intending to destroy it).
>
> Conceptually, the first two are like ARef::into_raw() and
> ARef::from_raw() (or Box for that matter), while the third would
> basically return a &Page with an arbitrary lifetime (up to the caller to
> enforce the rules). The latter two would be unsafe functions by nature,
> of course.
>
> I think this would work just as well with some kind of Owned/Ownable
> solution. Basically, I just need to be able to express the two concepts
> of "Page owned and allocated by Rust" and "Page borrowed from a physical
> address".

I actually think the Owned/Ownable solution is even better for what
you need, because having a borrowed reference to the current Page
abstraction is pretty awkward as it assumes that the page is always
owned.

Alice

> This maps to pagetable management like this:
> - On PT allocation, a Page is allocated, cleared, and turned into its
> physical address (to be populated in the parent PTE or top-level TTB)
> - On PT free, a page physical address is converted back to a Page, its
> PTEs are walked to recursively free child PTs or verify they are empty
> entries for leaf PTs (invariant: no leaf PTEs, all mappings should be
> removed before PT free) and dropped.
> - On PT walk/PTE insertion and removal, a physical address is borrowed
> as a Page, then `Page::with_page_mapped()` is used to perform R/W
> operations on the PTEs contained within.
>
> Tying the lifetime of actual leaf data pages mapped into the page table
> to the page table itself is a higher-level concern that isn't relevant
> here, drm_gpuvm handles that part and those pages are not allocated
> directly via the page allocator, but rather as GEM objects which
> ultimately come from shmem)
>
> (Note: this hardware is always 64-bit without highmem so those concerns
> don't apply here.)
>
> ~~ Lina
>