Re: [PATCH v20 2/8] rust: types: Add Ownable/Owned types

[email protected]
Newsgroups org.freedesktop.lists.dri-devel,org.kernel.vger.linux-pci
Message-ID <[email protected]>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The safety documentation for `Owned::from_raw` fails to require the caller to uphold the pinning invariant.
--

--- Patch [2]: [PATCH v20 2/8] rust: types: Add Ownable/Owned types ---
commit 82ccf756b593f4c76a8a6694855add1d65c27d53
Author: Asahi Lina <[email protected]>
rust: types: Add Ownable/Owned types

This commit introduces Ownable and Owned types for unique ownership of objects
with custom drop logic. It provides a mechanism for C-allocated objects to be 
owned by Rust without generic allocators, delegating resource management to 
the type itself.

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..7fe9ec3e55126
> --- /dev/null
> +++ b/rust/kernel/owned.rs
[ ... ]
> +    /// 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: Medium]
Should the safety documentation for from_raw() explicitly require the caller
to uphold the pinning invariant?

Since as_pin_mut() returns a Pin<&mut T> and the internal comments note that
T is treated as pinned from now on, it seems the caller must guarantee this.

If a caller provides an unpinned allocation, retrieves the pointer later with
into_raw(), and safely moves the object, it could violate the pinning
assumptions established here. Could this lead to memory corruption if the
pinned object is self-referential or registered elsewhere?

> +        // 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.