Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/amdkfd: foreign same-hive device-private pages fail in svm_range_dma_map_dev() - [Medium] drm/amdkfd: zone_device pages passed to dma_map_page() in svm_range_dma_map_dev() --
commit 4c4b5ea0be7e18e7dd7d7ea4bdf8afccdf70d9ed Author: Felix Kuehling <[email protected]> drm/amdkfd: Check ownership of device pages for DMA mapping This commit checks whether a zone device page belongs to the target SVM range via zone_device_data before computing its VRAM address in svm_range_dma_map_dev(). Device-private pages from other devices or ranges are rejected with -EINVAL, while other non-owned pages fall through to dma_map_page(). > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c > b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c > index ed5e3318877be..7320a2981d3b2 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c > @@ -156,6 +156,18 @@ svm_is_valid_dma_mapping_addr(struct device *dev, > dma_addr_t dma_addr) > !(dma_addr & SVM_RANGE_VRAM_DOMAIN); > } > > +static bool svm_page_is_range_owned(struct page *page, struct svm_range > *prange) > +{ > + bool is_owned; > + > + mutex_lock(&prange->lock); > + is_owned = is_zone_device_page(page) && prange->ttm_res && > + page->zone_device_data == prange->svm_bo; > + mutex_unlock(&prange->lock); > + > + return is_owned; > +} > + > static int > svm_range_dma_map_dev(struct amdgpu_device *adev, struct svm_range *prange, > unsigned long offset, unsigned long npages, > @@ -180,7 +192,7 @@ svm_range_dma_map_dev(struct amdgpu_device *adev, struct > svm_range *prange, > dma_unmap_page(dev, addr[i], PAGE_SIZE, dir); > > page = hmm_pfn_to_page(hmm_pfns[i]); > - if (is_zone_device_page(page)) { > + if (svm_page_is_range_owned(page, prange)) { > struct amdgpu_device *bo_adev = > prange->svm_bo->node->adev; > > addr[i] = (hmm_pfns[i] << PAGE_SHIFT) + > @@ -189,6 +201,16 @@ svm_range_dma_map_dev(struct amdgpu_device *adev, struct > svm_range *prange, > addr[i] |= SVM_RANGE_VRAM_DOMAIN; > pr_debug_ratelimited("vram address: 0x%llx\n", addr[i]); > continue; > + } else if (is_device_private_page(page)) { > + /* Device private memory from another driver or range. > + * This should never happen in practice but it's > + * theoretically possible for read-only VMAs after a > + * process forks, creates a new KFD context and maps > + * the same page. > + */ > + pr_debug("Trying to map device private page from > another device or range\n"); > + addr[i] = 0; > + return -EINVAL; > } [Severity: High] As reported by Sashiko on v1 [1], can this also trigger on writable COW VMAs after fork() or after mremap() from another range on the same GPU or XGMI hive, causing svm_range_dma_map_dev() to fail with -EINVAL instead of faulting the page back to system RAM? On GPUs using MEMORY_DEVICE_PRIVATE, kgd2kfd_init_zone_device() sets pgmap->owner to SVM_ADEV_PGMAP_OWNER(adev), which is shared across all processes and all svm_range instances on that GPU or hive. In svm_range_validate_and_map(), kfd_svm_page_owner() passes this device-wide or hive-wide owner to amdgpu_hmm_range_get_pages() even when prange->ttm_res is NULL or prange->svm_bo does not own the underlying pages: drivers/gpu/drm/amd/amdkfd/kfd_svm.c:svm_range_validate_and_map() { ... owner = kfd_svm_page_owner(p, find_first_bit(ctx->bitmap, MAX_GPU_INSTANCE)); for_each_set_bit(idx, ctx->bitmap, MAX_GPU_INSTANCE) { if (kfd_svm_page_owner(p, idx) != owner) { owner = NULL; break; } } ... } When amdgpu_hmm_range_get_pages() calls hmm_range_fault(), hmm_vma_handle_pte() checks whether the device-private swap entry's pgmap->owner matches range->dev_private_owner: mm/hmm.c:hmm_vma_handle_pte() { ... if (softleaf_is_device_private(entry) && page_pgmap(softleaf_to_page(entry))->owner == range->dev_private_owner) { cpu_flags = HMM_PFN_VALID; if (softleaf_is_device_private_write(entry)) cpu_flags |= HMM_PFN_WRITE; new_pfn_flags = softleaf_to_pfn(entry) | cpu_flags; goto out; } ... } Because pgmap->owner matches range->dev_private_owner, hmm_vma_handle_pte() jumps directly to out before calling hmm_pte_need_fault(), returning the device-private PFN in hmm_pfns without faulting it back to system RAM (even when HMM_PFN_REQ_WRITE is set on a read-only COW entry after fork(), or when pages were moved via mremap() from another range). When those PFNs reach svm_range_dma_map_dev(), svm_page_is_range_owned() returns false because prange->ttm_res is NULL or page->zone_device_data != prange->svm_bo, and is_device_private_page(page) returns true, failing with -EINVAL and causing svm_range_restore_work() to loop indefinitely while user queues remain suspended. > addr[i] = dma_map_page(dev, page, 0, PAGE_SIZE, dir); [Severity: Medium] As reported by Sashiko on v1 [2], is it valid to pass non-private ZONE_DEVICE pages (such as DAX pages or foreign MEMORY_DEVICE_COHERENT pages from another GPU hive or process) to dma_map_page() here? When a page in hmm_pfns is a ZONE_DEVICE page that does not belong to prange (svm_page_is_range_owned(page, prange) is false) and is not a device-private page (is_device_private_page(page) is false), execution falls through to dma_map_page(), which expands to dma_map_page_attrs(). In kernel/dma/mapping.c, dma_map_page_attrs() explicitly rejects all ZONE_DEVICE pages when CONFIG_DMA_API_DEBUG is enabled: kernel/dma/mapping.c:dma_map_page_attrs() { ... if (IS_ENABLED(CONFIG_DMA_API_DEBUG) && WARN_ON_ONCE(is_zone_device_page(page))) return DMA_MAPPING_ERROR; ... } Wouldn't mapping DAX memory or foreign coherent device pages in an SVM range trigger this warning and fail with -ENOMEM when CONFIG_DMA_API_DEBUG is enabled? [1] https://lore.kernel.org/r/[email protected] [2] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
