Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] VRAM prefetch silently skips migrating mixed ranges where only some pages are in VRAM. Pre-existing issues: - [Critical] Stale GPU mappings are left in the page tables if `amdgpu_svm_range_update_gpu_range` fails midway, leading to a Use-After-Free. -- commit a10d03ff5d198ccb21336100307e785425a497b5 Author: Junhua Shen <[email protected]> drm/amdgpu: integrate VRAM migration into SVM fault and prefetch paths This commit integrates VRAM migration into the SVM page fault and prefetch paths. It calls amdgpu_svm_range_migrate_to_vram() when VRAM migration is needed and allows prefetch to actively move pages to VRAM by bypassing the valid-mapping fast path. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > index 6ac785a3b5cba..62b4e0c306682 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c [ ... ] > @@ -275,7 +277,8 @@ amdgpu_svm_range_update_gpu_range(struct amdgpu_svm *svm, > unsigned long start_page, last_page; > bool is_last_seg; > > - if (entry->proto != DRM_INTERCONNECT_SYSTEM) > + if (entry->proto != DRM_INTERCONNECT_SYSTEM && > + entry->proto != AMDGPU_INTERCONNECT_VRAM) > return -EOPNOTSUPP; [Severity: Critical] This is a pre-existing issue, but does this loop leave stale GPU mappings if it fails midway? Looking at amdgpu_svm_range_update_gpu_range(), it loops over DMA addresses and updates the GPU page tables. If it hits an unsupported protocol or amdgpu_vm_update_range() fails, it returns an error immediately: drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c:amdgpu_svm_range_update_gpu_range() { while (mapped_pages < npages) { ... if (entry->proto != DRM_INTERCONNECT_SYSTEM && entry->proto != AMDGPU_INTERCONNECT_VRAM) return -EOPNOTSUPP; ... ret = amdgpu_vm_update_range(...); if (ret) return ret; } } Because it returns without rolling back the successful segments, amdgpu_svm_range_update_mapping() will leave range->gpu_mapped set to false. Later, when the MMU notifier fires to unmap the memory, it will skip zapping the PTEs because gpu_mapped is false: drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c:amdgpu_svm_range_notifier_event_begin() { ... if (range->pages.flags.unmapped || !svm_range->gpu_mapped) return false; ... amdgpu_svm_range_zap_ptes(svm, svm_range, start_page, last_page); } Could this result in the GPU continuing to access memory after the CPU has freed it? [ ... ] > @@ -449,12 +454,24 @@ amdgpu_svm_range_map_attrs(struct amdgpu_svm *svm, > if (next_addr <= addr) > return -EINVAL; > > - if (amdgpu_svm_range_is_valid(svm, range, attrs)) { > + /* > + * Prefetch to device memory should still run migration logic > even if > + * the current GPU mapping is already valid. This allows > prefetch > + * requests to actively move backing pages to VRAM instead of > being > + * skipped by the valid-mapping fast path. > + */ > + if (amdgpu_svm_range_is_valid(svm, range, attrs) && > + !need_vram_migration) { > addr = next_addr; > continue; > } > > - /* TODO: add migration */ > + if (need_vram_migration) { > + AMDGPU_SVM_RANGE_DEBUG(range, "MIGRATE PAGES"); > + ret = amdgpu_svm_range_migrate_to_vram(svm, > &range->base); > + if (ret) > + return ret; > + } [Severity: Medium] Is it possible that this prefetch logic silently skips migrating mixed ranges where only some pages are in VRAM? The new logic explicitly bypasses the valid mapping check to allow prefetching, but amdgpu_svm_range_migrate_to_vram() relies on range_needs_migrate_to_vram(): drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range_migrate.c:range_needs_migrate_to_vram() { if (range_in_vram(range)) return false; ... } And range_in_vram() checks if there are ANY device memory pages: drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range_migrate.c:range_in_vram() { return flags.has_devmem_pages; } If a range is mixed (contains both system memory and VRAM pages), range_in_vram() returns true, causing amdgpu_svm_range_migrate_to_vram() to return immediately without migrating the remaining system pages. Might this defeat the explicit prefetch intent for partially resident ranges? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
