Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Insufficient bounds check in `bar_offset()` allows out-of-bounds multi-byte accesses across mapping boundaries. - [Medium] Decoupled `GpuMm` lifetime in `BarUser::map` and `release` allows cross-device page table corruption. - [Medium] `BarUserAccess` breaks RAII, silently leaking address space on standard Rust error paths. - [Medium] BAR1 is mapped as uncacheable MMIO instead of write-combining (WC), causing massive performance degradation. --
commit f4c54c8b0e0e82f490fc3199121f6bdbfd3815b4 Author: Joel Fernandes <[email protected]> Subject: gpu: nova-core: mm: Add BAR1 user interface Adds the BAR1 user interface for CPU access to GPU virtual memory through the BAR1 aperture. > diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs > index 5723ff8f71ea4..0672a0707a713 100644 > --- a/drivers/gpu/nova-core/driver.rs > +++ b/drivers/gpu/nova-core/driver.rs [ ... ] > @@ -82,12 +105,18 @@ fn probe<'bound>( > > Ok(try_pin_init!(NovaCore { > bar: pdev.iomap_region_sized::<BAR0_SIZE>(0, > c"nova-core/bar0")?, > - // TODO: Use `&bar` self-referential pin-init syntax once > available. > - // > - // SAFETY: `bar` is initialized before this expression is > evaluated > - // (`try_pin_init!()` initializes fields in declaration > order), lives at a pinned > - // stable address, and is dropped after `gpu` (struct field > drop order). > - gpu <- Gpu::new(pdev, unsafe { &*core::ptr::from_ref(bar) }), > + bar1: { > + let bar1_idx = bar1_resource_index(pdev)?; > + pdev.iomap_region(bar1_idx, c"nova-core/bar1")? [Severity: Medium] Will this uncacheable MMIO mapping cause severe performance bottlenecks for CPU-to-GPU transfers? BAR1 is the framebuffer aperture and is designed for bulk data transfers like USERD or command buffers. By using iomap_region (uncacheable), every write will generate a separate, synchronous PCIe transaction. Could this be mapped with write-combining instead, even if it requires adding new WC bindings to the Rust pci module? > + }, > + // TODO: Use self-referential pin-init syntax once available. [ ... ] > diff --git a/drivers/gpu/nova-core/mm/bar_user.rs > b/drivers/gpu/nova-core/mm/bar_user.rs > new file mode 100644 > index 0000000000000..ef1d8e6f8c9c3 > --- /dev/null > +++ b/drivers/gpu/nova-core/mm/bar_user.rs [ ... ] > + /// Map physical pages to a contiguous BAR1 virtual range. > + pub(crate) fn map( > + self: &Arc<Self>, > + mm: &mut GpuMm<'_>, > + pfns: &[Pfn], > + writable: bool, > + ) -> Result<BarUserAccess<'gpu>> { [Severity: Medium] Does this decoupled GpuMm lifetime allow cross-device page table corruption? Because the mm parameter uses an unbound anonymous lifetime instead of the 'gpu lifetime associated with BarUser, the Rust compiler cannot statically prevent cross-device mixing. In a multi-GPU system, could a caller inadvertently pass a GpuMm belonging to a different device into the map or release methods, causing the method to allocate from the first GPU's VMM but write page table entries to the second GPU's PRAMIN? > + if pfns.is_empty() { > + return Err(EINVAL); > + } > + let mut vmm = self.vmm.lock(); > + let mapped = vmm.map_pages(mm, pfns, None, writable)?; [ ... ] > + /// Translate an offset within this mapping to a BAR1 aperture offset. > + fn bar_offset(&self, offset: usize) -> Result<usize> { > + if offset >= self.size() { > + return Err(EINVAL); > + } [Severity: High] Does this bounds check properly account for the multi-byte access width? When callers use BarUserAccess::try_read32/64 or try_write32/64, the access extends 4 or 8 bytes past the provided offset. If offset = self.size() - 2, this check passes, but the access will overflow the mapping boundary. Could this lead to unmapped MMIO reads/writes or accesses into adjacent allocations? > + > + let base_vfn: usize = self.mapped().vfn_start.raw().into_safe_cast(); > + let base = base_vfn.checked_mul(PAGE_SIZE).ok_or(EOVERFLOW)?; > + base.checked_add(offset).ok_or(EOVERFLOW) > + } [ ... ] > +impl Drop for BarUserAccess<'_> { > + fn drop(&mut self) { > + if self.mapped.is_some() { > + kernel::pr_warn!( > + "BarUserAccess dropped without calling release(). BarUser > address space will leak.\n" > + ); > + } [Severity: Medium] Could this design break Rust's RAII guarantees and permanently leak address space on standard error paths? If a caller maps BAR1, performs some fallible operations using the ? operator, and returns an error before manually calling access.release(mm), the BAR1 virtual address space will leak. Since BarUserAccess does not hold a reference to GpuMm, its Drop implementation cannot unmap the pages. Is there a way to refactor this to ensure automatic cleanup during drop? > + // The inner `MappedRange`'s own `MustUnmapGuard` will also fire, > + // identifying the leaked VA range. > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
