[PATCH V1 1/1] accel/amdxdna: Fix unsafe use of handle_mm_fault()

[PATCH V1 1/1] accel/amdxdna: Fix unsafe use of handle_mm_fault()

[Lizhi Hou]

handle_mm_fault() must not be called from the mmap callback because
the VMA has not yet been linked. The handle_mm_fault() API contract
assumes that the VMA is already linked.

Remove the handle_mm_fault() call from the mmap callback. For shmem
BOs, use remap_pfn_range() instead. For imported BOs, mark the
mapping as invalid and rely on the first command submission to fault
in the pages.

Fixes: e486147c912f ("accel/amdxdna: Add BO import and export")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 43 ++++++++++++++++-------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 0d165b66c1fc..3092e327c500 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -13,6 +13,7 @@
 #include <linux/dma-buf.h>
 #include <linux/dma-direct.h>
 #include <linux/iosys-map.h>
+#include <linux/mm.h>
 #include <linux/pagemap.h>
 #include <linux/vmalloc.h>
 
@@ -490,16 +491,10 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj *abo,
 {
 	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
 	unsigned long num_pages = vma_pages(vma);
-	unsigned long offset = 0;
+	unsigned long i;
 	int ret;
 
-	if (!is_import_bo(abo)) {
-		ret = drm_gem_shmem_mmap(&abo->base, vma);
-		if (ret) {
-			XDNA_ERR(xdna, "Failed shmem mmap %d", ret);
-			return ret;
-		}
-	} else {
+	if (is_import_bo(abo)) {
 		vma->vm_private_data = NULL;
 		vma->vm_ops = NULL;
 		ret = dma_buf_mmap(abo->dma_buf, vma, 0);
@@ -508,23 +503,33 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj *abo,
 			return ret;
 		}
 
+		amdxdna_mark_mapp_invalid(abo, vma);
+
 		/* Drop the reference drm_gem_mmap_obj() acquired.*/
 		drm_gem_object_put(to_gobj(abo));
+		return 0;
+	}
+
+	ret = drm_gem_shmem_mmap(&abo->base, vma);
+	if (ret) {
+		XDNA_ERR(xdna, "Failed shmem mmap %d", ret);
+		return ret;
 	}
 
-	do {
-		vm_fault_t fault_ret;
+	for (i = 0; i < num_pages; i++) {
 
-		fault_ret = handle_mm_fault(vma, vma->vm_start + offset,
-					    FAULT_FLAG_WRITE, NULL);
-		if (fault_ret & VM_FAULT_ERROR) {
-			XDNA_ERR(xdna, "Fault in page failed");
-			amdxdna_mark_mapp_invalid(abo, vma);
-			break;
+		ret = remap_pfn_range(vma, vma->vm_start + (i << PAGE_SHIFT),
+				      page_to_pfn(abo->base.pages[vma->vm_pgoff + i]),
+				      PAGE_SIZE, vma->vm_page_prot);
+		if (ret) {
+			if (i > 0)
+				zap_special_vma_range(vma, vma->vm_start, i << PAGE_SHIFT);
+			dma_resv_lock(to_gobj(abo)->resv, NULL);
+			drm_gem_shmem_put_pages_locked(&abo->base);
+			dma_resv_unlock(to_gobj(abo)->resv);
+			return ret;
 		}
-
-		offset += PAGE_SIZE;
-	} while (--num_pages);
+	}
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH V1 1/1] accel/amdxdna: Fix unsafe use of handle_mm_fault()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Using remap_pfn_range() to map normal refcounted shmem folios creates special PTEs, violating MM invariants.
--

commit 2da717e72d7319305e74d66b3b963f132ff12737
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix unsafe use of handle_mm_fault()

This commit removes the unsafe use of handle_mm_fault() from the mmap
callback since the VMA is not yet linked. For shmem BOs, it replaces the
faulting loop with direct calls to remap_pfn_range(), and for imported BOs
it marks the mapping as invalid to be faulted in later.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 3f05fa8811af1..7a5b0ad3235fd 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c

[ ... ]

> @@ -490,16 +491,10 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj *abo,

[ ... ]

> +	ret = drm_gem_shmem_mmap(&abo->base, vma);
> +	if (ret) {
> +		XDNA_ERR(xdna, "Failed shmem mmap %d", ret);
> +		return ret;
>  	}
>  
> -	do {
> -		vm_fault_t fault_ret;
> +	for (i = 0; i < num_pages; i++) {
>  
> -		fault_ret = handle_mm_fault(vma, vma->vm_start + offset,
> -					    FAULT_FLAG_WRITE, NULL);
> -		if (fault_ret & VM_FAULT_ERROR) {
> -			XDNA_ERR(xdna, "Fault in page failed");
> -			amdxdna_mark_mapp_invalid(abo, vma);
> -			break;
> +		ret = remap_pfn_range(vma, vma->vm_start + (i << PAGE_SHIFT),
> +				      page_to_pfn(abo->base.pages[vma->vm_pgoff + i]),
> +				      PAGE_SIZE, vma->vm_page_prot);

[Severity: High]
Does using remap_pfn_range() here to map normal refcounted shmem folios
create special PTEs?

Since remap_pfn_range() is designed for raw PFN mappings, it sets the
special bit on the resulting page table entries. If this is applied to
normal refcounted folios from the shmem BO, it violates the memory
management invariant that normal folios must not have special PTEs.

When the special bit is set, vm_normal_page() returns NULL, which hides
these pages from page table walkers, GUP (get_user_pages), and
mapcount-based refcount management. Can this break reverse mapping (rmap)
and lead to memory management corruption?

> +		if (ret) {
> +			if (i > 0)
> +				zap_special_vma_range(vma, vma->vm_start, i << PAGE_SHIFT);
> +			dma_resv_lock(to_gobj(abo)->resv, NULL);
> +			drm_gem_shmem_put_pages_locked(&abo->base);
> +			dma_resv_unlock(to_gobj(abo)->resv);
> +			return ret;
>  		}
> -
> -		offset += PAGE_SIZE;
> -	} while (--num_pages);
> +	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909163211.950709-1-lizhi.hou@amd.com?part=1