Re: [PATCH v17 05/10] rust: page: convert to `Ownable`

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Latent Memory Corruption / Pointer Tagging via incorrect `FOREIGN_ALIGN` on `Owned<T>`
- [Medium] Documentation and Type Invariant Violation in `Page`
- [Medium] Missing Safety Contract for `Ownable` implementation on `Page`
- [Medium] API Regression: `BorrowedPage` implicitly loses `Send` and `Sync` traits
- [Low] Unnecessary unsafe block and verbose pointer cast
- [Low] Incomplete Refactoring: Redundant `ManuallyDrop` wrapper
--

commit 3edfb37d39b0c201b33aed199fe66b3c430e3c6b
Author: Asahi Lina <lina+kernel@asahilina.net>
Subject: rust: page: convert to Ownable

This patch converts the Page reference implementation to use the Ownable
trait. This allows Page references to be returned as borrowed references
without necessarily owning the struct page.

> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> index 3bdcee0e16a8d..844c75e541345 100644
> --- a/rust/kernel/page.rs
> +++ b/rust/kernel/page.rs
[ ... ]
> @@ -105,7 +110,7 @@ pub const fn page_align(addr: usize) -> Option<usize> {
>  ///
>  /// [`VBox`]: kernel::alloc::VBox
>  /// [`Vmalloc`]: kernel::alloc::allocator::Vmalloc
> -pub struct BorrowedPage<'a>(ManuallyDrop<Page>, PhantomData<&'a Page>);
> +pub struct BorrowedPage<'a>(ManuallyDrop<NonNull<Page>>, PhantomData<&'a Page>);

[Severity: Medium]
Does this change implicitly remove the Send and Sync auto-traits from
BorrowedPage?

Since NonNull is not Send or Sync, it seems BorrowedPage loses its
thread-safety, which might break concurrency APIs relying on borrowed
pages being thread-safe.

[Severity: Low]
Is the ManuallyDrop wrapper still necessary here?

Since NonNull implements Copy and has no drop glue, wrapping it in
ManuallyDrop appears to be functionally redundant now that it no longer
contains a Page struct that requires drop suppression.

>  impl<'a> BorrowedPage<'a> {
>      /// Constructs a [`BorrowedPage`] from a raw pointer to a `struct page`.
> @@ -115,7 +120,9 @@ impl<'a> BorrowedPage<'a> {
>      /// - `ptr` must point to a valid `bindings::page`.
>      /// - `ptr` must remain valid for the entire lifetime `'a`.
>      pub unsafe fn from_raw(ptr: NonNull<bindings::page>) -> Self {
> -        let page = Page { page: ptr };
> +        let page: NonNull<Page> =
> +            // SAFETY: By function safety requirements `ptr` is non null.
> +            unsafe { NonNull::new_unchecked(ptr.as_ptr().cast()) };

[Severity: Low]
Can this verbose pointer cast be natively simplified?

It seems this could be safely replaced with the stable ptr.cast::<Page>()
method, avoiding the need for an unsafe block and safety comment altogether.

[ ... ]
> @@ -148,8 +156,9 @@ pub trait AsPageIter {
>  /// # Invariants
>  ///
>  /// The pointer is valid, and has ownership over the page.
> +#[repr(transparent)]
>  pub struct Page {
> -    page: NonNull<bindings::page>,
> +    page: Opaque<bindings::page>,
>  }

[Severity: Medium]
Does the documentation for Page need to be updated to match the new
semantics?

The doc and invariants still state it is "A pointer to a page that owns
the page allocation," but since it is now a transparent wrapper around
Opaque, it is no longer a pointer and no longer holds ownership directly.

[ ... ]
> @@ -370,10 +380,12 @@ pub unsafe fn copy_from_user_slice_raw(
>      }
>  }
>  
> -impl Drop for Page {
> +impl Ownable for Page {
>      #[inline]
> -    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) };
> +    unsafe fn release(&mut self) {
> +        let ptr: *mut Self = self;
> +        // SAFETY: By the function safety requirements, we have ownership of the page and can free
> +        // it. Since Page is transparent, we can cast the raw pointer directly.
> +        unsafe { bindings::__free_pages(ptr.cast(), 0) };
>      }
>  }

[Severity: Medium]
Does this missing safety contract allow silently freeing only part of a
higher-order page?

The release implementation hardcodes an order of 0. If a user constructs
an Owned<Page> from a higher-order allocation, dropping it would silently leak
the rest or corrupt the buddy allocator.

Should this order-0 restriction be explicitly documented in the safety
requirements of Page or its Ownable implementation?

Additionally, while reviewing the underlying ForeignOwnable implementation
for Owned<T> in rust/kernel/owned.rs, I noticed the following:

[Severity: High]
Does the definition of FOREIGN_ALIGN in the ForeignOwnable implementation
for Owned<T> provide the correct alignment guarantee?

rust/kernel/owned.rs:ForeignOwnable for Owned<T> {
    ...
    const FOREIGN_ALIGN: usize = core::mem::align_of::<Owned<T>>();
    ...
}

Since into_foreign returns a pointer to the inner T, if T has an alignment
smaller than 8 bytes, the returned pointer is not guaranteed to be 8-byte
aligned.

If another subsystem (like Workqueue or RBTree) stashes tag bits in the
lower bits of the pointer based on FOREIGN_ALIGN, wouldn't this mask off
valid address bits and cause memory corruption when the pointer is
reconstructed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-unique-ref-v17-0-7b4c3d2930b9@kernel.org?part=5