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 <[email protected]> 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/[email protected]?part=3
