Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Missing NULL check for destination device pages in `amdgpu_svm_copy_to_devmem` leads to arbitrary GPU physical memory corruption. - [High] Missing dma_resv lock acquisition before migration causes lockdep warnings, an imbalanced unlock, and a potential Use-After-Free in `amdgpu_svm_populate_mm`. - [High] `amdgpu_svm_copy_to_ram` executes asynchronous VRAM-to-RAM SDMA copies without waiting for prior GPU operations to complete, causing data corruption during eviction. --
commit 856e5efda96bd0348d0213ccfceffe664315be95 Author: Junhua Shen <[email protected]> Subject: drm/amdgpu: implement drm_pagemap SDMA migration callbacks This commit implements the drm_pagemap_devmem_ops and drm_pagemap_ops callbacks for the DRM GPUSVM migration framework. It introduces a new BO subtype for SVM to carry device memory allocations and implements mechanics for SDMA copies between system RAM and VRAM. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c [ ... ] > @@ -380,31 +380,31 @@ static int > amdgpu_svm_copy_memory_gart(struct amdgpu_device *adev, dma_addr_t *sys, > u64 *vram, u64 npages, int direction, > struct dma_fence **mfence) > { > const u64 max_pages = AMDGPU_GTT_MAX_TRANSFER_SIZE; > struct amdgpu_ring *ring = > to_amdgpu_ring(adev->mman.buffer_funcs_scheds[0]); > struct amdgpu_ttm_buffer_entity *entity = &adev->mman.move_entities[0]; > u64 gart_s, gart_d; > struct dma_fence *next; > u64 size; > int r; > > mutex_lock(&entity->lock); > > while (npages) { > size = min(max_pages, npages); > > if (direction == FROM_VRAM_TO_RAM) { > gart_s = amdgpu_svm_direct_mapping_addr(adev, *vram); > r = amdgpu_svm_gart_map(ring, entity, size, sys, > &gart_d, AMDGPU_PTE_WRITEABLE); > } else { > r = amdgpu_svm_gart_map(ring, entity, size, sys, > &gart_s, 0); > gart_d = amdgpu_svm_direct_mapping_addr(adev, *vram); > } > if (r) { > dev_err(adev->dev, "failed %d to map GART for SDMA\n", > r); > goto out_unlock; > } > > AMDGPU_MIGRATE_TRACE("SDMA_COPY: %s npages=%llu > vram_off=0x%llx\n", > direction == FROM_RAM_TO_VRAM ? "RAM->VRAM" : > "VRAM->RAM", > size, (u64)*vram); > > r = amdgpu_copy_buffer(adev, entity, gart_s, gart_d, > size * PAGE_SIZE, > NULL, &next, true, 0); [Severity: High] Can this SDMA copy race against active GPU compute shaders during eviction? Looking at amdgpu_svm_bo_evict, it calls drm_pagemap_evict_to_ram, which eventually calls amdgpu_svm_copy_to_ram without synchronously waiting on the BO resv. Then amdgpu_svm_copy_memory_gart passes NULL for the resv argument here to amdgpu_copy_buffer. Without a CPU wait on bo->tbo.base.resv or implicit dependencies added to the SDMA job, wouldn't the VRAM-to-RAM copy start immediately while writers might still be modifying the VRAM, migrating corrupted or torn data back to system memory? [ ... ] > + for (i = 0, j = 0; i < npages; i++) { > + if (!pagemap_addr[i].addr) > + goto flush; > + > + sys[j] = pagemap_addr[i].addr; > + vram[j] = ((u64)page_to_pfn(pages[i]) << PAGE_SHIFT) - > + svm_dm->hpa_base; [Severity: Critical] Does this code safely handle missing destination pages? The preamble loop in amdgpu_svm_copy_to_devmem acknowledges that the pages array may contain NULL entries, but this loop only checks if the source system page is valid via !pagemap_addr[i].addr. If pages[i] is NULL, evaluating page_to_pfn(pages[i]) will perform pointer arithmetic on a NULL pointer and produce a garbage PFN. Could this instruct the SDMA engine to silently overwrite arbitrary GPU physical memory? The sister function amdgpu_svm_copy_to_ram explicitly checks !pages[i]. > + > + /* Check if next vram page is contiguous with current */ > + if (j > 0 && vram[j] != vram[j - 1] + PAGE_SIZE) > + goto flush; > + > + j++; > + continue; > +flush: > + if (j) { > + ret = amdgpu_svm_copy_memory_gart(adev, sys, vram, j, > + FROM_RAM_TO_VRAM, > + &mfence); > + if (ret) > + goto out_fence; > + j = 0; > + } > + /* Re-process current page if it was valid but broke contiguity > */ > + if (pagemap_addr[i].addr) { > + sys[0] = pagemap_addr[i].addr; > + vram[0] = ((u64)page_to_pfn(pages[i]) << PAGE_SHIFT) - > + svm_dm->hpa_base; > + j = 1; > + } [Severity: Critical] Similarly, does the flush reprocessing block here need the same !pages[i] check to prevent translating a NULL page pointer into a bogus VRAM offset? [ ... ] > @@ -735,21 +735,21 @@ static int > amdgpu_svm_populate_mm(struct drm_pagemap *dpagemap, > unsigned long start, unsigned long end, > struct mm_struct *mm, > unsigned long timeslice_ms) > { > struct amdgpu_device *adev = dpagemap_to_adev(dpagemap); > struct drm_pagemap_migrate_details mdetails = { > .timeslice_ms = timeslice_ms, > }; > struct amdgpu_bo_svm *svm_bo; > int ret; > > svm_bo = amdgpu_bo_svm_alloc(adev, dpagemap, mm, end - start); > if (IS_ERR(svm_bo)) > return PTR_ERR(svm_bo); > > AMDGPU_MIGRATE_TRACE("populate_mm: [0x%lx-0x%lx] size=%lu\n", > start, end, end - start); > > ret = drm_pagemap_migrate_to_devmem(&svm_bo->devmem, > mm, start, end, > &mdetails); > > amdgpu_bo_unreserve(&svm_bo->bo); > > return ret; > } [Severity: High] Is this BO actually locked before being unreserved? In amdgpu_bo_svm_alloc, bp.resv is zero-initialized to NULL, which causes amdgpu_bo_create to explicitly unreserve the BO before returning it. So the BO is passed unlocked to drm_pagemap_migrate_to_devmem. This seems to cause three issues: 1. amdgpu_svm_populate_devmem_pfn will fail its dma_resv_assert_held(bo->tbo.base.resv) check. 2. amdgpu_bo_unreserve is attempting to unlock a ww_mutex that is already unlocked. 3. If the migration fails and the framework drops the devmem reference, amdgpu_svm_devmem_release frees the BO. The subsequent unreserve here would then dereference freed memory. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
