Re: [PATCH 5/5] rust: drm: add KUnit tests for panel

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Missing Kconfig dependency on DRM for RUST_DRM_PANEL_KUNIT_TEST
- [Low] Horizontal import formatting violates subsystem guidelines.

Pre-existing issues:
- [Critical] `Panel::from_of_node` creates an `ARef<Panel>` without incrementing the reference count, leading to premature free and use-after-free.
- [Critical] `Panel::new` improperly steals the devres-managed reference count, leading to use-after-free and double-free.
--

--- Patch [5]: [PATCH 5/5] rust: drm: add KUnit tests for panel ---
commit 075cc8b6578147fc552359e30a2b237e1bdb801a
Author: Albert Esteve <aesteve@redhat.com>

rust: drm: add KUnit tests for panel

Add a test module to verify basic data type conversions and layout for
the Rust DRM panel API.

> diff --git a/rust/kernel/Kconfig.test b/rust/kernel/Kconfig.test
> index e6a5c7a795f0f..397b916dca0d4 100644
> --- a/rust/kernel/Kconfig.test
> +++ b/rust/kernel/Kconfig.test
> @@ -83,4 +83,14 @@ config RUST_BITFIELD_KUNIT_TEST
>  
>  	  If unsure, say N.
>  
> +config RUST_DRM_PANEL_KUNIT_TEST
> +	bool "KUnit tests for Rust panel API" if !KUNIT_ALL_TESTS

[Severity: Low]
Will this Kconfig option be available even when CONFIG_DRM is disabled?

Since the tests reside in rust/kernel/drm/panel.rs which is compiled
conditionally under CONFIG_DRM, could this result in a dead configuration
option where Kconfig allows enabling the test without DRM, causing the
code to be silently skipped during compilation?

> +	default KUNIT_ALL_TESTS
> +	help
> +	  This option enables KUnit tests for the Rust panel API.
> +	  These are only for development and testing, not for regular
> +	  kernel use cases.
> +
> +	  If unsure, say N.
> +
>  endif
> diff --git a/rust/kernel/drm/panel.rs b/rust/kernel/drm/panel.rs
> index 8f87774e06ed7..3da203c39edd6 100644
> --- a/rust/kernel/drm/panel.rs
> +++ b/rust/kernel/drm/panel.rs
> @@ -595,3 +595,90 @@ pub(crate) const fn build() -> &'static bindings::drm_panel_funcs {
>          &Self::VTABLE
>      }
>  }
> +
> +#[cfg(CONFIG_RUST_DRM_PANEL_KUNIT_TEST)]
> +#[macros::kunit_tests(rust_kernel_drm_panel)]
> +mod tests {
> +    use super::*;
> +    use core::mem::{align_of, offset_of, size_of, ManuallyDrop, MaybeUninit};

[Severity: Low]
Does this horizontal import list violate the Rust subsystem guidelines?

The guidelines mandate following the kernel vertical import style when
commits touch imports, so shouldn't each import be placed on its own line?

[ ... ]

While reviewing this file, I noticed a couple of pre-existing issues in
rust/kernel/drm/panel.rs:

rust/kernel/drm/panel.rs:from_of_node() {
    ...
    // SAFETY: `from_err_ptr` guarantees a non-null pointer on success.
    // `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 I noticed that of_drm_find_panel() returns
a pointer without calling drm_panel_get() to increment its reference count.

Since ARef::from_raw() takes ownership of a reference count increment that
doesn't actually exist, will dropping the returned ARef<Panel> decrement the
panel's kref via drm_panel_put(), effectively stealing a reference and
causing the panel to be prematurely freed?


rust/kernel/drm/panel.rs:new() {
    ...
    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]
This isn't a bug introduced by this patch, but does Panel::new steal the
devres-managed reference count here?

__devm_drm_panel_alloc() allocates the panel, initializes the kref to 1,
and registers a devres action that owns this initial kref. By wrapping the
returned pointer in ARef::from_raw(), we assume ownership of an existing
reference count without incrementing it.

If the ARef is dropped, the kref reaches 0 and the panel is freed. Later,
when the device unbinds, won't devres execute its action on the freed
memory, causing a use-after-free or double-free?

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