Re: [PATCH 13/16] gpu: nova-core: mm: Add multi-page mapping API to VMM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Capacity exhaustion during page table installation causes hardware Use-After-Free.
- [Critical] Freeing the VFN range on partial mapping/unmapping failure leaves dangling hardware PTEs.
- [High] Early error return in `invalidate_ptes` skips mandatory TLB flush.
- [Medium] `PreparedMapping` and `MappedRange` permanently leak VFN space on drop.
- [Low] Spurious false-positive kernel warning on `execute_map` validation failure.
--

commit dc299c1a2ad37eca6a9c91c4b5425f4e7871fa45
Author: Joel Fernandes <joelagnelf@nvidia.com>

gpu: nova-core: mm: Add multi-page mapping API to VMM

Add the page table mapping and unmapping API to the Virtual Memory
Manager, implementing a two-phase prepare/execute model suitable for
use both inside and outside the DMA fence signalling critical path.

> diff --git a/drivers/gpu/nova-core/mm/pagetable/map.rs b/drivers/gpu/nova-core/mm/pagetable/map.rs
> --- a/drivers/gpu/nova-core/mm/pagetable/map.rs
> +++ b/drivers/gpu/nova-core/mm/pagetable/map.rs
[ ... ]
> +    pub(super) fn prepare_map(
> +        &self,
> +        mm: &mut GpuMm<'_>,
> +        vfn_start: Vfn,
> +        num_pages: usize,
> +        page_table_allocs: &mut KVec<Pin<KBox<AllocatedBlocks>>>,
> +        pt_pages: &mut RBTree<VramAddress, PreparedPtPage>,
> +    ) -> Result {
> +        // Pre-reserve so install_mappings() can use push_within_capacity (no alloc
> +        // in fence signalling critical path).
> +        let pt_upper_bound = M::pt_pages_upper_bound(num_pages);
> +        page_table_allocs.reserve(pt_upper_bound, GFP_KERNEL)?;

[Severity: Critical]
Does this reservation fail to account for multiple pending allocations?
If prepare_map() is called multiple times before execute_map() or fails
partway through, orphaned pages accumulate in the shared pt_pages tree.
This regression occurs because page_table_allocs.reserve() does not
accumulate capacity across multiple calls, causing install_mappings()
to later run out of reserved capacity.

[ ... ]
> +    pub(super) fn install_mappings(
> +        &self,
> +        mm: &mut GpuMm<'_>,
> +        pt_pages: &mut RBTree<VramAddress, PreparedPtPage>,
> +        page_table_allocs: &mut KVec<Pin<KBox<AllocatedBlocks>>>,
> +        vfn_start: Vfn,
> +        pfns: &[Pfn],
> +        writable: bool,
> +    ) -> Result {
> +        {
> +            let pramin = mm.pramin_mut();
> +
> +            // Drain prepared PT pages, install all pending PDEs.
> +            let mut cursor = pt_pages.cursor_front_mut();
> +            while let Some(c) = cursor {
> +                let (next, node) = c.remove_current();
> +                let (install_addr, page) = node.to_key_value();
> +                let page_vram =
> +                    VramAddress::from_raw(page.alloc.iter().next().ok_or(ENOMEM)?.offset());
> +
> +                if page.level == M::DUAL_PDE_LEVEL {
> +                    let new_dpde = M::DualPde::new_small(Pfn::from(page_vram));
> +                    new_dpde.write(pramin, install_addr)?;
> +                } else {
> +                    let new_pde = M::Pde::new(AperturePde::VideoMemory, Pfn::from(page_vram));
> +                    new_pde.write(pramin, install_addr)?;
> +                }
> +
> +                page_table_allocs
> +                    .push_within_capacity(page.alloc)
> +                    .map_err(|_| ENOMEM)?;

[Severity: Critical]
Can this capacity check failure lead to a use-after-free in the hardware?
The physical memory is committed to the hardware PDE right before the
capacity check fails. If push_within_capacity() returns an error, this
regression causes the allocation to be dropped and the memory to be freed
back to the buddy allocator, but the hardware PDE is left pointing to that
freed memory.

[ ... ]
> +    pub(super) fn invalidate_ptes(
> +        &self,
> +        mm: &mut GpuMm<'_>,
> +        vfn_start: Vfn,
> +        num_pages: usize,
> +    ) -> Result {
> +        let invalid_pte = M::Pte::invalid();
> +
> +        {
> +            let pramin = mm.pramin_mut();
> +            for i in 0..num_pages {
> +                let i_u64: u64 = i.into_safe_cast();
> +                let vfn = Vfn::new(vfn_start.raw() + i_u64);
> +                let result = self.walker.walk_to_pte_lookup_with_window(pramin, vfn)?;
> +
> +                match result {
> +                    WalkResult::Mapped { pte_addr, .. } | WalkResult::Unmapped { pte_addr } => {
> +                        invalid_pte.write(pramin, pte_addr)?;
> +                    }

[Severity: High]
Does the ? operator here skip the mandatory TLB flush? If a hardware I/O
error occurs during the page table window write, the function returns
early.

> +                    WalkResult::PageTableMissing => {
> +                        continue;
> +                    }
> +                }
> +            }
> +        }
> +
> +        mm.tlb().flush(self.pdb_addr)
> +    }

[Severity: High]
This required TLB flush is bypassed on the early error return mentioned
above. Any PTEs successfully invalidated before the error will remain
active in the GPU's TLB, creating a regression where continued access to
memory is allowed after it has been unmapped and freed.

> diff --git a/drivers/gpu/nova-core/mm/vmm.rs b/drivers/gpu/nova-core/mm/vmm.rs
> --- a/drivers/gpu/nova-core/mm/vmm.rs
> +++ b/drivers/gpu/nova-core/mm/vmm.rs
[ ... ]
> +impl Drop for MustExecuteGuard {
> +    fn drop(&mut self) {
> +        if self.armed.get() {
> +            kernel::pr_warn!("PreparedMapping dropped without calling execute_map()\n");
> +        }
> +    }
> +}

[Severity: Medium]
Does this drop guard permanently leak the allocated VFN space? While the
guard logs a warning, it lacks a reference to the Vmm and cannot free the
underlying virtual address allocation from the maple tree, causing a
regression that permanently leaks virtual address space.

[ ... ]
> +impl Drop for MustUnmapGuard {
> +    fn drop(&mut self) {
> +        if self.armed.get() {
> +            kernel::pr_warn!("MappedRange dropped without calling unmap_pages()\n");
> +        }
> +    }
> +}

[Severity: Medium]
Similarly, does dropping a MappedRange without calling unmap_pages() leak
the virtual address space? If a caller propagates an error and drops the
range, the warning is printed but the VFN space is permanently lost.

[ ... ]
> +    pub(crate) fn execute_map(
> +        &mut self,
> +        mm: &mut GpuMm<'_>,
> +        prepared: PreparedMapping,
> +        pfns: &[Pfn],
> +        writable: bool,
> +    ) -> Result<MappedRange> {
> +        if pfns.len() != prepared.num_pages {
> +            self.free_vfn(prepared.vfn_start);
> +            return Err(EINVAL);
> +        }

[Severity: Low]
Could this validation failure trigger a false-positive kernel warning?
The prepared mapping is passed by value and is dropped on this early
return. Because its drop guard is still armed, MustExecuteGuard::drop()
will log a warning that the mapping was dropped without calling
execute_map(), even though the caller did correctly invoke execute_map().

[ ... ]
> +        if let Err(e) = self.pt_map.install_mappings(
> +            mm,
> +            &mut self.pt_pages,
> +            &mut self.page_table_allocs,
> +            vfn_start,
> +            pfns,
> +            writable,
> +        ) {
> +            self.free_vfn(vfn_start);
> +            return Err(e);
> +        }

[Severity: Critical]
Does freeing the VFN range on partial mapping failure leave dangling
hardware PTEs? If install_mappings() returns early due to an I/O error
after writing some PTEs, self.free_vfn() frees the virtual address space,
and the caller will likely free the underlying physical pages. However, the
successfully written hardware PTEs are not rolled back. This regression
allows the GPU to access the freed physical memory.

[ ... ]
> +    pub(crate) fn unmap_pages(&mut self, mm: &mut GpuMm<'_>, range: MappedRange) -> Result {
> +        let result = self
> +            .pt_map
> +            .invalidate_ptes(mm, range.vfn_start, range.num_pages);
> +
> +        // TODO: Internal page table pages (PDE, PTE pages) are still kept around.
> +        // This is by design as repeated maps/unmaps will be fast. As a future TODO,
> +        // we can add a reclaimer here to reclaim if VRAM is short. For now, the PT
> +        // pages are dropped once the `Vmm` is dropped.
> +
> +        // Free the VA range regardless of PTE invalidation success, so that the VA
> +        // range is recovered even on failure (PTEs may be stale, but that is better
> +        // than leaking both PTEs and VA range).
> +        self.free_vfn(range.vfn_start);

[Severity: Critical]
Is it safe to free the VFN range if PTE invalidation fails? If
invalidate_ptes() fails partially, explicit freeing of the VFN range
leaves active, stale PTEs in the hardware. This regression allows the GPU
to retain access to physical pages that the caller will assume are safe to
free and reuse.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-mmrebase-v1-0-8dd5d4225d2e@nvidia.com?part=13