Re: [PATCH v3 1/3] gpu: nova-core: build the debugfs guard before registering the driver

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `NovaCore` struct fields are dropped in the wrong order, causing the auxiliary device to be unregistered after the GPU and BAR are destroyed.
--

commit 10c5ae1aadef16818d89098db4af7d31890aea8c
Author: Vladislav Zaharov <vladazaharova2018@gmail.com>

gpu: nova-core: build the debugfs guard before registering the driver

This commit reorders the initialization in try_pin_init! so that the debugfs
guard is built before the driver registration. This ensures that if
initialization fails, the debugfs directory is properly cleaned up instead of
persisting across module reloads. The drop order for the module remains
unchanged.

> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index 1133c6ce5c554..11fe1d2858a9c 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -47,7 +47,8 @@ fn drop(&mut self) {
>  #[pin_data]
>  struct NovaCoreModule {
>      // Fields are dropped in declaration order, so `_driver` is dropped first,
> -    // then `_debugfs_guard` clears `DEBUGFS_ROOT`.
> +    // then `_debugfs_guard` clears `DEBUGFS_ROOT`. They are initialized the
> +    // other way round, see `init()`.
>      #[pin]
>      _driver: Registration<pci::Adapter<driver::NovaCoreDriver>>,
>      _debugfs_guard: DebugfsRootGuard,

[Severity: High]
This isn't a bug introduced by this patch, but since this change discusses
drop orders, does the struct declaration order of NovaCore in
drivers/gpu/nova-core/driver.rs cause the auxiliary device to be unregistered
after the GPU and BAR are destroyed?

drivers/gpu/nova-core/driver.rs:
#[pin_data]
pub(crate) struct NovaCore<'bound> {
    #[pin]
    pub(crate) gpu: Gpu<'bound>,
    bar: pci::Bar<'bound, BAR0_SIZE>,
    #[allow(clippy::type_complexity)]
    _reg: auxiliary::Registration<'bound, CovariantForLt!(())>,
}

In Rust, struct fields are dropped in declaration order (first to last). In
NovaCore, gpu and bar are declared before _reg. Therefore, the GPU is dropped
and the MMIO region is unmapped before the auxiliary device registration
(_reg) is dropped.

When _reg is subsequently dropped, it synchronously unregisters the child
device and invokes the child driver's (e.g., nova-drm) remove callback. If
the child driver attempts to access the GPU or MMIO BAR during its teardown
(e.g., to disable the display or flush buffers), will it access destroyed
resources or unmapped memory, leading to a UAF and kernel oops?

This deterministic crash would trigger whenever the NovaCore device is
unbound or the module is unloaded.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912071842.622696-1-vladazaharova2018@gmail.com?part=1