--
commit 591a056ad47c1c9d032b2eceed7f51b0bb221d8d
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 svm_page_is_range_owned() before computing a VRAM domain
address in svm_range_dma_map_dev(). Device-private pages not owned by
the range trigger a warning and return -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..5807a26d7debf 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,11 @@ 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 (WARN_ON_ONCE(is_device_private_page(page))) {
+ /* Device private memory from another driver or range.
+ * This should never happen.
+ */
+ return -EINVAL;
}
[Severity: High]
Can device-private pages from another SVM range or a parent process on
the same GPU or XGMI hive reach this branch and trigger WARN_ON_ONCE()
and -EINVAL?
On GPUs using MEMORY_DEVICE_PRIVATE, kgd2kfd_init_zone_device() sets
pgmap->owner to SVM_ADEV_PGMAP_OWNER(adev), which is a device-wide or
hive-wide pointer shared across all processes and all svm_range structs
on that GPU or hive.
In svm_range_validate_and_map(), owner is computed via
kfd_svm_page_owner(), which unconditionally returns
SVM_ADEV_PGMAP_OWNER(pdd->dev->adev) even when prange->ttm_res is NULL
or prange->svm_bo is a different BO:
drivers/gpu/drm/amd/amdkfd/kfd_svm.c:kfd_svm_page_owner() {
...
return SVM_ADEV_PGMAP_OWNER(pdd->dev->adev);
}
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;
}
}
...
r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, npages,
readonly, owner,
ctx->hmm_range[bitmap_unchanged]);
}
When amdgpu_hmm_range_get_pages() calls hmm_range_fault(),
hmm_vma_handle_pte() skips faulting any device-private swap entry whose
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;
}
}
If a process validates an SVM range containing MEMORY_DEVICE_PRIVATE
pages inherited via fork() or moved via mremap() from another range on
the same GPU or hive, hmm_range_fault() will not fault those pages back
to system RAM and will return them in hmm_pfns. When
svm_range_dma_map_dev() then checks svm_page_is_range_owned(page,
prange), it returns false because prange->ttm_res is NULL or
page->zone_device_data != prange->svm_bo, hitting the WARN_ON_ONCE() and
failing range validation with -EINVAL.
[Severity: High]
Also, if this -EINVAL path is taken when addr[i] previously held a valid
system-memory DMA mapping, does it leave a stale DMA address in addr[i]
that causes a double dma_unmap_page()?
At the start of the loop in svm_range_dma_map_dev(), an existing valid
mapping is unmapped without clearing addr[i]:
drivers/gpu/drm/amd/amdkfd/kfd_svm.c:svm_range_dma_map_dev() {
...
for (i = 0; i < npages; i++) {
if (svm_is_valid_dma_mapping_addr(dev, addr[i]))
dma_unmap_page(dev, addr[i], PAGE_SIZE, dir);
}
Prior to this change, every path after dma_unmap_page() overwrote
addr[i]. Returning -EINVAL before updating or zeroing addr[i] leaves the
unmapped DMA address in prange->dma_addr[gpuidx], so a later call to
svm_range_dma_unmap_dev() (or a subsequent svm_range_dma_map_dev() call)
will see svm_is_valid_dma_mapping_addr(dev, addr[i]) as true and call
dma_unmap_page() a second time on the same address:
drivers/gpu/drm/amd/amdkfd/kfd_svm.c:svm_range_dma_unmap_dev() {
...
for (i = offset; i < offset + npages; i++) {
if (!svm_is_valid_dma_mapping_addr(dev, dma_addr[i]))
continue;
pr_debug_ratelimited("unmap 0x%llx\n", dma_addr[i] >>
PAGE_SHIFT);
dma_unmap_page(dev, dma_addr[i], PAGE_SIZE, dir);
dma_addr[i] = 0;
}
}
addr[i] = dma_map_page(dev, page, 0, PAGE_SIZE, dir);
r = dma_mapping_error(dev, addr[i]);
[Severity: Medium]
Is it valid to pass non-private ZONE_DEVICE pages (such as DAX pages or
foreign MEMORY_DEVICE_COHERENT pages) to dma_map_page() here?
When svm_page_is_range_owned(page, prange) and
is_device_private_page(page) both return false for a ZONE_DEVICE page,
execution falls through to dma_map_page(), which expands to
dma_map_page_attrs(). In kernel/dma/mapping.c, dma_map_page_attrs()
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 coherent device pages from another GPU
hive trigger this warning and fail with -ENOMEM on kernels with
CONFIG_DMA_API_DEBUG enabled?
