Re: [PATCH 4/5] rust: drm: add panel producer abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] `Panel::new` steals the devres-owned reference count initialized by `__devm_drm_panel_alloc`, leading to a double-free on device unbind or drop.
- [Critical] The destructor of the driver data `T` is tied to device unbind via devres, rather than the panel's actual reference count, creating a split-lifetime Use-After-Free.
- [High] Missing `Send`, `Sync`, and `'static` bounds on the driver data type `T` in `Panel::new` allows data races and Use-After-Free via non-thread-safe or short-lived types.

Pre-existing issues:
- [Critical] `Panel::from_of_node` improperly converts the result of `of_drm_find_panel` into an `ARef<Panel>` without incrementing the refcount, causing a premature free.
--

--- Patch [4]: [PATCH 4/5] rust: drm: add panel producer abstractions ---
commit 0e243089bb12e5256a62894a39811280cadcb027
Author: Albert Esteve <aesteve@redhat.com>
Subject: rust: drm: add panel producer abstractions

This commit adds abstractions for DRM panel drivers, introducing the
PanelFuncs trait and PanelContainer struct. It provides devres-managed
allocation via __devm_drm_panel_alloc and ties the lifetime of the 
data to the device.

> diff --git a/rust/kernel/drm/panel.rs b/rust/kernel/drm/panel.rs
> index fd21cc2236685..8f87774e06ed7 100644
> --- a/rust/kernel/drm/panel.rs
> +++ b/rust/kernel/drm/panel.rs

[ ... ]

> @@ -139,6 +143,66 @@ pub fn from_of_node(node: &of::Node) -> Result<ARef<Self>> {
>          // `of_drm_find_panel` returns a kref-incremented reference.
>          Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(panel).cast()) })
>      }

[Severity: Critical]
This is a pre-existing issue, but does of_drm_find_panel actually increment
the reference count?

Looking at the C implementation, of_drm_find_panel iterates over the global
panel_list and returns the pointer without calling kref_get or
drm_panel_get. If we consume the pointer directly with ARef::from_raw(),
dropping the ARef will decrement the refcount. Could this drop the refcount
to zero and prematurely free the panel while the provider driver's devres
still owns it?

> +
> +    /// Allocates and initialises a device-managed panel.
> +    ///
> +    /// `data` is embedded in the same allocation as the `drm_panel` and its
> +    /// destructor is called automatically when `dev` is unbound.
> +    ///
> +    /// Use [`Registration::register`] to add the panel to the global registry
> +    /// once it is ready to be used by display drivers.
> +    pub fn new<T: PanelFuncs>(
> +        dev: &Device,
> +        data: T,
> +        connector_type: ConnectorType,
> +    ) -> Result<ARef<Self>> {

[Severity: High]
Does T need Send, Sync, and 'static bounds here?

Since Panel::new takes ownership of T and stores it in a heap allocation
accessed by the concurrent C DRM subsystem, safe Rust code could provide a
non-thread-safe type (like one containing Cell or Rc) or a short-lived
reference. Would this allow data races or use-after-free when the C DRM
core concurrently invokes vtable callbacks?

> +        // SAFETY: `dev` is valid by its type invariants; `PanelFuncsVTable::build()`
> +        // returns a valid, static `drm_panel_funcs` pointer.
> +        let container = error::from_err_ptr(unsafe {
> +            bindings::__devm_drm_panel_alloc(
> +                dev.as_raw(),
> +                core::mem::size_of::<PanelContainer<T>>(),
> +                core::mem::offset_of!(PanelContainer<T>, panel),
> +                PanelFuncsVTable::<T>::build(),
> +                connector_type as i32,
> +            )
> +        })? as *mut PanelContainer<T>;
> +
> +        // SAFETY: `container` is a valid pointer to uninitialized memory.
> +        unsafe {
> +            core::ptr::write(
> +                core::ptr::addr_of_mut!((*container).data),
> +                ManuallyDrop::new(data),
> +            )
> +        };
> +
> +        // SAFETY:
> +        // - `dev.as_raw()` is a pointer to a valid and bound device.
> +        // - `container.cast()` is a valid pointer to the initialized `PanelContainer<T>`.
> +        error::to_result(unsafe {
> +            // `devm_add_action_or_reset` calls `drop_panel_data` on failure, so `data`
> +            // is dropped even if this registration fails.
> +            // Registering after `__devm_drm_panel_alloc` ensures devres LIFO order:
> +            // `drop_panel_data` runs before `kfree(container)`.
> +            bindings::devm_add_action_or_reset(
> +                dev.as_raw(),
> +                Some(drop_panel_data::<T>),
> +                container.cast(),
> +            )

[Severity: Critical]
Is it safe to tie the destruction of T to device unbind rather than the
panel's reference count?

Since the panel is reference-counted and exposed as an ARef<Panel>, consumer
drivers could keep the panel alive after the provider device unbinds. When
unbind occurs, drop_panel_data will destroy T. If a consumer later calls
a panel method like get_modes, the C code will invoke the Rust vtable
callbacks which operate on the dropped T, potentially leading to a
use-after-free.

> +        })?;
> +
> +        // SAFETY: `__devm_drm_panel_alloc` was successful, hence `container` is
> +        // valid and the `drm_panel` at this offset is initialised.
> +        let raw = unsafe {
> +            (container as *mut u8)
> +                .add(core::mem::offset_of!(PanelContainer<T>, panel))
> +                .cast::<bindings::drm_panel>()
> +        };
> +
> +        // SAFETY: `__devm_drm_panel_alloc` was successful, hence `raw` is valid
> +        // and the refcount is non-zero.
> +        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(raw).cast()) })

[Severity: Critical]
Does ARef::from_raw steal the devres-owned reference count here?

__devm_drm_panel_alloc initializes the panel's refcount to 1 and immediately
registers a devres action (drm_panel_put_void) that owns this reference.
Wrapping the raw pointer directly with ARef::from_raw without incrementing
the refcount means both ARef and devres think they own the same refcount.

When the ARef is dropped, won't it free the panel via __drm_panel_free,
causing a double-free later when device unbind triggers the devres action?

> +    }
>  }

[ ... ]

> +/// Operations implemented by a DRM panel driver.
> +///
> +/// Implement this trait to provide a DRM panel driver and its callbacks. Use
> +/// [`Panel::new`] to allocate the panel, passing the driver data as `T`.
> +///
> +/// C header: [`include/drm/drm_panel.h`](srctree/include/drm/drm_panel.h)
> +#[vtable]
> +pub trait PanelFuncs {

[Severity: High]
Should this trait require Send, Sync, and 'static supertraits?

Similar to the missing bounds in Panel::new(), without these supertraits,
types implementing PanelFuncs might contain thread-unsafe primitives
or short-lived references, which could cause data races or use-after-free
when accessed concurrently by the DRM subsystem.

> +    /// Turn on panel and perform set up.

[ ... ]

> +// Devres action: run T's destructor before `kfree(container)`.
> +//
> +// # Safety
> +//
> +// `ptr` must be the base of a live `PanelContainer<T>` whose `data` field was
> +// initialised by `Panel::new` and has not yet been dropped.
> +unsafe extern "C" fn drop_panel_data<T>(ptr: *mut core::ffi::c_void) {
> +    // SAFETY: Caller guarantees `ptr` is the base of a live `PanelContainer<T>`
> +    // with an initialised `data` field. `data` is at offset 0, so `ptr as *mut T`
> +    // is valid.
> +    unsafe { core::ptr::drop_in_place(ptr as *mut T) };

[Severity: Critical]
Does this execute the destructor while the container might still be alive?

If a consumer driver still holds an ARef<Panel>, the container's reference
count is non-zero. Since drop_panel_data is called unconditionally on
device unbind, won't drop_in_place destroy T while it could still be
accessed via the panel's vtable callbacks?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-drm_panel_bindings-v1-0-1f974508a31c@redhat.com?part=4