Re: [PATCH 12/16] gpu: nova-core: mm: Add virtual address range tracking to VMM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `free_vfn` inadvertently frees entire multi-page or unrelated adjacent allocations because the maple tree automatically coalesces identical values and `erase` removes the full node.
- [High] `alloc_vfn_range` silently truncates unaligned requested virtual addresses instead of returning an error, mapping a different virtual address range than requested.
- [High] `alloc_vfn_range` fails to validate that an explicitly requested `va_range` falls within the VMM's maximum address space limits.
--

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

gpu: nova-core: mm: Add virtual address range tracking to VMM
    
This commit adds a maple tree allocator for tracking contiguous virtual
address ranges. It enables allocating and freeing virtual frame numbers
for mappings.

> diff --git a/drivers/gpu/nova-core/mm/vmm.rs b/drivers/gpu/nova-core/mm/vmm.rs
> index a7911efb65a03..0bcae29db4f2c 100644
> --- a/drivers/gpu/nova-core/mm/vmm.rs
> +++ b/drivers/gpu/nova-core/mm/vmm.rs

[ ... ]

> @@ -34,18 +43,74 @@ pub(crate) struct Vmm {
>      mmu_version: MmuVersion,
>      /// Page table allocations required for mappings.
>      page_table_allocs: KVec<Pin<KBox<AllocatedBlocks>>>,
> +    /// Maple tree allocator for virtual address range tracking.
> +    virt_alloc: Pin<KBox<MapleTreeAlloc<()>>>,
> +    /// Total number of pages in the virtual address space.
> +    va_pages: usize,
>  }
>  
>  impl Vmm {
>      /// Create a new [`Vmm`] for the given Page Directory Base address.
> -    pub(crate) fn new(pdb_addr: VramAddress, mmu_version: MmuVersion) -> Result<Self> {
> +    ///
> +    /// The [`Vmm`] will manage a virtual address space of `va_size` bytes.
> +    pub(crate) fn new(
> +        pdb_addr: VramAddress,
> +        mmu_version: MmuVersion,
> +        va_size: u64,
> +    ) -> Result<Self> {
> +        let page_size: u64 = PAGE_SIZE.into_safe_cast();
> +        let va_pages: usize = (va_size / page_size).into_safe_cast();
> +        let virt_alloc = KBox::pin_init(MapleTreeAlloc::<()>::new(), GFP_KERNEL)?;

[Severity: High]
Since the maple tree allocator is instantiated with () as the value, won't it
automatically coalesce contiguous allocations into a single node because the
stored pointers are identical?

> +
>          Ok(Self {
>              pdb_addr,
>              mmu_version,
>              page_table_allocs: KVec::new(),
> +            virt_alloc,
> +            va_pages,
>          })
>      }
>  
> +    /// Allocate a contiguous virtual frame number range.
> +    ///
> +    /// # Arguments
> +    ///
> +    /// - `num_pages`: Number of pages to allocate.
> +    /// - `va_range`: `None` = allocate anywhere, `Some(range)` = constrain allocation to the given
> +    ///   range.
> +    fn alloc_vfn_range(&self, num_pages: usize, va_range: Option<Range<u64>>) -> Result<Vfn> {
> +        let page_size: u64 = PAGE_SIZE.into_safe_cast();
> +
> +        let start_vfn = match va_range {
> +            Some(r) => {
> +                let num_pages_u64: u64 = num_pages.into_safe_cast();
> +                let size = num_pages_u64.checked_mul(page_size).ok_or(EOVERFLOW)?;
> +                let range_size = r.end.checked_sub(r.start).ok_or(EOVERFLOW)?;
> +                if range_size != size {
> +                    return Err(EINVAL);
> +                }
> +                let start_vfn: usize = (r.start / page_size).into_safe_cast();
> +                let end_vfn: usize = (r.end / page_size).into_safe_cast();

[Severity: High]
Could the integer division truncate misaligned addresses here?

If r.start or r.end are not page-aligned, this division silently maps a
different page-aligned address instead of returning -EINVAL for the misaligned
request.

> +                self.virt_alloc
> +                    .insert_range(start_vfn..end_vfn, (), GFP_KERNEL)?;

[Severity: High]
Does this insertion need to be validated against the VMM's maximum address space
limit?

If an explicitly requested end_vfn exceeds self.va_pages, this succeeds but
could later cause wrap-around truncation and page table corruption during
hardware programming.

> +                start_vfn
> +            }
> +            None => self
> +                .virt_alloc
> +                .alloc_range(num_pages, (), ..self.va_pages, GFP_KERNEL)?,
> +        };
> +
> +        Ok(Vfn::new(start_vfn.into_safe_cast()))
> +    }
> +
> +    /// Free a virtual frame number range back to the maple tree.
> +    fn free_vfn(&self, vfn: Vfn) {
> +        let vfn_index: usize = vfn.raw().into_safe_cast();
> +        if self.virt_alloc.erase(vfn_index).is_none() {

[Severity: High]
If the maple tree automatically coalesced contiguous allocations because they all
share the () value, won't erase() remove the entire coalesced node rather
than just the originally requested range?

This could inadvertently free unrelated adjacent mappings or the remainder of a
multi-page allocation.

> +            kernel::pr_warn!("free_vfn: VFN {} not found in maple tree\n", vfn_index);
> +        }
> +    }

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