linux-kernel - Re: [PATCH 2/7] rust: v4l2: add support for v4l2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DC5FT0XAFW59.VN0EKI1LYNKW@kernel.org>
Date: Mon, 18 Aug 2025 11:14:02 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Alexandre Courbot"
 <acourbot@...dia.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <kernel@...labora.com>,
 <linux-media@...r.kernel.org>
Subject: Re: [PATCH 2/7] rust: v4l2: add support for v4l2_device

On Mon Aug 18, 2025 at 7:49 AM CEST, Daniel Almeida wrote:
> +/// A logical V4L2 device handle.
> +///
> +/// # Invariants
> +///
> +/// - `inner` points to a valid `v4l2_device` that has been registered.
> +#[pin_data]
> +#[repr(C)]
> +pub struct Device<T: Driver> {
> +    #[pin]
> +    inner: Opaque<bindings::v4l2_device>,
> +    #[pin]
> +    data: T::Data,
> +}
> +
> +impl<T: Driver> Device<T> {
> +    /// Converts a raw pointer to a `Device<T>` reference.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `ptr` must be a valid pointer to a `struct v4l2_device` that must
> +    ///   remain valid for the lifetime 'a.

"valid pointer to a `struct v4l2_device`" is not sufficient for casting it to
Device<T>.

> +    #[expect(dead_code)]
> +    pub(super) unsafe fn from_raw<'a>(ptr: *mut bindings::v4l2_device) -> &'a Device<T> {
> +        // SAFETY: `ptr` is a valid pointer to a `struct v4l2_device` as per the
> +        // safety requirements of this function.
> +        unsafe { &*(ptr.cast::<Device<T>>()) }
> +    }

<snip>

> +/// Represents the registration of a [`Device`].
> +///
> +/// # Invariants
> +///
> +/// - The underlying device was registered via [`bindings::v4l2_device_register`].
> +pub struct Registration<T: Driver>(ARef<Device<T>>);
> +
> +impl<T: Driver> Registration<T> {
> +    /// Creates and registers a [`Device`] given a [`kernel::device::Device`] reference and
> +    /// the associated data.
> +    pub fn new(
> +        dev: &device::Device,
> +        data: impl PinInit<T::Data, Error>,
> +        flags: alloc::Flags,
> +    ) -> Result<Self> {

Why does Registration::new() create the Device<T> instance?

I think Device<T> should have its own constructor. It is confusing that the
Device<T> is silently created by the Registration and to get a reference one has
to call `reg.device().into()` afterwards.

> +        let v4l2_dev = try_pin_init!(Device {
> +            inner <- Opaque::try_ffi_init(move |slot: *mut bindings::v4l2_device| {
> +                let v4l2_dev = bindings::v4l2_device {
> +                    release: Some(Device::<T>::release_callback),
> +                    // SAFETY: All zeros is valid for this C type.
> +                    ..unsafe { MaybeUninit::zeroed().assume_init() }
> +                };
> +
> +                // SAFETY: The initializer can write to the slot.
> +                unsafe { slot.write(v4l2_dev) };
> +
> +                // SAFETY: It is OK to call this function on a zeroed
> +                // v4l2_device and a valid `device::Device` reference.
> +                to_result(unsafe { bindings::v4l2_device_register(dev.as_raw(), slot) })
> +            }),
> +            data <- data,
> +        });
> +
> +        let v4l2_dev = KBox::pin_init(v4l2_dev, flags)?;
> +
> +        // SAFETY: We will be passing the ownership of the increment to ARef<T>,
> +        // which treats the underlying memory as pinned throughout its lifetime.
> +        //
> +        // This is true because:
> +        //
> +        // - ARef<T> does not expose a &mut T, so there is no way to move the T
> +        // (e.g.: via a `core::mem::swap` or similar).
> +        // - ARef<T>'s member functions do not move the T either.
> +        let ptr = KBox::into_raw(unsafe { Pin::into_inner_unchecked(v4l2_dev) });
> +
> +        // SAFETY:
> +        //
> +        // - the refcount is one, and we are transfering the ownership of that
> +        // increment to the ARef.
> +        // - `ptr` is non-null as it came from `KBox::into_raw`, so it is safe
> +        // to call `NonNulll::new_unchecked`.
> +        Ok(Self(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) }))
> +    }
> +
> +    /// Returns a reference to the underlying [`Device`].
> +    pub fn device(&self) -> &Device<T> {
> +        &self.0
> +    }
> +}
> +
> +impl<T: Driver> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as per the invariants of [`Registration`].
> +        unsafe { bindings::v4l2_device_unregister(self.0.as_raw()) }
> +    }
> +}