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

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

[Felix Kuehling]

This issue was raised independently in two contexts: pages from DAX
devices and pages from other GPU hives or inherited from other
processed. This patch addresses both concerns together along with
Sashiko comments raised for one of the previous reviews:
https://sashiko.dev/#/patchset/20261103-kfd-svm-foreign-vram-20261103-v1-1-d6282053cad3@gmail.com?part=1

Before treating a page as a zone device page belonging to the SVM range,
check that it actually belongs to that range based on the
zone_device_data. Non-private pages belonging to other devices or ranges
can be treated like system memory for the purpose of DMA mapping.

Cc: matthewkrebs8@gmail.com
Cc: jm5905938@gmail.com
Signed-off-by: Felix Kuehling <felix.kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 876476046926..649f7192b861 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;
 		}
 		addr[i] = dma_map_page(dev, page, 0, PAGE_SIZE, dir);
 		r = dma_mapping_error(dev, addr[i]);
-- 
2.43.0

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

[sashiko-bot]

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

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

[Kuehling, Felix]

On 2026-11-05 14:11, sashiko-bot@kernel.org wrote:
> 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()

I'll address these in the next revision.


> - [Medium] drm/amdkfd: calling dma_map_page() on zone_device pages in svm_range_dma_map_dev()

Matthew, I believe this would have applied to your patch "drm/amdkfd: 
Only take the VRAM path for KFD device pages in svm_range_dma_map_dev" 
as well. See details below. So this is probably not a complete solution 
for your problem.

Regards,
   Felix


> --
>
> 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?
>