Re: [PATCH] drm/amdkfd: Check ownership of device pages for DMA mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/amdkfd: foreign same-hive device-private pages fail in svm_range_dma_map_dev()
- [High] drm/amdkfd: double dma_unmap_page() on error return in svm_range_dma_map_dev()
- [Medium] drm/amdkfd: calling dma_map_page() on zone_device pages in svm_range_dma_map_dev()
--

commit 591a056ad47c1c9d032b2eceed7f51b0bb221d8d
Author: Felix Kuehling <felix.kuehling@amd.com>

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105185828.85367-1-felix.kuehling@amd.com?part=1