Re: [PATCH v17 03/10] rust: implement `ForeignOwnable` for `Owned`

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `FOREIGN_ALIGN` is incorrectly set to `align_of::<Owned<T>>()` instead of `align_of::<T>()`, violating the `ForeignOwnable` safety contract and risking pointer corruption when used with `XArray`.
- [Low] Missing `// INVARIANT:` comment when constructing `Owned<T>` in `from_foreign()`.
--

commit 65031e6cc93134c76e2f6b85f3d269f18d531a88
Author: Andreas Hindborg <a.hindborg@kernel.org>
rust: implement ForeignOwnable for Owned

This commit implements the ForeignOwnable trait for Owned<T>. This enables
the use of Owned<T> in collections like XArray.

> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> index 456e239e906ef..5eacdf327d121 100644
> --- a/rust/kernel/owned.rs
> +++ b/rust/kernel/owned.rs
[ ... ]
> @@ -185,3 +188,46 @@ fn drop(&mut self) {
>          unsafe { T::release(self.ptr.as_mut()) };
>      }
>  }
> +
> +// SAFETY: We derive the pointer to `T` from a valid `T`, so the returned
> +// pointer satisfy alignment requirements of `T`.
> +unsafe impl<T: Ownable + 'static> ForeignOwnable for Owned<T> {
> +    const FOREIGN_ALIGN: usize = core::mem::align_of::<Owned<T>>();

[Severity: High]
Does this alignment calculation break the ForeignOwnable contract if T has a
smaller alignment than Owned<T>?

The ForeignOwnable trait guarantees that pointers returned by into_foreign()
will have a minimum alignment of FOREIGN_ALIGN. However, into_foreign()
returns a raw pointer to the underlying T, while FOREIGN_ALIGN is hardcoded
to the alignment of Owned<T> (which evaluates to the alignment of NonNull<T>,
typically 8 bytes).

If T is a type with a smaller alignment requirement (such as 1 or 4 bytes),
into_foreign() might return a pointer that is not 8-byte aligned, yet
FOREIGN_ALIGN will falsely claim it is. XArray relies on this alignment
to use the lower bits for internal tagging. Could this lead to pointer
corruption when inserting an Owned<T> holding an underaligned T into an
XArray?

> +
> +    type Borrowed<'a> = &'a T;
> +    type BorrowedMut<'a> = Pin<&'a mut T>;
> +
> +    #[inline]
> +    fn into_foreign(self) -> *mut kernel::ffi::c_void {
> +        let ptr = self.ptr.as_ptr().cast();
> +        core::mem::forget(self);
> +        ptr
> +    }
> +
> +    #[inline]
> +    unsafe fn from_foreign(ptr: *mut kernel::ffi::c_void) -> Self {
> +        Self {

[Severity: Low]
This isn't a bug, but is this missing an invariant comment for the Owned<T>
struct construction?

According to the Rust subsystem guidelines, when a struct with an
Invariants documentation section is constructed, the code should have an
// INVARIANT: comment explaining why the invariants are satisfied. The
from_foreign() implementation constructs Self { ptr: ... } without this
mandatory comment explaining how reconstructing the struct maintains the
ownership and pinning invariants.

> +            // SAFETY: By function safety contract, `ptr` came from
> +            // `into_foreign` and cannot be null.
> +            ptr: unsafe { NonNull::new_unchecked(ptr.cast()) },
> +        }
> +    }
[ ... ]

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