Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] `Ownable::release` takes `&mut self` instead of a raw pointer, causing Undefined Behavior due to invalidating a live reference during deallocation and narrowing pointer provenance. - [High] `Owned<T>` inherently pins its contents, but the safety contracts for `Ownable` and `from_raw` fail to enforce or document this Pin guarantee. --
commit 14ba35325a0c0c8b1032beb4bbdfd6396842dfdc Author: Asahi Lina <[email protected]> rust: types: Add Ownable/Owned types By analogy to AlwaysRefCounted and ARef, an Ownable type is a (typically C FFI) type that may be owned by Rust, but need not be. Unlike AlwaysRefCounted, this mechanism expects the reference to be unique within Rust, and does not allow cloning. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs > new file mode 100644 > index 0000000000000..456e239e906ef > --- /dev/null > +++ b/rust/kernel/owned.rs > @@ -0,0 +1,187 @@ > +// SPDX-License-Identifier: GPL-2.0 [ ... ] > +pub trait Ownable { > + /// Tear down this `Ownable`. > + /// > + /// Implementers of `Ownable` can use this function to clean up the use > of `Self`. This can > + /// include freeing the underlying object. > + /// > + /// # Safety > + /// > + /// Callers must ensure that the caller has exclusive ownership of `T`, > and this ownership can > + /// be transferred to the `release` method. > + unsafe fn release(&mut self); [Severity: High] Does taking a mutable reference here instead of a raw pointer risk undefined behavior? If Owned::drop() creates a temporary reference and passes it to release(), deallocating the backing memory of this live mutable reference violates Rust's aliasing rules because the reference remains in scope during the free: Owned::drop() T::release(self.ptr.as_mut()) Additionally, if T is an opaque C-allocated type (often represented as a Zero-Sized Type), creating a mutable reference narrows the pointer's provenance to exactly 0 bytes. Converting this back to a raw pointer and passing it to a C deallocator would be invalid. Would it be safer for release() to take a raw pointer (like NonNull<Self> or *mut Self), similar to AlwaysRefCounted::dec_ref()? [Severity: High] Since Owned<T> guarantees that its underlying T is pinned and exposes Pin<&mut T> via as_pin_mut(), does Ownable need to be an unsafe trait to enforce pinning invariants? Currently, Ownable is safe to implement, and release() provides an unpinned mutable reference. A safe implementation of release() could potentially move the value. When Owned::drop() is called, this would move a value that was previously pinned, violating the pinning guarantees. > +} [ ... ] > +impl<T: Ownable> Owned<T> { > + /// Creates a new instance of [`Owned`]. > + /// > + /// This function takes over ownership of the underlying object. > + /// > + /// # Safety > + /// > + /// Callers must ensure that: > + /// - `ptr` points to a valid instance of `T`. > + /// - Until `T::release` is called, the returned `Owned<T>` exclusively > owns the underlying `T`. > + #[inline] > + pub unsafe fn from_raw(ptr: NonNull<T>) -> Self { [Severity: High] Does the safety documentation for from_raw() need to require that the allocation is permanently pinned? Without this requirement, a caller could pass an unpinned allocation, extract it later via into_raw(), and move it. This would violate the internal pinning assumptions made by Owned<T> when calling as_pin_mut(). > + // INVARIANT: By function safety requirement we satisfy the first > invariant of `Self`. > + // We treat `T` as pinned from now on. > + Self { ptr } > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
