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 <[email protected]> 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/[email protected]?part=13
