Re: [PATCH REF v3 24/27] gpu: nova-core: use lifetime for Bar

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Teardown order inversion causes Use-After-Free of PCI BAR during auxiliary device unbind
- [Low] Incorrect safety comment regarding `try_pin_init!` evaluation order contradicts the struct definition
--

commit a3050bb7eec253af2f0de783f6b97a3dcbc7b499
Author: Danilo Krummrich <dakr@kernel.org>

gpu: nova-core: use lifetime for Bar
    
Take advantage of the lifetime-parameterized pci::Bar<'bound> to hold
the BAR mapping directly in NovaCore<'bound>, and pass a borrowed
reference to Gpu<'bound>. This eliminates the Arc<Devres<Bar0>> indirection
and simplifies Gpu::unbind().

> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
> index 3f6d8cedf5abc..fb52d21a7d927 100644
> --- a/drivers/gpu/nova-core/driver.rs
> +++ b/drivers/gpu/nova-core/driver.rs
> @@ -30,9 +27,10 @@
>  static AUXILIARY_ID_COUNTER: Atomic<u32> = Atomic::new(0);
>  
>  #[pin_data]
> -pub(crate) struct NovaCore {
> +pub(crate) struct NovaCore<'bound> {
>      #[pin]
> -    pub(crate) gpu: Gpu,
> +    pub(crate) gpu: Gpu<'bound>,
> +    bar: pci::Bar<'bound, BAR0_SIZE>,
>      #[allow(clippy::type_complexity)]
>      _reg: Devres<auxiliary::Registration<ForLt!(())>>,
>  }

[ ... ]

> @@ -94,14 +94,14 @@ fn probe<'bound>(
>              // other threads of execution.
>              unsafe { pdev.dma_set_mask_and_coherent(DmaMask::new::<GPU_DMA_BITS>())? };
>  
> -            let bar = Arc::new(
> -                pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?
> -                    .into_devres()?,
> -                GFP_KERNEL,
> -            )?;
> -
> -            Ok(try_pin_init!(Self {
> -                gpu <- Gpu::new(pdev, bar.clone(), bar.access(pdev.as_ref())?),
> +            Ok(try_pin_init!(NovaCore {
> +                bar: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?,

Can this teardown order inversion cause a use-after-free of the PCI BAR
during the auxiliary device unbind process?

Since bar is now placed directly in NovaCore while _reg is still managed
by devres via .into_devres()?, driver data (NovaCore) drops before
devres_release_all() is executed. 

This means bar will be dropped and the MMIO region unmapped when NovaCore
drops, but the auxiliary device (nova-drm) remains registered until the devres
subsystem cleans it up later. 

When the auxiliary device eventually unregisters and unbinds, won't it
attempt to access the already unmapped GPU hardware BAR? 

Should _reg also be removed from devres and placed as a direct field in
NovaCore? If so, since Rust drops struct fields in declaration order, _reg
would need to be declared before bar to ensure it is dropped first.

> +                // TODO: Use `&bar` self-referential pin-init syntax once available.
> +                //
> +                // SAFETY: `bar` is initialized before this expression is evaluated
> +                // (`try_pin_init!()` initializes fields in declaration order), lives at a pinned
> +                // stable address, and is dropped after `gpu` (struct field drop order).

This isn't a bug, but is the safety comment regarding the try_pin_init!
evaluation order accurate?

The comment states that try_pin_init! initializes fields in declaration order.
However, in the NovaCore struct definition above, gpu is actually declared
before bar.

If the macro evaluated fields in declaration order, gpu would be
initialized first, and the unsafe block below would read uninitialized memory.
Doesn't try_pin_init! evaluate initializers in the lexical order they are
written within the macro invocation, which is why bar is safely initialized
first here?

> +                gpu <- Gpu::new(pdev, unsafe { &*core::ptr::from_ref(bar) }),
>                  _reg: auxiliary::Registration::new(
>                      pdev.as_ref(),
>                      c"nova-drm",
>                      AUXILIARY_ID_COUNTER.fetch_add(1, Relaxed),
>                  )
>                  .into_devres()?,
>              }))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517000149.3226762-1-dakr@kernel.org?part=24