[PATCH v2 0/5] Introduce Owned type and Ownable trait (was: "rust: page: Add support for vmalloc_to_page")

[PATCH v2 0/5] Introduce Owned type and Ownable trait (was: "rust: page: Add support for vmalloc_to_page")

[Abdiel Janulgue]

Hi all,

This series introduces the Owned type and Ownable trait which is the v2 of
"rust: page: Add support for vmalloc_to_page" [0]. This series includes changes
for firmware as well to make use of the new wrapper.

Changes since v2:
- Use Owned and Ownable types for constructing Page as suggested in [1]
  instad of using ptr::read().

[0] https://lore.kernel.org/rust-for-linux/20241007202752.3096472-1-abdiel.janulgue@gmail.com/
[1] https://lore.kernel.org/rust-for-linux/ZwUYmunVpzpexGV8@boqun-archlinux/

Abdiel Janulgue (5):
  rust: types: add `Owned` type and `Ownable` trait
  rust: page: Make ownership of the page pointer explicit.
  rust: page: Extend support to vmalloc_to_page
  rust: page: Add page_slice_to_page
  rust: firmware: implement `Ownable` for Firmware

 rust/kernel/firmware.rs |  31 ++++++-----
 rust/kernel/page.rs     | 116 +++++++++++++++++++++++++++++++++++-----
 rust/kernel/types.rs    |  62 +++++++++++++++++++++
 3 files changed, 184 insertions(+), 25 deletions(-)


base-commit: 15541c9263ce34ff95a06bc68f45d9bc5c990bcd
-- 
2.43.0

[PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Abdiel Janulgue]

Add the 'Owned' type, a simple smart pointer type that owns the
underlying data.

An object implementing `Ownable' can constructed by wrapping it in
`Owned`, which has the advantage of allowing fine-grained control
over it's resource allocation and deallocation.

Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index ced143600eb1..3f632916bd4d 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -429,3 +429,65 @@ pub enum Either<L, R> {
     /// Constructs an instance of [`Either`] containing a value of type `R`.
     Right(R),
 }
+
+/// A smart pointer that owns the underlying data `T`.
+///
+/// This is a simple smart pointer that owns the underlying data. Typically, this would be
+/// returned as a wrapper for `T` in `T`'s constructor.
+/// When an object adds an option of being constructed this way, in addition to implementing
+/// `Drop`, it implements `Ownable` as well, thus having finer-grained control in where
+/// resource allocation and deallocation happens.
+///
+/// # Invariants
+///
+/// The pointer is always valid and owns the underlying data.
+pub struct Owned<T: Ownable> {
+    ptr: NonNull<T>,
+}
+
+impl<T: Ownable> Owned<T> {
+    /// Creates a new smart pointer that owns `T`.
+    ///
+    /// # Safety
+    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
+    /// in other words, no other entity should free the underlying data.
+    pub unsafe fn to_owned(ptr: *mut T) -> Self {
+	// SAFETY: Per function safety requirement.
+	Self { ptr: unsafe { NonNull::new_unchecked(ptr) } }
+    }
+}
+
+impl<T: Ownable> Deref for Owned<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
+        // safe to dereference it.
+        unsafe { self.ptr.as_ref() }
+    }
+}
+
+impl<T: Ownable> DerefMut for Owned<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
+        // safe to dereference it.
+        unsafe { self.ptr.as_mut() }
+    }
+}
+
+/// An Ownable type is a type that can be put into `Owned<T>`, and when `Owned<T>` drops,
+/// `ptr_drop` will be called.
+pub unsafe trait Ownable {
+    /// # Safety
+    /// This could only be called in the `Owned::drop` function.
+    unsafe fn ptr_drop(ptr: *mut Self);
+}
+
+impl<T: Ownable> Drop for Owned<T> {
+    fn drop(&mut self) {
+	// SAFETY: In Owned<T>::drop.
+	unsafe {
+	    <T as Ownable>::ptr_drop(self.ptr.as_mut());
+	}
+    }
+}
-- 
2.43.0

[PATCH v2 2/5] rust: page: Make ownership of the page pointer explicit.

[Abdiel Janulgue]

Ensure only `Page::alloc_page` return pages that own the page allocation.
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.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/page.rs | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index fdac6c375fe4..a8288c15b860 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -8,8 +8,9 @@
     error::code::*,
     error::Result,
     uaccess::UserSliceReader,
+    types::{Opaque, Owned, Ownable},
 };
-use core::ptr::{self, NonNull};
+use core::ptr::{self};
 
 /// A bitwise shift for the page size.
 pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize;
@@ -35,8 +36,9 @@ pub const fn page_align(addr: usize) -> usize {
 /// # Invariants
 ///
 /// The pointer is valid, and has ownership over the page.
+#[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,24 @@ 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<Owned<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)?;
+        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, so we now have ownership of the newly
         // allocated page. We transfer that ownership to the new `Page` object.
-        Ok(Self { page })
+        // SAFETY: According to invariant above ptr is valid.
+        Ok(unsafe { Owned::to_owned(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 +259,9 @@ pub unsafe fn copy_from_user_slice_raw(
     }
 }
 
-impl Drop for Page {
-    fn drop(&mut self) {
+unsafe impl Ownable for Page {
+    unsafe fn ptr_drop(ptr: *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) };
+        unsafe { bindings::__free_pages(ptr.cast(), 0) };
     }
 }
-- 
2.43.0

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

[Abdiel Janulgue]

Extend Page to support pages that are not allocated by the constructor, for
example, those returned by vmalloc_to_page(). Since we don't own those pages
we shouldn't Drop them either. Hence we take advantage of the switch to Opaque
so we can cast to a Page pointer from a struct page pointer and be able to
retrieve the reference on an existing struct page mapping. In this case
no destructor will be called since we are not instantiating a new Page instance.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/page.rs | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index a8288c15b860..465928986f4b 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -31,11 +31,12 @@ pub const fn page_align(addr: usize) -> usize {
     (addr + (PAGE_SIZE - 1)) & PAGE_MASK
 }
 
-/// A pointer to a page that owns the page allocation.
+/// A pointer to a page that may own the page allocation.
 ///
 /// # Invariants
 ///
-/// The pointer is valid, and has ownership over the page.
+/// The pointer is valid, and has ownership over the page if the page is allocated by this
+/// abstraction.
 #[repr(transparent)]
 pub struct Page {
     page: Opaque<bindings::page>,
@@ -88,6 +89,33 @@ pub fn alloc_page(flags: Flags) -> Result<Owned<Self>, AllocError> {
         Ok(unsafe { Owned::to_owned(ptr) })
     }
 
+    /// This is just a wrapper to vmalloc_to_page which returns an existing page mapping, hence
+    /// we don't take ownership of the page. Returns an error if the pointer is null or if it
+    /// is not returned by vmalloc().
+    pub fn vmalloc_to_page<'a>(
+        cpu_addr: *const core::ffi::c_void
+    ) -> Result<&'a Self, AllocError>
+    {
+        if cpu_addr.is_null() {
+            return Err(AllocError);
+        }
+        // SAFETY: We've checked that the pointer is not null, so it is safe to call this method.
+        if unsafe { !bindings::is_vmalloc_addr(cpu_addr) } {
+            return Err(AllocError);
+        }
+        // SAFETY: We've initially ensured the pointer argument to this function is not null and
+        // checked for the requirement the the buffer passed to it should be allocated by vmalloc,
+        // so it is safe to call this method.
+        let page = unsafe { bindings::vmalloc_to_page(cpu_addr) };
+        if page.is_null() {
+            return Err(AllocError);
+        }
+        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::page`.
+        // SAFETY: We just successfully allocated a page, therefore dereferencing
+        // the page pointer is valid.
+        Ok(unsafe { &*page.cast() })
+    }
+
     /// Returns a raw pointer to the page.
     pub fn as_ptr(&self) -> *mut bindings::page {
         self.page.get()
-- 
2.43.0

[PATCH v2 4/5] rust: page: Add page_slice_to_page

[Abdiel Janulgue]

Make it convenient to convert buffers into page-sized vmalloc'd chunks which
can then be used in vmalloc_to_page().

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/page.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index 465928986f4b..2f0507fac9f0 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, Flags, VVec, flags::*},
     bindings,
     error::code::*,
     error::Result,
@@ -116,6 +116,26 @@ pub fn vmalloc_to_page<'a>(
         Ok(unsafe { &*page.cast() })
     }
 
+    /// A convenience wrapper to vmalloc_to_page which ensures it takes a page-sized buffer
+    /// represented by `PageSlice`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::page::*;
+    ///
+    /// # fn dox() -> Result<(), kernel::alloc::AllocError> {
+    /// 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(()) }
+    /// ```
+    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self, AllocError>
+    {
+        Self::vmalloc_to_page(page.0.as_ptr() as _)
+    }
+
     /// Returns a raw pointer to the page.
     pub fn as_ptr(&self) -> *mut bindings::page {
         self.page.get()
@@ -287,6 +307,43 @@ pub unsafe fn copy_from_user_slice_raw(
     }
 }
 
+/// A page-aligned, page-sized object. This is used for convenience to convert a large
+/// buffer into an array of page-sized vmalloc'd chunks which can then be used in
+/// `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]);
+
+impl TryFrom<&[u8]> for VVec<PageSlice> {
+    type Error = AllocError;
+
+    fn try_from(val: &[u8]) -> Result<Self, AllocError> {
+        let mut k = VVec::new();
+        let aligned_len: usize;
+
+        // Ensure the size is page-aligned.
+        match core::alloc::Layout::from_size_align(val.len(), PAGE_SIZE) {
+            Ok(align) => { aligned_len = align.pad_to_align().size() },
+            Err(_) => return Err(AllocError),
+        };
+        let pages = aligned_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),
+        }
+    }
+}
+
 unsafe impl Ownable for Page {
     unsafe fn ptr_drop(ptr: *mut Self) {
         // SAFETY: By the type invariants, we have ownership of the page and can free it.
-- 
2.43.0

[PATCH v2 5/5] rust: firmware: implement `Ownable` for Firmware

[Abdiel Janulgue]

For consistency, wrap the firmware as an `Owned` smart pointer in the
constructor.

Cc: Danilo Krummrich <dakr@redhat.com>
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index dee5b4b18aec..6da834b37455 100644
--- a/rust/kernel/firmware.rs
+++ b/rust/kernel/firmware.rs
@@ -4,8 +4,8 @@
 //!
 //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h)
 
-use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
-use core::ptr::NonNull;
+use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
+            types::{Opaque, Owned, Ownable}};
 
 /// # Invariants
 ///
@@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
 /// # Ok(())
 /// # }
 /// ```
-pub struct Firmware(NonNull<bindings::firmware>);
+ #[repr(transparent)]
+pub struct Firmware(Opaque<bindings::firmware>);
 
 impl Firmware {
-    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
+    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {
         let mut fw: *mut bindings::firmware = core::ptr::null_mut();
         let pfw: *mut *mut bindings::firmware = &mut fw;
 
@@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
         if ret != 0 {
             return Err(Error::from_errno(ret));
         }
-
+        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
+        let ptr = fw.cast::<Self>();
         // SAFETY: `func` not bailing out with a non-zero error code, guarantees that `fw` is a
         // valid pointer to `bindings::firmware`.
-        Ok(Firmware(unsafe { NonNull::new_unchecked(fw) }))
+        Ok(unsafe { Owned::to_owned(ptr) })
     }
 
     /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
-    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
+    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
         Self::request_internal(name, dev, FwFunc::request())
     }
 
     /// Send a request for an optional firmware module. See also
     /// `bindings::firmware_request_nowarn`.
-    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
+    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
         Self::request_internal(name, dev, FwFunc::request_nowarn())
     }
 
     fn as_raw(&self) -> *mut bindings::firmware {
-        self.0.as_ptr()
+        self.0.get()
     }
 
     /// Returns the size of the requested firmware in bytes.
@@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
     }
 }
 
-impl Drop for Firmware {
-    fn drop(&mut self) {
-        // SAFETY: `self.as_raw()` is valid by the type invariant.
-        unsafe { bindings::release_firmware(self.as_raw()) };
+unsafe impl Ownable for Firmware {
+    unsafe fn ptr_drop(ptr: *mut Self) {
+        // SAFETY:
+        // - By the type invariants, we have ownership of the ptr and can free it.
+        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
+        //   unique pointer to object, it's safe to release the firmware.
+        unsafe { bindings::release_firmware(ptr.cast()) };
     }
 }
 
-- 
2.43.0

Re: [PATCH v2 0/5] Introduce Owned type and Ownable trait (was: "rust: page: Add support for vmalloc_to_page")

[Danilo Krummrich]

On Wed, Oct 23, 2024 at 01:44:44AM +0300, Abdiel Janulgue wrote:
> Hi all,
> 
> This series introduces the Owned type and Ownable trait which is the v2 of
> "rust: page: Add support for vmalloc_to_page" [0]. This series includes changes
> for firmware as well to make use of the new wrapper.

Please make sure to add all relevant maintainers. Since this includes a firmware
patch, you should make sure to add all firmware maintainers. Remember to use
scripts/get_maintainer.pl.

Also there are a few minor checkpatch warnings. Please also make sure to run
scripts/checkpatch.pl.

Please also make sure to compile the code with `CLIPPY=1` (there are a bunch of
warnings) and make sure to also run the `rustfmt` target (there are some
formatting issues).

I wonder if it would make sense to make `CLIPPY=1` the default and only disable
it by explicitly passing `CLIPPY=0`.

> 
> Changes since v2:
> - Use Owned and Ownable types for constructing Page as suggested in [1]
>   instad of using ptr::read().
> 
> [0] https://lore.kernel.org/rust-for-linux/20241007202752.3096472-1-abdiel.janulgue@gmail.com/
> [1] https://lore.kernel.org/rust-for-linux/ZwUYmunVpzpexGV8@boqun-archlinux/
> 
> Abdiel Janulgue (5):
>   rust: types: add `Owned` type and `Ownable` trait
>   rust: page: Make ownership of the page pointer explicit.
>   rust: page: Extend support to vmalloc_to_page
>   rust: page: Add page_slice_to_page
>   rust: firmware: implement `Ownable` for Firmware
> 
>  rust/kernel/firmware.rs |  31 ++++++-----
>  rust/kernel/page.rs     | 116 +++++++++++++++++++++++++++++++++++-----
>  rust/kernel/types.rs    |  62 +++++++++++++++++++++
>  3 files changed, 184 insertions(+), 25 deletions(-)
> 
> 
> base-commit: 15541c9263ce34ff95a06bc68f45d9bc5c990bcd
> -- 
> 2.43.0
> 

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Danilo Krummrich]

On Wed, Oct 23, 2024 at 01:44:45AM +0300, Abdiel Janulgue wrote:
> Add the 'Owned' type, a simple smart pointer type that owns the
> underlying data.
> 
> An object implementing `Ownable' can constructed by wrapping it in
> `Owned`, which has the advantage of allowing fine-grained control
> over it's resource allocation and deallocation.
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index ced143600eb1..3f632916bd4d 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -429,3 +429,65 @@ pub enum Either<L, R> {
>      /// Constructs an instance of [`Either`] containing a value of type `R`.
>      Right(R),
>  }
> +
> +/// A smart pointer that owns the underlying data `T`.
> +///
> +/// This is a simple smart pointer that owns the underlying data. Typically, this would be
> +/// returned as a wrapper for `T` in `T`'s constructor.
> +/// When an object adds an option of being constructed this way, in addition to implementing
> +/// `Drop`, it implements `Ownable` as well, thus having finer-grained control in where
> +/// resource allocation and deallocation happens.
> +///
> +/// # Invariants
> +///
> +/// The pointer is always valid and owns the underlying data.
> +pub struct Owned<T: Ownable> {
> +    ptr: NonNull<T>,
> +}
> +
> +impl<T: Ownable> Owned<T> {
> +    /// Creates a new smart pointer that owns `T`.
> +    ///
> +    /// # Safety
> +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> +    /// in other words, no other entity should free the underlying data.
> +    pub unsafe fn to_owned(ptr: *mut T) -> Self {
> +	// SAFETY: Per function safety requirement.
> +	Self { ptr: unsafe { NonNull::new_unchecked(ptr) } }
> +    }

I wonder if this should just be

   pub fn new(ptr: NonNull<T>) -> Self

This way the caller could decide whether to use the fallible variant
`NonNull::new` or `NonNull::new_unchecked`.

Alternatively, you could have your own `new` and `new_unchecked` methods, but
that seems a bit redundant.

Sometimes this might be more elegant. For instance in the page code, as it is
now, you have to give up on

   let page = NonNull::new(page).ok_or(AllocError)?;

and instead have to do a NULL check by hand for the subsequent unsafe call to
`Owned::to_owned`.

> +}
> +
> +impl<T: Ownable> Deref for Owned<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl<T: Ownable> DerefMut for Owned<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_mut() }
> +    }
> +}
> +
> +/// An Ownable type is a type that can be put into `Owned<T>`, and when `Owned<T>` drops,
> +/// `ptr_drop` will be called.
> +pub unsafe trait Ownable {
> +    /// # Safety
> +    /// This could only be called in the `Owned::drop` function.
> +    unsafe fn ptr_drop(ptr: *mut Self);
> +}
> +
> +impl<T: Ownable> Drop for Owned<T> {
> +    fn drop(&mut self) {
> +	// SAFETY: In Owned<T>::drop.
> +	unsafe {
> +	    <T as Ownable>::ptr_drop(self.ptr.as_mut());
> +	}
> +    }
> +}
> -- 
> 2.43.0
> 

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

[Danilo Krummrich]

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

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

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

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

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

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

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

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

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

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

I think those should all return `EINVAL` instead.

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

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

[Danilo Krummrich]

On Wed, Oct 23, 2024 at 10:42:27AM +0200, Danilo Krummrich wrote:
> On Wed, Oct 23, 2024 at 01:44:47AM +0300, Abdiel Janulgue wrote:
> > Extend Page to support pages that are not allocated by the constructor, for
> > example, those returned by vmalloc_to_page(). Since we don't own those pages
> > we shouldn't Drop them either. Hence we take advantage of the switch to Opaque
> > so we can cast to a Page pointer from a struct page pointer and be able to
> > retrieve the reference on an existing struct page mapping. In this case
> > no destructor will be called since we are not instantiating a new Page instance.
> > 
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > ---
> >  rust/kernel/page.rs | 32 ++++++++++++++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> > index a8288c15b860..465928986f4b 100644
> > --- a/rust/kernel/page.rs
> > +++ b/rust/kernel/page.rs
> > @@ -31,11 +31,12 @@ pub const fn page_align(addr: usize) -> usize {
> >      (addr + (PAGE_SIZE - 1)) & PAGE_MASK
> >  }
> >  
> > -/// A pointer to a page that owns the page allocation.
> > +/// A pointer to a page that may own the page allocation.
> >  ///
> >  /// # Invariants
> >  ///
> > -/// The pointer is valid, and has ownership over the page.
> > +/// The pointer is valid, and has ownership over the page if the page is allocated by this
> > +/// abstraction.
> >  #[repr(transparent)]
> >  pub struct Page {
> >      page: Opaque<bindings::page>,
> > @@ -88,6 +89,33 @@ pub fn alloc_page(flags: Flags) -> Result<Owned<Self>, AllocError> {
> >          Ok(unsafe { Owned::to_owned(ptr) })
> >      }
> >  
> > +    /// This is just a wrapper to vmalloc_to_page which returns an existing page mapping, hence
> 
> In documentation, try to avoid filler words, such as "just". Better say
> something like:
> 
> "This is an abstraction around the C `vmalloc_to_page()` function. Note that by
> a call to this function the caller doesn't take ownership of the returned `Page`
> [...]."
> 
> > +    /// we don't take ownership of the page. Returns an error if the pointer is null or if it
> > +    /// is not returned by vmalloc().
> > +    pub fn vmalloc_to_page<'a>(
> > +        cpu_addr: *const core::ffi::c_void
> 
> When you have a raw pointer argument in your function it becomes unsafe by
> definition.

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

> 
> I also think it would also be better to pass a `NonNull<u8>` instead.
> 
> > +    ) -> Result<&'a Self, AllocError>
> 
> Please don't use `AllocError`. We're not allocating anything here.
> 
> Anyway, do we need this as a separate function at all?
> 
> > +    {
> > +        if cpu_addr.is_null() {
> > +            return Err(AllocError);
> > +        }
> > +        // SAFETY: We've checked that the pointer is not null, so it is safe to call this method.
> > +        if unsafe { !bindings::is_vmalloc_addr(cpu_addr) } {
> > +            return Err(AllocError);
> > +        }
> > +        // SAFETY: We've initially ensured the pointer argument to this function is not null and
> > +        // checked for the requirement the the buffer passed to it should be allocated by vmalloc,
> > +        // so it is safe to call this method.

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

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

> > +        let page = unsafe { bindings::vmalloc_to_page(cpu_addr) };
> > +        if page.is_null() {
> > +            return Err(AllocError);
> > +        }
> 
> I think those should all return `EINVAL` instead.
> 
> > +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::page`.
> > +        // SAFETY: We just successfully allocated a page, therefore dereferencing
> > +        // the page pointer is valid.
> > +        Ok(unsafe { &*page.cast() })
> > +    }
> > +
> >      /// Returns a raw pointer to the page.
> >      pub fn as_ptr(&self) -> *mut bindings::page {
> >          self.page.get()
> > -- 
> > 2.43.0
> > 
> 

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Alice Ryhl]

On Wed, Oct 23, 2024 at 12:49 AM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> Add the 'Owned' type, a simple smart pointer type that owns the
> underlying data.
>
> An object implementing `Ownable' can constructed by wrapping it in
> `Owned`, which has the advantage of allowing fine-grained control
> over it's resource allocation and deallocation.
>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index ced143600eb1..3f632916bd4d 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -429,3 +429,65 @@ pub enum Either<L, R> {
>      /// Constructs an instance of [`Either`] containing a value of type `R`.
>      Right(R),
>  }
> +
> +/// A smart pointer that owns the underlying data `T`.
> +///
> +/// This is a simple smart pointer that owns the underlying data. Typically, this would be
> +/// returned as a wrapper for `T` in `T`'s constructor.
> +/// When an object adds an option of being constructed this way, in addition to implementing
> +/// `Drop`, it implements `Ownable` as well, thus having finer-grained control in where
> +/// resource allocation and deallocation happens.
> +///
> +/// # Invariants
> +///
> +/// The pointer is always valid and owns the underlying data.
> +pub struct Owned<T: Ownable> {
> +    ptr: NonNull<T>,
> +}
> +
> +impl<T: Ownable> Owned<T> {
> +    /// Creates a new smart pointer that owns `T`.
> +    ///
> +    /// # Safety
> +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> +    /// in other words, no other entity should free the underlying data.
> +    pub unsafe fn to_owned(ptr: *mut T) -> Self {

Please rename this function to from_raw to match the name used by
other similar functions.

Also, I don't love this wording. We don't really want to guarantee
that it is unique. For example, pages have one primary owner, but
there can be others who also have refcounts to the page, so it's not
really unique. I think you just want to say that `ptr` must point at a
valid value of type `T`, and it must remain valid until `ptr_drop` is
called.

> +impl<T: Ownable> Deref for Owned<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl<T: Ownable> DerefMut for Owned<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_mut() }
> +    }
> +}

We only want Deref, not DerefMut. DerefMut both requires uniqueness in
a way that is stronger than what we can really promise, and it also
implies that the value is *not* pinned, but we generally want to use
Owned with pinned things. Thus, we can't use DerefMut.

> +/// An Ownable type is a type that can be put into `Owned<T>`, and when `Owned<T>` drops,
> +/// `ptr_drop` will be called.
> +pub unsafe trait Ownable {
> +    /// # Safety
> +    /// This could only be called in the `Owned::drop` function.
> +    unsafe fn ptr_drop(ptr: *mut Self);
> +}
> +
> +impl<T: Ownable> Drop for Owned<T> {
> +    fn drop(&mut self) {
> +       // SAFETY: In Owned<T>::drop.
> +       unsafe {
> +           <T as Ownable>::ptr_drop(self.ptr.as_mut());

This uses NonNull::as_mut which creates a mutable reference. You
should use NonNull::as_ptr instead.

Also this code will look better if you move the semicolon so it is
outside of the unsafe block.


Alice

Re: [PATCH v2 5/5] rust: firmware: implement `Ownable` for Firmware

[Danilo Krummrich]

On Wed, Oct 23, 2024 at 01:44:49AM +0300, Abdiel Janulgue wrote:
> For consistency, wrap the firmware as an `Owned` smart pointer in the
> constructor.
> 
> Cc: Danilo Krummrich <dakr@redhat.com>
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index dee5b4b18aec..6da834b37455 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -4,8 +4,8 @@
>  //!
>  //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h)
>  
> -use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
> -use core::ptr::NonNull;
> +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
> +            types::{Opaque, Owned, Ownable}};
>  
>  /// # Invariants
>  ///
> @@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
>  /// # Ok(())
>  /// # }
>  /// ```
> -pub struct Firmware(NonNull<bindings::firmware>);
> + #[repr(transparent)]
> +pub struct Firmware(Opaque<bindings::firmware>);
>  
>  impl Firmware {
> -    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> +    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {

I think it's fine to implement this for consistency, but I'm not sure I like
that drivers have to refer to it as `Owned<Firmware>`.

Anyway, if we keep it this way the patch also needs the following change.

diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index 6da834b37455..1db854eb2422 100644
--- a/rust/kernel/firmware.rs
+++ b/rust/kernel/firmware.rs
@@ -115,8 +115,8 @@ unsafe fn ptr_drop(ptr: *mut Self) {

 // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, which is safe to be used from
 // any thread.
-unsafe impl Send for Firmware {}
+unsafe impl Send for Owned<Firmware> {}

 // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
 // be used from any thread.
-unsafe impl Sync for Firmware {}
+unsafe impl Sync for Owned<Firmware> {}

>          let mut fw: *mut bindings::firmware = core::ptr::null_mut();
>          let pfw: *mut *mut bindings::firmware = &mut fw;
>  
> @@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
>          if ret != 0 {
>              return Err(Error::from_errno(ret));
>          }
> -
> +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
> +        let ptr = fw.cast::<Self>();
>          // SAFETY: `func` not bailing out with a non-zero error code, guarantees that `fw` is a
>          // valid pointer to `bindings::firmware`.
> -        Ok(Firmware(unsafe { NonNull::new_unchecked(fw) }))
> +        Ok(unsafe { Owned::to_owned(ptr) })
>      }
>  
>      /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> -    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> +    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
>          Self::request_internal(name, dev, FwFunc::request())
>      }
>  
>      /// Send a request for an optional firmware module. See also
>      /// `bindings::firmware_request_nowarn`.
> -    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
>          Self::request_internal(name, dev, FwFunc::request_nowarn())
>      }
>  
>      fn as_raw(&self) -> *mut bindings::firmware {
> -        self.0.as_ptr()
> +        self.0.get()
>      }
>  
>      /// Returns the size of the requested firmware in bytes.
> @@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
>      }
>  }
>  
> -impl Drop for Firmware {
> -    fn drop(&mut self) {
> -        // SAFETY: `self.as_raw()` is valid by the type invariant.
> -        unsafe { bindings::release_firmware(self.as_raw()) };
> +unsafe impl Ownable for Firmware {
> +    unsafe fn ptr_drop(ptr: *mut Self) {
> +        // SAFETY:
> +        // - By the type invariants, we have ownership of the ptr and can free it.
> +        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
> +        //   unique pointer to object, it's safe to release the firmware.
> +        unsafe { bindings::release_firmware(ptr.cast()) };
>      }
>  }
>  
> -- 
> 2.43.0
> 

Re: [PATCH v2 5/5] rust: firmware: implement `Ownable` for Firmware

[Danilo Krummrich]

On Wed, Oct 23, 2024 at 11:35:20AM +0200, Danilo Krummrich wrote:
> On Wed, Oct 23, 2024 at 01:44:49AM +0300, Abdiel Janulgue wrote:
> > For consistency, wrap the firmware as an `Owned` smart pointer in the
> > constructor.
> > 
> > Cc: Danilo Krummrich <dakr@redhat.com>
> > Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > ---
> >  rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index dee5b4b18aec..6da834b37455 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -4,8 +4,8 @@
> >  //!
> >  //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h)
> >  
> > -use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
> > -use core::ptr::NonNull;
> > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
> > +            types::{Opaque, Owned, Ownable}};
> >  
> >  /// # Invariants
> >  ///
> > @@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
> >  /// # Ok(())
> >  /// # }
> >  /// ```
> > -pub struct Firmware(NonNull<bindings::firmware>);
> > + #[repr(transparent)]
> > +pub struct Firmware(Opaque<bindings::firmware>);
> >  
> >  impl Firmware {
> > -    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > +    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {
> 
> I think it's fine to implement this for consistency, but I'm not sure I like
> that drivers have to refer to it as `Owned<Firmware>`.
> 
> Anyway, if we keep it this way the patch also needs the following change.
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 6da834b37455..1db854eb2422 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -115,8 +115,8 @@ unsafe fn ptr_drop(ptr: *mut Self) {
> 
>  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, which is safe to be used from
>  // any thread.
> -unsafe impl Send for Firmware {}
> +unsafe impl Send for Owned<Firmware> {}
> 
>  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
>  // be used from any thread.
> -unsafe impl Sync for Firmware {}
> +unsafe impl Sync for Owned<Firmware> {}

Actually, I think `Owned` should implement `Send` and `Sync` like this instead.

   unsafe impl<T> Sync for Owned<T> where T: Sync + Ownable {}
   unsafe impl<T> Send for Owned<T> where T: Send + Ownable {}

> 
> >          let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> >          let pfw: *mut *mut bindings::firmware = &mut fw;
> >  
> > @@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> >          if ret != 0 {
> >              return Err(Error::from_errno(ret));
> >          }
> > -
> > +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
> > +        let ptr = fw.cast::<Self>();
> >          // SAFETY: `func` not bailing out with a non-zero error code, guarantees that `fw` is a
> >          // valid pointer to `bindings::firmware`.
> > -        Ok(Firmware(unsafe { NonNull::new_unchecked(fw) }))
> > +        Ok(unsafe { Owned::to_owned(ptr) })
> >      }
> >  
> >      /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> > -    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > +    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> >          Self::request_internal(name, dev, FwFunc::request())
> >      }
> >  
> >      /// Send a request for an optional firmware module. See also
> >      /// `bindings::firmware_request_nowarn`.
> > -    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> > +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> >          Self::request_internal(name, dev, FwFunc::request_nowarn())
> >      }
> >  
> >      fn as_raw(&self) -> *mut bindings::firmware {
> > -        self.0.as_ptr()
> > +        self.0.get()
> >      }
> >  
> >      /// Returns the size of the requested firmware in bytes.
> > @@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
> >      }
> >  }
> >  
> > -impl Drop for Firmware {
> > -    fn drop(&mut self) {
> > -        // SAFETY: `self.as_raw()` is valid by the type invariant.
> > -        unsafe { bindings::release_firmware(self.as_raw()) };
> > +unsafe impl Ownable for Firmware {
> > +    unsafe fn ptr_drop(ptr: *mut Self) {
> > +        // SAFETY:
> > +        // - By the type invariants, we have ownership of the ptr and can free it.
> > +        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
> > +        //   unique pointer to object, it's safe to release the firmware.
> > +        unsafe { bindings::release_firmware(ptr.cast()) };
> >      }
> >  }
> >  
> > -- 
> > 2.43.0
> > 

Re: [PATCH v2 0/5] Introduce Owned type and Ownable trait (was: "rust: page: Add support for vmalloc_to_page")

[Miguel Ojeda]

On Wed, Oct 23, 2024 at 10:03 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> I wonder if it would make sense to make `CLIPPY=1` the default and only disable
> it by explicitly passing `CLIPPY=0`.

That is what I wanted, but when I asked long ago to the Clippy
maintainers if using `clippy-driver` was guaranteed to not affect
codegen, the answer was no: for instance, optimization may be affected
(at least back then), and the maintainers said the intention is that
is not to be used for normal compiling. So I sent a PR to document
that. See:

    https://github.com/rust-lang/rust-clippy/issues/8035
    https://github.com/rust-lang/rust-clippy/pull/8037

Similarly, Christian proposed running `rustfmtcheck` unconditionally
on build and offering a way to turn it off instead. I think that would
be ideal too, but it could potentially lead to problems too, so I am
not sure either; see e.g.:

    https://lore.kernel.org/rust-for-linux/CANiq72==AkkqCDaZMENQRg8cf4zdeHpTHwdWS3sZiFWm0vyJUA@mail.gmail.com/

So I wonder if we should instead go with a "dev mode" like `D=1` that
enables Clippy, `rustfmtcheck`, `-Dwarnings` (even if `WERROR=n` and
applying to everything, not just kernel objects,), potentially
`rustdoc`-related warnings too, and whatever other tooling/checks in
the future (e.g. klint), and not just for Rust but potentially for C
and other bits too (e.g. `W=1`, some important subset of Coccinelle
scripts...).

That way, "normal builds" (i.e. those done by users) stay as
fast/clean/warning-free/bug-free/optimized as possible even across
compiler versions, potential bugs in the toolchain, etc. And I imagine
it would be easier for newcomers, too.

Opinions welcome! I am happy to prepare an RFC, since it seems a few
people would like something like that.

Cheers,
Miguel

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Abdiel Janulgue]

On 23/10/2024 12:28, Alice Ryhl wrote:
> On Wed, Oct 23, 2024 at 12:49 AM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
>>
>> Add the 'Owned' type, a simple smart pointer type that owns the
>> underlying data.
>>
>> An object implementing `Ownable' can constructed by wrapping it in
>> `Owned`, which has the advantage of allowing fine-grained control
>> over it's resource allocation and deallocation.
>>
>> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>> ---
>>   rust/kernel/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 62 insertions(+)
>>
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index ced143600eb1..3f632916bd4d 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -429,3 +429,65 @@ pub enum Either<L, R> {
>>       /// Constructs an instance of [`Either`] containing a value of type `R`.
>>       Right(R),
>>   }
>> +
>> +/// A smart pointer that owns the underlying data `T`.
>> +///
>> +/// This is a simple smart pointer that owns the underlying data. Typically, this would be
>> +/// returned as a wrapper for `T` in `T`'s constructor.
>> +/// When an object adds an option of being constructed this way, in addition to implementing
>> +/// `Drop`, it implements `Ownable` as well, thus having finer-grained control in where
>> +/// resource allocation and deallocation happens.
>> +///
>> +/// # Invariants
>> +///
>> +/// The pointer is always valid and owns the underlying data.
>> +pub struct Owned<T: Ownable> {
>> +    ptr: NonNull<T>,
>> +}
>> +
>> +impl<T: Ownable> Owned<T> {
>> +    /// Creates a new smart pointer that owns `T`.
>> +    ///
>> +    /// # Safety
>> +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
>> +    /// in other words, no other entity should free the underlying data.
>> +    pub unsafe fn to_owned(ptr: *mut T) -> Self {
> 
> Please rename this function to from_raw to match the name used by
> other similar functions.
> 
> Also, I don't love this wording. We don't really want to guarantee
> that it is unique. For example, pages have one primary owner, but
> there can be others who also have refcounts to the page, so it's not
> really unique. I think you just want to say that `ptr` must point at a
> valid value of type `T`, and it must remain valid until `ptr_drop` is
> called.
> 
>> +impl<T: Ownable> Deref for Owned<T> {
>> +    type Target = T;
>> +
>> +    fn deref(&self) -> &Self::Target {
>> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
>> +        // safe to dereference it.
>> +        unsafe { self.ptr.as_ref() }
>> +    }
>> +}
>> +
>> +impl<T: Ownable> DerefMut for Owned<T> {
>> +    fn deref_mut(&mut self) -> &mut Self::Target {
>> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
>> +        // safe to dereference it.
>> +        unsafe { self.ptr.as_mut() }
>> +    }
>> +}
> 
> We only want Deref, not DerefMut. DerefMut both requires uniqueness in
> a way that is stronger than what we can really promise, and it also
> implies that the value is *not* pinned, but we generally want to use
> Owned with pinned things. Thus, we can't use DerefMut.
> 
>> +/// An Ownable type is a type that can be put into `Owned<T>`, and when `Owned<T>` drops,
>> +/// `ptr_drop` will be called.
>> +pub unsafe trait Ownable {
>> +    /// # Safety
>> +    /// This could only be called in the `Owned::drop` function.
>> +    unsafe fn ptr_drop(ptr: *mut Self);
>> +}
>> +
>> +impl<T: Ownable> Drop for Owned<T> {
>> +    fn drop(&mut self) {
>> +       // SAFETY: In Owned<T>::drop.
>> +       unsafe {
>> +           <T as Ownable>::ptr_drop(self.ptr.as_mut());
> 
> This uses NonNull::as_mut which creates a mutable reference. You
> should use NonNull::as_ptr instead.
> 
> Also this code will look better if you move the semicolon so it is
> outside of the unsafe block.

Thanks for the feedback! Will do that in next revision.

/Abdiel

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

[Abdiel Janulgue]

On 23/10/2024 11:42, Danilo Krummrich wrote:
>> +    ) -> Result<&'a Self, AllocError>
> 
> Please don't use `AllocError`. We're not allocating anything here.
> 
> Anyway, do we need this as a separate function at all?
Thanks. Would it make sense to squash this function into 
`Page::page_slice_to_page` instead?

/Abdiel

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

[Danilo Krummrich]

On Wed, Oct 23, 2024 at 01:26:37PM +0300, Abdiel Janulgue wrote:
> On 23/10/2024 11:42, Danilo Krummrich wrote:
> > > +    ) -> Result<&'a Self, AllocError>
> > 
> > Please don't use `AllocError`. We're not allocating anything here.
> > 
> > Anyway, do we need this as a separate function at all?
> Thanks. Would it make sense to squash this function into
> `Page::page_slice_to_page` instead?

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

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

> 
> /Abdiel
> 

Re: [PATCH v2 0/5] Introduce Owned type and Ownable trait (was: "rust: page: Add support for vmalloc_to_page")

[Danilo Krummrich]

On Wed, Oct 23, 2024 at 11:51:47AM +0200, Miguel Ojeda wrote:
> On Wed, Oct 23, 2024 at 10:03 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > I wonder if it would make sense to make `CLIPPY=1` the default and only disable
> > it by explicitly passing `CLIPPY=0`.
> 
> That is what I wanted, but when I asked long ago to the Clippy
> maintainers if using `clippy-driver` was guaranteed to not affect
> codegen, the answer was no: for instance, optimization may be affected
> (at least back then), and the maintainers said the intention is that
> is not to be used for normal compiling. So I sent a PR to document
> that. See:
> 
>     https://github.com/rust-lang/rust-clippy/issues/8035
>     https://github.com/rust-lang/rust-clippy/pull/8037

That's pretty unfortunate, I didn't know.

I think for the long term it'd be good to find a way though. Once more and more
subsystems / people start adding Rust code, I could imagine patches to start
slipping through and fixing things up after it's been discovered in -next would
be painful.

> 
> Similarly, Christian proposed running `rustfmtcheck` unconditionally
> on build and offering a way to turn it off instead. I think that would
> be ideal too, but it could potentially lead to problems too, so I am
> not sure either; see e.g.:
> 
>     https://lore.kernel.org/rust-for-linux/CANiq72==AkkqCDaZMENQRg8cf4zdeHpTHwdWS3sZiFWm0vyJUA@mail.gmail.com/

Yeah, that's a tricky one; if not enabled by default I'd be a bit worried about
the same thing to happen as mentioned above.

Additionally, for development trees where things slipped through it'd be
annoying when `rustfmt` changes more stuff than expected.

> 
> So I wonder if we should instead go with a "dev mode" like `D=1` that
> enables Clippy, `rustfmtcheck`, `-Dwarnings` (even if `WERROR=n` and
> applying to everything, not just kernel objects,), potentially
> `rustdoc`-related warnings too, and whatever other tooling/checks in
> the future (e.g. klint), and not just for Rust but potentially for C
> and other bits too (e.g. `W=1`, some important subset of Coccinelle
> scripts...).

I think that'd be great for short / mid term, it'd make it much easier to ensure
that all relevant checks were executed and hence make it less likely for things
slip through.

For the long term, I think it'd be great to keep looking for ways to always
enable the clippy and format checks. Or at least the clippy ones if we're too
concerned about `rustfmt` to break.

> 
> That way, "normal builds" (i.e. those done by users) stay as
> fast/clean/warning-free/bug-free/optimized as possible even across
> compiler versions, potential bugs in the toolchain, etc. And I imagine
> it would be easier for newcomers, too.
> 
> Opinions welcome! I am happy to prepare an RFC, since it seems a few
> people would like something like that.
> 
> Cheers,
> Miguel
> 

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Boqun Feng]

On Wed, Oct 23, 2024 at 01:26:14PM +0300, Abdiel Janulgue wrote:
> 
> 
> On 23/10/2024 12:28, Alice Ryhl wrote:
> > On Wed, Oct 23, 2024 at 12:49 AM Abdiel Janulgue
> > <abdiel.janulgue@gmail.com> wrote:
> > > 
> > > Add the 'Owned' type, a simple smart pointer type that owns the
> > > underlying data.
> > > 
> > > An object implementing `Ownable' can constructed by wrapping it in
> > > `Owned`, which has the advantage of allowing fine-grained control
> > > over it's resource allocation and deallocation.
> > > 
> > > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > > ---
> > >   rust/kernel/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 62 insertions(+)
> > > 
> > > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > > index ced143600eb1..3f632916bd4d 100644
> > > --- a/rust/kernel/types.rs
> > > +++ b/rust/kernel/types.rs
> > > @@ -429,3 +429,65 @@ pub enum Either<L, R> {
> > >       /// Constructs an instance of [`Either`] containing a value of type `R`.
> > >       Right(R),
> > >   }
> > > +
> > > +/// A smart pointer that owns the underlying data `T`.
> > > +///
> > > +/// This is a simple smart pointer that owns the underlying data. Typically, this would be
> > > +/// returned as a wrapper for `T` in `T`'s constructor.
> > > +/// When an object adds an option of being constructed this way, in addition to implementing
> > > +/// `Drop`, it implements `Ownable` as well, thus having finer-grained control in where
> > > +/// resource allocation and deallocation happens.
> > > +///
> > > +/// # Invariants
> > > +///
> > > +/// The pointer is always valid and owns the underlying data.
> > > +pub struct Owned<T: Ownable> {
> > > +    ptr: NonNull<T>,
> > > +}
> > > +
> > > +impl<T: Ownable> Owned<T> {
> > > +    /// Creates a new smart pointer that owns `T`.
> > > +    ///
> > > +    /// # Safety
> > > +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> > > +    /// in other words, no other entity should free the underlying data.
> > > +    pub unsafe fn to_owned(ptr: *mut T) -> Self {
> > 
> > Please rename this function to from_raw to match the name used by
> > other similar functions.
> > 
> > Also, I don't love this wording. We don't really want to guarantee
> > that it is unique. For example, pages have one primary owner, but
> > there can be others who also have refcounts to the page, so it's not
> > really unique. I think you just want to say that `ptr` must point at a

But then when `Owned<Page>` dropped, it will call __free_pages() which
invalidate any other existing users. Do you assume that the users will
use pointers anyway, so it's their unsafe responsiblity to guarantee
that they don't use an invalid pointer?

Also I assume you mean the others have refcounts to the page *before* an
`Owned<Page>` is created, right? Because if we really have a use case
where we want to have multiple users of a page after `Owned<Page>`
created, we should better provide a `Owned<Page>` to `ARef<Page>`
function.

Regards,
Boqun

> > valid value of type `T`, and it must remain valid until `ptr_drop` is
> > called.
> > 
> > > +impl<T: Ownable> Deref for Owned<T> {
> > > +    type Target = T;
> > > +
> > > +    fn deref(&self) -> &Self::Target {
> > > +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> > > +        // safe to dereference it.
> > > +        unsafe { self.ptr.as_ref() }
> > > +    }
> > > +}
> > > +
> > > +impl<T: Ownable> DerefMut for Owned<T> {
> > > +    fn deref_mut(&mut self) -> &mut Self::Target {
> > > +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> > > +        // safe to dereference it.
> > > +        unsafe { self.ptr.as_mut() }
> > > +    }
> > > +}
> > 
> > We only want Deref, not DerefMut. DerefMut both requires uniqueness in
> > a way that is stronger than what we can really promise, and it also
> > implies that the value is *not* pinned, but we generally want to use
> > Owned with pinned things. Thus, we can't use DerefMut.
> > 
> > > +/// An Ownable type is a type that can be put into `Owned<T>`, and when `Owned<T>` drops,
> > > +/// `ptr_drop` will be called.
> > > +pub unsafe trait Ownable {
> > > +    /// # Safety
> > > +    /// This could only be called in the `Owned::drop` function.
> > > +    unsafe fn ptr_drop(ptr: *mut Self);
> > > +}
> > > +
> > > +impl<T: Ownable> Drop for Owned<T> {
> > > +    fn drop(&mut self) {
> > > +       // SAFETY: In Owned<T>::drop.
> > > +       unsafe {
> > > +           <T as Ownable>::ptr_drop(self.ptr.as_mut());
> > 
> > This uses NonNull::as_mut which creates a mutable reference. You
> > should use NonNull::as_ptr instead.
> > 
> > Also this code will look better if you move the semicolon so it is
> > outside of the unsafe block.
> 
> Thanks for the feedback! Will do that in next revision.
> 
> /Abdiel

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Alice Ryhl]

On Wed, Oct 23, 2024 at 4:58 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Oct 23, 2024 at 01:26:14PM +0300, Abdiel Janulgue wrote:
> >
> >
> > On 23/10/2024 12:28, Alice Ryhl wrote:
> > > On Wed, Oct 23, 2024 at 12:49 AM Abdiel Janulgue
> > > <abdiel.janulgue@gmail.com> wrote:
> > > >
> > > > Add the 'Owned' type, a simple smart pointer type that owns the
> > > > underlying data.
> > > >
> > > > An object implementing `Ownable' can constructed by wrapping it in
> > > > `Owned`, which has the advantage of allowing fine-grained control
> > > > over it's resource allocation and deallocation.
> > > >
> > > > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > > > ---
> > > >   rust/kernel/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 62 insertions(+)
> > > >
> > > > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > > > index ced143600eb1..3f632916bd4d 100644
> > > > --- a/rust/kernel/types.rs
> > > > +++ b/rust/kernel/types.rs
> > > > @@ -429,3 +429,65 @@ pub enum Either<L, R> {
> > > >       /// Constructs an instance of [`Either`] containing a value of type `R`.
> > > >       Right(R),
> > > >   }
> > > > +
> > > > +/// A smart pointer that owns the underlying data `T`.
> > > > +///
> > > > +/// This is a simple smart pointer that owns the underlying data. Typically, this would be
> > > > +/// returned as a wrapper for `T` in `T`'s constructor.
> > > > +/// When an object adds an option of being constructed this way, in addition to implementing
> > > > +/// `Drop`, it implements `Ownable` as well, thus having finer-grained control in where
> > > > +/// resource allocation and deallocation happens.
> > > > +///
> > > > +/// # Invariants
> > > > +///
> > > > +/// The pointer is always valid and owns the underlying data.
> > > > +pub struct Owned<T: Ownable> {
> > > > +    ptr: NonNull<T>,
> > > > +}
> > > > +
> > > > +impl<T: Ownable> Owned<T> {
> > > > +    /// Creates a new smart pointer that owns `T`.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> > > > +    /// in other words, no other entity should free the underlying data.
> > > > +    pub unsafe fn to_owned(ptr: *mut T) -> Self {
> > >
> > > Please rename this function to from_raw to match the name used by
> > > other similar functions.
> > >
> > > Also, I don't love this wording. We don't really want to guarantee
> > > that it is unique. For example, pages have one primary owner, but
> > > there can be others who also have refcounts to the page, so it's not
> > > really unique. I think you just want to say that `ptr` must point at a
>
> But then when `Owned<Page>` dropped, it will call __free_pages() which
> invalidate any other existing users. Do you assume that the users will
> use pointers anyway, so it's their unsafe responsiblity to guarantee
> that they don't use an invalid pointer?
>
> Also I assume you mean the others have refcounts to the page *before* an
> `Owned<Page>` is created, right? Because if we really have a use case
> where we want to have multiple users of a page after `Owned<Page>`
> created, we should better provide a `Owned<Page>` to `ARef<Page>`
> function.

The __free_pages function just decrements a refcount. If there are
other references to it, it's not actually freed.

Alice

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Boqun Feng]

On Wed, Oct 23, 2024 at 07:52:23PM +0200, Alice Ryhl wrote:
[...]
> > > > > +impl<T: Ownable> Owned<T> {
> > > > > +    /// Creates a new smart pointer that owns `T`.
> > > > > +    ///
> > > > > +    /// # Safety
> > > > > +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> > > > > +    /// in other words, no other entity should free the underlying data.
> > > > > +    pub unsafe fn to_owned(ptr: *mut T) -> Self {
> > > >
> > > > Please rename this function to from_raw to match the name used by
> > > > other similar functions.
> > > >
> > > > Also, I don't love this wording. We don't really want to guarantee
> > > > that it is unique. For example, pages have one primary owner, but
> > > > there can be others who also have refcounts to the page, so it's not
> > > > really unique. I think you just want to say that `ptr` must point at a
> >
> > But then when `Owned<Page>` dropped, it will call __free_pages() which
> > invalidate any other existing users. Do you assume that the users will
> > use pointers anyway, so it's their unsafe responsiblity to guarantee
> > that they don't use an invalid pointer?
> >
> > Also I assume you mean the others have refcounts to the page *before* an
> > `Owned<Page>` is created, right? Because if we really have a use case
> > where we want to have multiple users of a page after `Owned<Page>`
> > created, we should better provide a `Owned<Page>` to `ARef<Page>`
> > function.
> 
> The __free_pages function just decrements a refcount. If there are
> other references to it, it's not actually freed.
> 

Then why don't we use page_put() there? ;-) And instead of
`Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?

Regards,
Boqun

> Alice

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Alice Ryhl]

On Wed, Oct 23, 2024 at 8:07 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Oct 23, 2024 at 07:52:23PM +0200, Alice Ryhl wrote:
> [...]
> > > > > > +impl<T: Ownable> Owned<T> {
> > > > > > +    /// Creates a new smart pointer that owns `T`.
> > > > > > +    ///
> > > > > > +    /// # Safety
> > > > > > +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> > > > > > +    /// in other words, no other entity should free the underlying data.
> > > > > > +    pub unsafe fn to_owned(ptr: *mut T) -> Self {
> > > > >
> > > > > Please rename this function to from_raw to match the name used by
> > > > > other similar functions.
> > > > >
> > > > > Also, I don't love this wording. We don't really want to guarantee
> > > > > that it is unique. For example, pages have one primary owner, but
> > > > > there can be others who also have refcounts to the page, so it's not
> > > > > really unique. I think you just want to say that `ptr` must point at a
> > >
> > > But then when `Owned<Page>` dropped, it will call __free_pages() which
> > > invalidate any other existing users. Do you assume that the users will
> > > use pointers anyway, so it's their unsafe responsiblity to guarantee
> > > that they don't use an invalid pointer?
> > >
> > > Also I assume you mean the others have refcounts to the page *before* an
> > > `Owned<Page>` is created, right? Because if we really have a use case
> > > where we want to have multiple users of a page after `Owned<Page>`
> > > created, we should better provide a `Owned<Page>` to `ARef<Page>`
> > > function.
> >
> > The __free_pages function just decrements a refcount. If there are
> > other references to it, it's not actually freed.
> >
>
> Then why don't we use page_put() there? ;-) And instead of
> `Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?

I don't think there's a function called page_put?

Alice

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Alice Ryhl]

On Thu, Oct 24, 2024 at 9:23 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Wed, Oct 23, 2024 at 8:07 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Wed, Oct 23, 2024 at 07:52:23PM +0200, Alice Ryhl wrote:
> > [...]
> > > > > > > +impl<T: Ownable> Owned<T> {
> > > > > > > +    /// Creates a new smart pointer that owns `T`.
> > > > > > > +    ///
> > > > > > > +    /// # Safety
> > > > > > > +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> > > > > > > +    /// in other words, no other entity should free the underlying data.
> > > > > > > +    pub unsafe fn to_owned(ptr: *mut T) -> Self {
> > > > > >
> > > > > > Please rename this function to from_raw to match the name used by
> > > > > > other similar functions.
> > > > > >
> > > > > > Also, I don't love this wording. We don't really want to guarantee
> > > > > > that it is unique. For example, pages have one primary owner, but
> > > > > > there can be others who also have refcounts to the page, so it's not
> > > > > > really unique. I think you just want to say that `ptr` must point at a
> > > >
> > > > But then when `Owned<Page>` dropped, it will call __free_pages() which
> > > > invalidate any other existing users. Do you assume that the users will
> > > > use pointers anyway, so it's their unsafe responsiblity to guarantee
> > > > that they don't use an invalid pointer?
> > > >
> > > > Also I assume you mean the others have refcounts to the page *before* an
> > > > `Owned<Page>` is created, right? Because if we really have a use case
> > > > where we want to have multiple users of a page after `Owned<Page>`
> > > > created, we should better provide a `Owned<Page>` to `ARef<Page>`
> > > > function.
> > >
> > > The __free_pages function just decrements a refcount. If there are
> > > other references to it, it's not actually freed.
> > >
> >
> > Then why don't we use page_put() there? ;-) And instead of
> > `Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?
>
> I don't think there's a function called page_put?

Sorry I confused myself. It's because it's called put_page.

Alice

Re: [PATCH v2 5/5] rust: firmware: implement `Ownable` for Firmware

[Boqun Feng]

On Wed, Oct 23, 2024 at 11:35:15AM +0200, Danilo Krummrich wrote:
> On Wed, Oct 23, 2024 at 01:44:49AM +0300, Abdiel Janulgue wrote:
> > For consistency, wrap the firmware as an `Owned` smart pointer in the
> > constructor.
> > 
> > Cc: Danilo Krummrich <dakr@redhat.com>
> > Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > ---
> >  rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index dee5b4b18aec..6da834b37455 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -4,8 +4,8 @@
> >  //!
> >  //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h)
> >  
> > -use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
> > -use core::ptr::NonNull;
> > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
> > +            types::{Opaque, Owned, Ownable}};
> >  
> >  /// # Invariants
> >  ///
> > @@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
> >  /// # Ok(())
> >  /// # }
> >  /// ```
> > -pub struct Firmware(NonNull<bindings::firmware>);
> > + #[repr(transparent)]
> > +pub struct Firmware(Opaque<bindings::firmware>);
> >  
> >  impl Firmware {
> > -    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > +    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {
> 
> I think it's fine to implement this for consistency, but I'm not sure I like
> that drivers have to refer to it as `Owned<Firmware>`.
> 

May I ask why not? ;-)

Ideally, we should not wrap a pointer to particular type, instead we
should wrap the type and then combine it with a meaningful pointer type,
e.g. Box<>, ARef<>, Owned<> ... in this way, we de-couple how the
lifetime of object is maintained (described by the pointer type) and
what operations are available on the object (described by the wrapper
type).

If later on, a firmware object creation is doable in pure Rust code for
some condition, we can then have a function that returns a
`KBox<Firmware>` (assume using kmalloc for the object), and it will just
work (tm).

Regards,
Boqun

> Anyway, if we keep it this way the patch also needs the following change.
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 6da834b37455..1db854eb2422 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -115,8 +115,8 @@ unsafe fn ptr_drop(ptr: *mut Self) {
> 
>  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, which is safe to be used from
>  // any thread.
> -unsafe impl Send for Firmware {}
> +unsafe impl Send for Owned<Firmware> {}
> 
>  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
>  // be used from any thread.
> -unsafe impl Sync for Firmware {}
> +unsafe impl Sync for Owned<Firmware> {}
> 
> >          let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> >          let pfw: *mut *mut bindings::firmware = &mut fw;
> >  
> > @@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> >          if ret != 0 {
> >              return Err(Error::from_errno(ret));
> >          }
> > -
> > +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
> > +        let ptr = fw.cast::<Self>();
> >          // SAFETY: `func` not bailing out with a non-zero error code, guarantees that `fw` is a
> >          // valid pointer to `bindings::firmware`.
> > -        Ok(Firmware(unsafe { NonNull::new_unchecked(fw) }))
> > +        Ok(unsafe { Owned::to_owned(ptr) })
> >      }
> >  
> >      /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> > -    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > +    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> >          Self::request_internal(name, dev, FwFunc::request())
> >      }
> >  
> >      /// Send a request for an optional firmware module. See also
> >      /// `bindings::firmware_request_nowarn`.
> > -    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> > +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> >          Self::request_internal(name, dev, FwFunc::request_nowarn())
> >      }
> >  
> >      fn as_raw(&self) -> *mut bindings::firmware {
> > -        self.0.as_ptr()
> > +        self.0.get()
> >      }
> >  
> >      /// Returns the size of the requested firmware in bytes.
> > @@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
> >      }
> >  }
> >  
> > -impl Drop for Firmware {
> > -    fn drop(&mut self) {
> > -        // SAFETY: `self.as_raw()` is valid by the type invariant.
> > -        unsafe { bindings::release_firmware(self.as_raw()) };
> > +unsafe impl Ownable for Firmware {
> > +    unsafe fn ptr_drop(ptr: *mut Self) {
> > +        // SAFETY:
> > +        // - By the type invariants, we have ownership of the ptr and can free it.
> > +        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
> > +        //   unique pointer to object, it's safe to release the firmware.
> > +        unsafe { bindings::release_firmware(ptr.cast()) };
> >      }
> >  }
> >  
> > -- 
> > 2.43.0
> > 
> 

Re: [PATCH v2 5/5] rust: firmware: implement `Ownable` for Firmware

[Danilo Krummrich]

On Sun, Oct 27, 2024 at 03:20:03PM -0700, Boqun Feng wrote:
> On Wed, Oct 23, 2024 at 11:35:15AM +0200, Danilo Krummrich wrote:
> > On Wed, Oct 23, 2024 at 01:44:49AM +0300, Abdiel Janulgue wrote:
> > > For consistency, wrap the firmware as an `Owned` smart pointer in the
> > > constructor.
> > > 
> > > Cc: Danilo Krummrich <dakr@redhat.com>
> > > Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> > > ---
> > >  rust/kernel/firmware.rs | 31 ++++++++++++++++++-------------
> > >  1 file changed, 18 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > > index dee5b4b18aec..6da834b37455 100644
> > > --- a/rust/kernel/firmware.rs
> > > +++ b/rust/kernel/firmware.rs
> > > @@ -4,8 +4,8 @@
> > >  //!
> > >  //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h)
> > >  
> > > -use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
> > > -use core::ptr::NonNull;
> > > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr,
> > > +            types::{Opaque, Owned, Ownable}};
> > >  
> > >  /// # Invariants
> > >  ///
> > > @@ -52,10 +52,11 @@ fn request_nowarn() -> Self {
> > >  /// # Ok(())
> > >  /// # }
> > >  /// ```
> > > -pub struct Firmware(NonNull<bindings::firmware>);
> > > + #[repr(transparent)]
> > > +pub struct Firmware(Opaque<bindings::firmware>);
> > >  
> > >  impl Firmware {
> > > -    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > > +    fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Owned<Self>> {
> > 
> > I think it's fine to implement this for consistency, but I'm not sure I like
> > that drivers have to refer to it as `Owned<Firmware>`.
> > 
> 
> May I ask why not? ;-)

I think it's because with this an instance of `Firmware` is never a valid thing
to have, which is a bit weird at a first glance.

But I fully agree with the existance of the `Owned` type and the rationale
below.

> 
> Ideally, we should not wrap a pointer to particular type, instead we
> should wrap the type and then combine it with a meaningful pointer type,
> e.g. Box<>, ARef<>, Owned<> ... in this way, we de-couple how the
> lifetime of object is maintained (described by the pointer type) and
> what operations are available on the object (described by the wrapper
> type).
> 
> If later on, a firmware object creation is doable in pure Rust code for
> some condition, we can then have a function that returns a
> `KBox<Firmware>` (assume using kmalloc for the object), and it will just
> work (tm).
> 
> Regards,
> Boqun
> 
> > Anyway, if we keep it this way the patch also needs the following change.
> > 
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index 6da834b37455..1db854eb2422 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -115,8 +115,8 @@ unsafe fn ptr_drop(ptr: *mut Self) {
> > 
> >  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, which is safe to be used from
> >  // any thread.
> > -unsafe impl Send for Firmware {}
> > +unsafe impl Send for Owned<Firmware> {}
> > 
> >  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
> >  // be used from any thread.
> > -unsafe impl Sync for Firmware {}
> > +unsafe impl Sync for Owned<Firmware> {}
> > 
> > >          let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> > >          let pfw: *mut *mut bindings::firmware = &mut fw;
> > >  
> > > @@ -65,25 +66,26 @@ fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
> > >          if ret != 0 {
> > >              return Err(Error::from_errno(ret));
> > >          }
> > > -
> > > +        // CAST: Self` is a `repr(transparent)` wrapper around `bindings::firmware`.
> > > +        let ptr = fw.cast::<Self>();
> > >          // SAFETY: `func` not bailing out with a non-zero error code, guarantees that `fw` is a
> > >          // valid pointer to `bindings::firmware`.
> > > -        Ok(Firmware(unsafe { NonNull::new_unchecked(fw) }))
> > > +        Ok(unsafe { Owned::to_owned(ptr) })
> > >      }
> > >  
> > >      /// Send a firmware request and wait for it. See also `bindings::request_firmware`.
> > > -    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
> > > +    pub fn request(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> > >          Self::request_internal(name, dev, FwFunc::request())
> > >      }
> > >  
> > >      /// Send a request for an optional firmware module. See also
> > >      /// `bindings::firmware_request_nowarn`.
> > > -    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> {
> > > +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Owned<Self>> {
> > >          Self::request_internal(name, dev, FwFunc::request_nowarn())
> > >      }
> > >  
> > >      fn as_raw(&self) -> *mut bindings::firmware {
> > > -        self.0.as_ptr()
> > > +        self.0.get()
> > >      }
> > >  
> > >      /// Returns the size of the requested firmware in bytes.
> > > @@ -101,10 +103,13 @@ pub fn data(&self) -> &[u8] {
> > >      }
> > >  }
> > >  
> > > -impl Drop for Firmware {
> > > -    fn drop(&mut self) {
> > > -        // SAFETY: `self.as_raw()` is valid by the type invariant.
> > > -        unsafe { bindings::release_firmware(self.as_raw()) };
> > > +unsafe impl Ownable for Firmware {
> > > +    unsafe fn ptr_drop(ptr: *mut Self) {
> > > +        // SAFETY:
> > > +        // - By the type invariants, we have ownership of the ptr and can free it.
> > > +        // - Per function safety, this is called in Owned::drop(), so `ptr` is a
> > > +        //   unique pointer to object, it's safe to release the firmware.
> > > +        unsafe { bindings::release_firmware(ptr.cast()) };
> > >      }
> > >  }
> > >  
> > > -- 
> > > 2.43.0
> > > 
> > 
> 

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Abdiel Janulgue]

Hi Alice, Boqun:

On 24/10/2024 10:33, Alice Ryhl wrote:
>>>>>>>
>>>>>>> Please rename this function to from_raw to match the name used by
>>>>>>> other similar functions.
>>>>>>>
>>>>>>> Also, I don't love this wording. We don't really want to guarantee
>>>>>>> that it is unique. For example, pages have one primary owner, but
>>>>>>> there can be others who also have refcounts to the page, so it's not
>>>>>>> really unique. I think you just want to say that `ptr` must point at a
>>>>>
>>>>> But then when `Owned<Page>` dropped, it will call __free_pages() which
>>>>> invalidate any other existing users. Do you assume that the users will
>>>>> use pointers anyway, so it's their unsafe responsiblity to guarantee
>>>>> that they don't use an invalid pointer?
>>>>>
>>>>> Also I assume you mean the others have refcounts to the page *before* an
>>>>> `Owned<Page>` is created, right? Because if we really have a use case
>>>>> where we want to have multiple users of a page after `Owned<Page>`
>>>>> created, we should better provide a `Owned<Page>` to `ARef<Page>`
>>>>> function.
>>>>
>>>> The __free_pages function just decrements a refcount. If there are
>>>> other references to it, it's not actually freed.
>>>>
>>>
>>> Then why don't we use page_put() there? ;-) And instead of
>>> `Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?
>>
>> I don't think there's a function called page_put?
> 
> Sorry I confused myself. It's because it's called put_page.
> 

How do I proceed with this? Should we use the page's reference count to 
decide when to free the allocation and use put_page() instead of 
__free_pages() in Page::Drop?.

In that case, there would be no need for `Ownable`, right? As we could 
just return ARef<Page> in both vmalloc_to_page() case and in 
Page::alloc_page(), letting the kernel handle ownership internally.

Regards,
Abdiel

Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Alice Ryhl]

On Fri, Nov 1, 2024 at 2:38 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> Hi Alice, Boqun:
>
> On 24/10/2024 10:33, Alice Ryhl wrote:
> >>>>>>>
> >>>>>>> Please rename this function to from_raw to match the name used by
> >>>>>>> other similar functions.
> >>>>>>>
> >>>>>>> Also, I don't love this wording. We don't really want to guarantee
> >>>>>>> that it is unique. For example, pages have one primary owner, but
> >>>>>>> there can be others who also have refcounts to the page, so it's not
> >>>>>>> really unique. I think you just want to say that `ptr` must point at a
> >>>>>
> >>>>> But then when `Owned<Page>` dropped, it will call __free_pages() which
> >>>>> invalidate any other existing users. Do you assume that the users will
> >>>>> use pointers anyway, so it's their unsafe responsiblity to guarantee
> >>>>> that they don't use an invalid pointer?
> >>>>>
> >>>>> Also I assume you mean the others have refcounts to the page *before* an
> >>>>> `Owned<Page>` is created, right? Because if we really have a use case
> >>>>> where we want to have multiple users of a page after `Owned<Page>`
> >>>>> created, we should better provide a `Owned<Page>` to `ARef<Page>`
> >>>>> function.
> >>>>
> >>>> The __free_pages function just decrements a refcount. If there are
> >>>> other references to it, it's not actually freed.
> >>>>
> >>>
> >>> Then why don't we use page_put() there? ;-) And instead of
> >>> `Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?
> >>
> >> I don't think there's a function called page_put?
> >
> > Sorry I confused myself. It's because it's called put_page.
> >
>
> How do I proceed with this? Should we use the page's reference count to
> decide when to free the allocation and use put_page() instead of
> __free_pages() in Page::Drop?.
>
> In that case, there would be no need for `Ownable`, right? As we could
> just return ARef<Page> in both vmalloc_to_page() case and in
> Page::alloc_page(), letting the kernel handle ownership internally.

Yes, it seems like we don't need Ownable for Page in the end. You can
use ARef together with put_page() and get_page().

Alice