Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] `OwnableRefCounted` is a safe trait, but its default `into_shared` method contains an `unsafe` block relying on undocumented invariants. - [High] `ForeignOwnable::FOREIGN_ALIGN` for `Owned<T>` incorrectly promises the alignment of `Owned<T>` rather than `T`, leading to under-aligned pointers. - [High] `Ownable::release` takes `&mut self` but is expected to free the underlying memory, violating Rust's Stacked Borrows aliasing rules. - [Low] Incorrect link destination for `ARef<Self>` in doc comments. - [Low] Missing `// INVARIANT:` comment when constructing `Foo` in the doctest. - [Low] Missing `// INVARIANT:` comment when constructing `Owned<T>` in `ForeignOwnable::from_foreign`. - [Low] Unnecessary `mut` keyword in the doc example for `ARef::from(foo)`. --
commit 7e04551249f9cff4815fc7ad79ac13555baea1a7 Author: Oliver Mangold <[email protected]> rust: Add OwnableRefCounted This commit introduces the OwnableRefCounted trait to allow safe conversions between ARef and Owned pointer types. It also updates documentation and adds examples for reference-counted types requiring both shared and unique access. > diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs > index bedd4fef84fa2..9db0daab22256 100644 > --- a/rust/kernel/owned.rs > +++ b/rust/kernel/owned.rs [ ... ] > pub trait Ownable { [ ... ] > unsafe fn release(&mut self); [Severity: High] Does taking &mut self here violate Rust's Stacked Borrows aliasing rules? The Ownable trait defines release as taking &mut self. Owned<T>'s Drop implementation creates a mutable reference to the heap allocation and passes it to T::release. Implementations of release are expected to deallocate the memory backing this reference. Deallocating memory that a mutable reference points to before the lifetime of that reference ends is Undefined Behavior in Rust. Should release take a raw pointer like NonNull<Self> instead of a mutable reference? [ ... ] > impl<T: Ownable> Drop for Owned<T> { > #[inline] > fn drop(&mut self) { > // SAFETY: By existence of `&mut self` we exclusively own `self` and > the underlying `T`. As > // we are dropping `self`, we can transfer ownership of the `T` to > the `release` method. > unsafe { T::release(self.ptr.as_mut()) }; [Severity: High] Is it safe to pass self.ptr.as_mut() here if release frees the memory? This creates the mutable reference that gets passed to release and is subsequently deallocated, triggering the Stacked Borrows violation described above. [ ... ] > unsafe impl<T: Ownable + 'static> ForeignOwnable for Owned<T> { > const FOREIGN_ALIGN: usize = core::mem::align_of::<Owned<T>>(); [Severity: High] Does this incorrectly promise the alignment of Owned<T> rather than T? ForeignOwnable::into_foreign guarantees that the returned pointer has a minimum alignment of FOREIGN_ALIGN. For Owned<T>, into_foreign returns a raw pointer to T. Therefore, the alignment of this pointer is strictly determined by T. If T has an alignment smaller than 8, FOREIGN_ALIGN will falsely promise an 8-byte alignment. Data structures like XArray rely on this statically guaranteed minimum alignment. Passing an under-aligned pointer will cause the pointer to be corrupted by tag data. [ ... ] > #[inline] > unsafe fn from_foreign(ptr: *mut kernel::ffi::c_void) -> Self { > Self { > // SAFETY: By function safety contract, `ptr` came from > // `into_foreign` and cannot be null. > ptr: unsafe { NonNull::new_unchecked(ptr.cast()) }, > } [Severity: Low] Should there be an // INVARIANT: comment here? The struct Owned<T> has an Invariants section. ForeignOwnable::from_foreign constructs Self { ptr: ... } without an // INVARIANT: comment to justify why the ownership and pinning invariants are upheld. [ ... ] > @@ -231,3 +237,125 @@ unsafe fn borrow_mut<'a>(ptr: *mut kernel::ffi::c_void) > -> Self::BorrowedMut<'a> [ ... ] > +struct Foo { > + refcount: Cell<usize>, > +} > + > +impl Foo { > + fn new() -> Result<Owned<Self>> { > + // We are just using a `KBox` here to handle the actual allocation, > as our `Foo` is > + // not actually a C-allocated object. > + let result = KBox::new( > + Foo { > + refcount: Cell::new(1), > + }, > + flags::GFP_KERNEL, > + )?; [Severity: Low] Should we add an // INVARIANT: comment here? The struct Foo is documented with invariants above, but this construction lacks the comment explaining why the invariants are satisfied. > + let result = KBox::into_non_null(result); > + // SAFETY: > + // - We just allocated the `Self`, thus it is valid and we own it. > + // - We can transfer this ownership to the `from_raw` method. > + Ok(unsafe { Owned::from_raw(result) }) > + } > +} [ ... ] > +let foo = Foo::new()?; > +let mut foo = ARef::from(foo); [Severity: Low] Can we remove the mut keyword here? The variable foo is only cloned and consumed by Owned::try_from, but never mutated. This will trigger an unused mutability warning which can break the build for documentation tests when warnings are treated as errors. > +{ > + let bar = foo.clone(); > + assert!(Owned::try_from(bar).is_err()); > +} > +assert!(Owned::try_from(foo).is_ok()); > +# Ok::<(), Error>(()) > +``` > +pub trait OwnableRefCounted: RefCounted + Ownable + Sized { > + /// Checks if the [`ARef`] is unique and converts it to an [`Owned`] if > that is the case. > + /// Otherwise it returns again an [`ARef`] to the same underlying object. > + fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>>; > + > + /// Converts the [`Owned`] into an [`ARef`]. > + #[inline] > + fn into_shared(this: Owned<Self>) -> ARef<Self> { > + // SAFETY: Safe by the requirements on implementing the trait. > + unsafe { ARef::from_raw(Owned::into_raw(this)) } [Severity: High] Does into_shared rely on undocumented invariants that make this unsafe block unsound? Since OwnableRefCounted is a safe trait, any safe code can implement it. The default implementation of into_shared calls ARef::from_raw, which assumes the raw pointer has an active reference count increment that can be taken ownership of. Because there are no safety requirements on the trait preventing it, a user could safely implement OwnableRefCounted for a type where Owned<T> does not hold a reference count increment. Calling into_shared on such a type would pass a pointer without an active refcount to ARef::from_raw, and when dropped, it would decrement a non-existent refcount. Should this trait be declared as unsafe trait OwnableRefCounted to prevent undefined behavior? > + } > +} > diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs > index 818c84fa923a3..68d31f43e6743 100644 > --- a/rust/kernel/sync/aref.rs > +++ b/rust/kernel/sync/aref.rs [ ... ] > @@ -35,7 +39,10 @@ > /// Note: Implementing this trait allows types to be wrapped in an > [`ARef<Self>`]. It requires an > /// internal reference count and provides only shared references. If unique > references are required > /// [`Ownable`](crate::types::Ownable) should be implemented which allows > types to be wrapped in an > -/// [`Owned<Self>`](crate::types::Owned). > +/// [`Owned<Self>`](crate::types::Owned). Implementing the trait > +/// [`OwnableRefCounted`] allows to convert between unique and > +/// shared references (i.e. [`Owned<Self>`](crate::types::Owned) and > +/// [`ARef<Self>`](crate::types::Owned)). [Severity: Low] Is this doc link destination correct for ARef<Self>? The link target incorrectly points to Owned instead of ARef. It should point to the correct type, e.g., crate::sync::aref::ARef. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
