Re: [PATCH REF v4 25/27] gpu: nova-core: unregister sysmem flush page from Drop

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing MMIO readback after unregistering the sysmem flush page allows DMA to freed memory.
- [High] Incorrect teardown order causes child DRM drivers to be unregistered after GPU resources are freed.
- [High] GPU hardware is not halted before its DMA memory is freed during teardown.
--

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

gpu: nova-core: unregister sysmem flush page from Drop

Now that SysmemFlush can borrow the Bar via HRT lifetime, store a
&'bound Bar0 reference and implement Drop to automatically unregister
the sysmem flush page. This removes the need for manual unregister()
calls and the Gpu::unbind() method.

Link: https://lore.kernel.org/all/20260409-fix-systemflush-v1-1-a1d6c968f17c@nvidia.com/

> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
> index d4cf4379ee87c..cff5034c2dcd2 100644
> --- a/drivers/gpu/nova-core/driver.rs
> +++ b/drivers/gpu/nova-core/driver.rs
> @@ -113,8 +113,4 @@ fn probe<'bound>(
>              }))
>          })
>      }
> -
> -    fn unbind<'bound>(_pdev: &'bound pci::Device<Core<'_>>, this: Pin<&Self::Data<'bound>>) {
> -        this.gpu.unbind();
> -    }

[Severity: High]
This isn't a bug introduced by this patch, but by relying on Drop for
teardown, does the struct field declaration order cause child DRM drivers
to be unregistered after GPU resources are freed?

In Rust, struct fields are dropped in declaration order. If gpu and bar
are declared before _reg (the auxiliary device registration) in NovaCore,
dropping NovaCore will drop gpu first (freeing GSP DMA buffers and
unmapping the sysmem flush page).

Later, _reg is dropped, which invokes auxiliary_device_delete and
triggers the child DRM driver's remove callback. This means the child
driver is unbound and executes its teardown after the GPU hardware state
has been torn down and its backing DMA memory freed.

Could this lead to crashes or use-after-free?

[Severity: High]
This is a pre-existing issue, but is the GPU hardware halted before its
DMA memory is freed during teardown?

When the Gpu struct is dropped, the DMA memory for the sysmem flush page
and GSP is freed via their respective Drop implementations. However,
neither Gpu, Gsp, nor Falcon appear to implement a Drop routine that
actually halts the GSP firmware or resets the falcons.

Because the hardware is left running while its DMA backing memory is
returned to the system allocator, could the GPU continue to execute and
access the freed memory?

>  }
> diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
> index bdd5eed760e17..64fe5f27f41ee 100644
> --- a/drivers/gpu/nova-core/fb.rs
> +++ b/drivers/gpu/nova-core/fb.rs

[ ... ]

> @@ -70,19 +69,18 @@ pub(crate) fn register(
>          Ok(Self {
>              chipset,
>              device: dev.into(),
> +            bar,
>              page,
>          })
>      }
> +}
>  
> -    /// Unregister the managed sysmem flush page.
> -    ///
> -    /// In order to gracefully tear down the GPU, users must make sure to call this method before
> -    /// dropping the object.
> -    pub(crate) fn unregister(&self, bar: &Bar0) {
> +impl Drop for SysmemFlush<'_> {
> +    fn drop(&mut self) {
>          let hal = hal::fb_hal(self.chipset);
>  
> -        if hal.read_sysmem_flush_page(bar) == self.page.dma_handle() {
> -            let _ = hal.write_sysmem_flush_page(bar, 0).inspect_err(|e| {
> +        if hal.read_sysmem_flush_page(self.bar) == self.page.dma_handle() {
> +            let _ = hal.write_sysmem_flush_page(self.bar, 0).inspect_err(|e| {

[Severity: High]
This is a pre-existing issue, but does this MMIO write need a readback?

When SysmemFlush::drop unregisters the flush page, it issues a PCIe MMIO
write. PCIe writes are posted and can be buffered. Because there is no
subsequent MMIO read to flush the posted write, the CPU can immediately
proceed to drop self.page and free the DMA memory while the hardware's
register update is still in flight.

If the GPU executes a sysmem flush before the register actually updates,
could it write to freed memory?

>                  dev_warn!(
>                      &self.device,
>                      "failed to unregister sysmem flush page: {:?}\n",

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