[PATCH] gpu: nova-core: mm: keep a page table page before installing its PDE

[PATCH] gpu: nova-core: mm: keep a page table page before installing its PDE

[Vladislav Zaharov]

install_mappings() writes the PDE of a new page table page before moving
the page into page_table_allocs. If push_within_capacity() then fails,
the page is dropped and its VRAM freed while the PDE still points at it.

Move the page into page_table_allocs first, so that a failure leaves no
PDE pointing at freed memory.

The problem was found by an LLM-assisted code review.

Fixes: 4e4182282d4e ("gpu: nova-core: mm: Add multi-page mapping API to VMM")
Signed-off-by: Vladislav Zaharov <vladazaharova2018@gmail.com>
---
I could not reproduce the failure, as it needs push_within_capacity() to
fail. With the change, the MM self-tests still pass on a GB203.

 drivers/gpu/nova-core/mm/pagetable/map.rs | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/nova-core/mm/pagetable/map.rs b/drivers/gpu/nova-core/mm/pagetable/map.rs
index f7774b9cc972..e678197ec009 100644
--- a/drivers/gpu/nova-core/mm/pagetable/map.rs
+++ b/drivers/gpu/nova-core/mm/pagetable/map.rs
@@ -209,8 +209,15 @@ pub(super) fn install_mappings(
                 let (install_addr, page) = node.to_key_value();
                 let page_vram =
                     VramAddress::from_raw(page.alloc.iter().next().ok_or(ENOMEM)?.offset());
+                let level = page.level;
 
-                if page.level == M::DUAL_PDE_LEVEL {
+                // Keep the page before pointing a PDE at it, so that a failure here cannot leave a
+                // PDE pointing at freed VRAM.
+                page_table_allocs
+                    .push_within_capacity(page.alloc)
+                    .map_err(|_| ENOMEM)?;
+
+                if level == M::DUAL_PDE_LEVEL {
                     let new_dpde = M::DualPde::new_small(Pfn::from(page_vram));
                     new_dpde.write(pramin, install_addr)?;
                 } else {
@@ -218,10 +225,6 @@ pub(super) fn install_mappings(
                     new_pde.write(pramin, install_addr)?;
                 }
 
-                page_table_allocs
-                    .push_within_capacity(page.alloc)
-                    .map_err(|_| ENOMEM)?;
-
                 cursor = next;
             }
 

base-commit: 0ff9e5b9ff6736c37593cd7db89f4593ec4f8e86
-- 
2.56.0

Re: [PATCH] gpu: nova-core: mm: keep a page table page before installing its PDE

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] gpu: nova-core: mm: dangling child pt_pages entries in
  install_mappings()
--

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

gpu: nova-core: mm: keep a page table page before installing its PDE

This commit moves the page_table_allocs.push_within_capacity() call in
install_mappings() ahead of writing the PDE for each prepared page table
page.

> diff --git a/drivers/gpu/nova-core/mm/pagetable/map.rs b/drivers/gpu/nova-core/mm/pagetable/map.rs
> index f7774b9cc972f..e678197ec009c 100644
> --- a/drivers/gpu/nova-core/mm/pagetable/map.rs
> +++ b/drivers/gpu/nova-core/mm/pagetable/map.rs
> @@ -209,8 +209,15 @@ pub(super) fn install_mappings(
>                  let (install_addr, page) = node.to_key_value();
>                  let page_vram =
>                      VramAddress::from_raw(page.alloc.iter().next().ok_or(ENOMEM)?.offset());
> +                let level = page.level;
>  
> -                if page.level == M::DUAL_PDE_LEVEL {
> +                // Keep the page before pointing a PDE at it, so that a failure here cannot leave a
> +                // PDE pointing at freed VRAM.
> +                page_table_allocs
> +                    .push_within_capacity(page.alloc)
> +                    .map_err(|_| ENOMEM)?;

[Severity: Medium]
Does failing in push_within_capacity() or the following PDE write still
leave child entries in pt_pages pointing into freed or unlinked parent
page table VRAM?

ensure_single_pte_path() inserts missing page table levels top-down into
Vmm::pt_pages, where a child node's install_addr key is a physical address
inside its prepared parent page's VRAM allocation.  In install_mappings(),
c.remove_current() removes the current node before push_within_capacity()
is called.

If a parent node is visited before its child and push_within_capacity()
fails on the parent, dropping page.alloc frees the parent's VRAM while the
child node remains in Vmm::pt_pages (which Vmm::execute_map() does not
clear on error); a later Vmm::execute_map() or Vmm::map_pages() call then
drains Vmm::pt_pages and writes the child PDE into the freed parent VRAM
page.  Conversely, if the child is visited first in VramAddress order, its
PDE is written into the parent page and its allocation is kept in
page_table_allocs before the parent fails and is freed, and if the PDE
write itself fails after push_within_capacity() succeeds, the parent page
is kept in page_table_allocs without being linked into the hardware page
tables while remaining children in pt_pages still target it.

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