Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [High] Double unmap of DMA pages in nouveau_dmem_evict_chunk error path. - [High] DMA mapping leak in device memory migration paths for THP folios. - [High] Device memory migration completely broken due to invalid page_shift in nouveau_pfns_map(). - [High] Splitting a THP folio will cause chunk->callocated to underflow. - [High] NULL pointer dereference occurs in nouveau_dmem_chunk_alloc() when migrating large folios if CONFIG_TRANSPARENT_HUGEPAGE is disabled. - [Medium] Device memory leak of 508KB per chunk in nouveau_dmem_chunk_alloc() for non-THP allocations. -- --- Patch [1]: [PATCH 1/2] drm/nouveau/dmem: pin VRAM for the whole registered range --- commit d125a5f88d0941299dc186384f431b1921ef50ed Author: Junrui Luo <[email protected]> drm/nouveau/dmem: pin VRAM for the whole registered range Commit c32287471077 ("gpu/drm/nouveau: enable THP support for GPU memory migration") grew the device-private region that nouveau_dmem_chunk_alloc() registers from DMEM_CHUNK_SIZE to DMEM_CHUNK_SIZE * NR_CHUNKS, but left the VRAM buffer object backing that region at DMEM_CHUNK_SIZE. This commit sizes the buffer object to the region it backs. Fixes: c32287471077 ("gpu/drm/nouveau: enable THP support for GPU memory migration") Reported-by: Yuhao Jiang <[email protected]> Assisted-by: Claude:claude-opus-5 Cc: [email protected] Signed-off-by: Junrui Luo <[email protected]> > diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c > b/drivers/gpu/drm/nouveau/nouveau_dmem.c > index 9442ec6e1f6c1..356ff8f3c1b84 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c > +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c > @@ -325,8 +325,8 @@ nouveau_dmem_chunk_alloc(struct nouveau_drm *drm, struct > page **ppage, > chunk->pagemap.ops = &nouveau_dmem_pagemap_ops; > chunk->pagemap.owner = drm->dev; > > - ret = nouveau_bo_new_pin(&drm->client, NOUVEAU_GEM_DOMAIN_VRAM, > DMEM_CHUNK_SIZE, > - &chunk->bo); > + ret = nouveau_bo_new_pin(&drm->client, NOUVEAU_GEM_DOMAIN_VRAM, > + DMEM_CHUNK_SIZE * NR_CHUNKS, &chunk->bo); > if (ret) > goto out_release; [ ... ] > pfn = pfn_first; > for (i = 0; i < NR_CHUNKS; i++) { > int j; > > if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) || !is_large) { > for (j = 0; j < DMEM_CHUNK_NPAGES - 1; j++, pfn++) { [Severity: Medium] This isn't a bug introduced by this patch, but does this loop skip and leak pages? The loop initialization runs NR_CHUNKS times, and its inner loop runs DMEM_CHUNK_NPAGES - 1 times, continually incrementing pfn. This populates exactly NR_CHUNKS * (DMEM_CHUNK_NPAGES - 1) pages into free_pages. After the loop, 1 page is popped and returned to the caller, leaving the remaining pages in free_pages. The rest of the pages at the end of the allocated physical and VRAM region appear to be uninitialized and permanently leaked. [ ... ] > } else { > page = pfn_to_page(pfn); > page->zone_device_data = drm->dmem->free_folios; > drm->dmem->free_folios = page_folio(page); > pfn += DMEM_CHUNK_NPAGES; > } > } > > /* Move to next page */ > if (is_large) { > *ppage = &drm->dmem->free_folios->page; [Severity: High] This is a pre-existing issue, but can this dereference a NULL pointer? If migrate_vma_setup() sets MIGRATE_PFN_COMPOUND for large page cache folios, is_large evaluates to true. If CONFIG_TRANSPARENT_HUGEPAGE is disabled, the earlier condition evaluates to true and populates free_pages instead of free_folios. Immediately after, the code checks if (is_large) and attempts to dereference drm->dmem->free_folios->page, which would still be NULL. drivers/gpu/drm/nouveau/nouveau_dmem.c:nouveau_dmem_evict_chunk() { ... nouveau_dmem_copy_folio(chunk->drm, page_folio(migrate_pfn_to_page(src_pfns[i])), page_folio(dpage), &dma_info[i]); ... dma_unmap_page(chunk->drm->dev->dev, dma_info[i].dma_addr, dma_info[i].size, DMA_BIDIRECTIONAL); ... } [Severity: High] This is a pre-existing issue, but does this lead to a double unmap of DMA pages on error? In nouveau_dmem_copy_folio(), if the internal copy fails, it calls dma_unmap_page() but returns -EIO without clearing dma_info->dma_addr or size. The loop in nouveau_dmem_evict_chunk() ignores the return value from nouveau_dmem_copy_folio() and proceeds. It then unconditionally loops over all npages and calls dma_unmap_page() using dma_info[i]. For pages that failed to copy, wouldn't this unmap the same DMA address a second time, potentially corrupting DMA API state? drivers/gpu/drm/nouveau/nouveau_dmem.c:nouveau_dmem_migrate_to_ram() { ... dma_unmap_page(drm->dev->dev, dma_info.dma_addr, PAGE_SIZE, DMA_BIDIRECTIONAL); ... } [Severity: High] This is a pre-existing issue, but does this hardcoded PAGE_SIZE leak DMA mappings for THP folios? nouveau_dmem_copy_folio() maps destination pages using page_size(dpage), which evaluates to the full size for THP folios, and stores this in dma_info.size. However, the cleanup paths in nouveau_dmem_migrate_to_ram() on success, and nouveau_dmem_migrate_copy_one() on error, hardcode PAGE_SIZE when calling dma_unmap_page(). Does this leave the remaining pages of the folio permanently mapped in the IOMMU? drivers/gpu/drm/nouveau/nouveau_dmem.c:nouveau_dmem_migrate_chunk() { ... nouveau_pfns_map(svmm, args->vma->vm_mm, args->start, pfns, i, order); ... } drivers/gpu/drm/nouveau/nouveau_svm.c:nouveau_pfns_map() { ... args->p.size = npages << page_shift; args->p.page = page_shift; ... } [Severity: High] This is a pre-existing issue, but is the page_shift parameter incorrect here? In nouveau_dmem_migrate_chunk(), nouveau_pfns_map() is called with order (which is 0 or 9) instead of the actual shift like PAGE_SHIFT (12 or 21). In nouveau_pfns_map(), this parameter is used to populate args->p.size and args->p.page. Because npages is the number of 4KB pages processed, if order = 0 is passed, size evaluates to exactly the number of 4KB pages in bytes, instead of the correct mapping size. Does this cause the VMM IOCTL backend to reject the mapping payload due to invalid sizes and shifts? drivers/gpu/drm/nouveau/nouveau_dmem.c:nouveau_dmem_folio_split() { ... static void nouveau_dmem_folio_split(struct folio *head, struct folio *tail) ... } drivers/gpu/drm/nouveau/nouveau_dmem.c:nouveau_dmem_folio_free() { ... WARN_ON(!chunk->callocated); chunk->callocated--; ... } [Severity: High] This isn't a bug introduced by this patch, but does splitting a THP folio cause an underflow of chunk->callocated? When a large THP folio is allocated, it increments chunk->callocated by 1. If the kernel splits this folio, nouveau_dmem_folio_split() is called for each tail page. However, nouveau_dmem_folio_split() does not appear to adjust chunk->callocated to account for the newly split discrete pages. When these split pages are subsequently freed, nouveau_dmem_folio_free() will decrement chunk->callocated multiple times for what was originally a single increment. Will this underflow chunk->callocated and trigger the WARN_ON() checks? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
