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

[PATCH V3] 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 imported
BOs, mark the mapping as invalid and rely on the first command submission
to fault in the pages.

For shmem BOs, set the VM_MIXEDMAP flag and use vm_insert_pages().
Implement amdxdna_gem_mixed_vm_ops to handle the page faults.

Fixes: e486147c912f ("accel/amdxdna: Add BO import and export")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
v2 & v3:
  Fix sashiko comment.

 drivers/accel/amdxdna/amdxdna_gem.c | 151 ++++++++++++++++++++++------
 1 file changed, 118 insertions(+), 33 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 0d165b66c1fc..a12a762b3a6b 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -14,6 +14,7 @@
 #include <linux/dma-direct.h>
 #include <linux/iosys-map.h>
 #include <linux/pagemap.h>
+#include <linux/swap.h>
 #include <linux/vmalloc.h>
 
 #include "amdxdna_cbuf.h"
@@ -490,16 +491,16 @@ 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;
 	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 {
+	/*
+	 * Until today there is not any use case to mmap with non-zero
+	 * offset. Put an explicit check here.
+	 */
+	if (vma->vm_pgoff - drm_vma_node_start(&to_gobj(abo)->vma_node))
+		return -EINVAL;
+
+	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 +509,28 @@ 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;
 	}
 
-	do {
-		vm_fault_t fault_ret;
-
-		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 = drm_gem_shmem_mmap(&abo->base, vma);
+	if (ret) {
+		XDNA_ERR(xdna, "Failed shmem mmap %d", ret);
+		return ret;
+	}
 
-		offset += PAGE_SIZE;
-	} while (--num_pages);
+	vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
+	ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages, &num_pages);
+	if (ret) {
+		XDNA_ERR(xdna, "Failed to insert pages %d", ret);
+		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;
+	}
 
 	return 0;
 }
@@ -536,6 +542,10 @@ static int amdxdna_gem_obj_mmap(struct drm_gem_object *gobj,
 	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
 	int ret;
 
+	XDNA_DBG(xdna, "BO map_offset 0x%llx type %d userptr 0x%lx size 0x%lx",
+		 drm_vma_node_offset_addr(&gobj->vma_node), abo->type,
+		 vma->vm_start, gobj->size);
+
 	ret = amdxdna_hmm_register(abo, vma);
 	if (ret)
 		return ret;
@@ -546,9 +556,6 @@ static int amdxdna_gem_obj_mmap(struct drm_gem_object *gobj,
 		goto hmm_unreg;
 	}
 
-	XDNA_DBG(xdna, "BO map_offset 0x%llx type %d userptr 0x%lx size 0x%lx",
-		 drm_vma_node_offset_addr(&gobj->vma_node), abo->type,
-		 vma->vm_start, gobj->size);
 	return 0;
 
 hmm_unreg:
@@ -556,14 +563,99 @@ static int amdxdna_gem_obj_mmap(struct drm_gem_object *gobj,
 	return ret;
 }
 
+/*
+ * VM operations for amdxdna shmem VMAs that use VM_MIXEDMAP.
+ *
+ * drm_gem_shmem_vm_ops cannot be used on VM_MIXEDMAP VMAs because its fault
+ * handler calls vmf_insert_pfn() → vmf_insert_pfn_prot() which contains:
+ *   BUG_ON((vma->vm_flags & VM_MIXEDMAP) && pfn_valid(pfn))
+ * All amdxdna shmem pages are ordinary struct pages so pfn_valid() is always
+ * true, making the combination fatal.
+ *
+ * These ops use vmf_insert_page() (struct-page based) instead, which is the
+ * correct API for VM_MIXEDMAP VMAs backed by real struct pages.  The open and
+ * close handlers replicate drm_gem_shmem_vm_open/close using only exported
+ * symbols.
+ */
+static vm_fault_t amdxdna_gem_mixedmap_fault(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct drm_gem_object *gobj = vma->vm_private_data;
+	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
+	loff_t num_pages = gobj->size >> PAGE_SHIFT;
+	vm_fault_t ret = VM_FAULT_SIGBUS;
+	pgoff_t page_offset;
+	struct page *page;
+
+	/*
+	 * Partial free of vma is unexpected. Otherwise, the wrong page
+	 * will be faulted in and the user application may crash itself.
+	 */
+	page_offset = vmf->pgoff - vma->vm_pgoff;
+
+	dma_resv_lock(gobj->resv, NULL);
+
+	if (!shmem->pages || shmem->madv < 0 || page_offset >= num_pages)
+		goto out;
+
+	page = shmem->pages[page_offset];
+	if (WARN_ON_ONCE(!page))
+		goto out;
+
+	/*
+	 * Use vmf_insert_page() (struct-page path) not vmf_insert_pfn()
+	 * (PFN path) because this VMA carries VM_MIXEDMAP.
+	 */
+	ret = vmf_insert_page(vma, vmf->address, page);
+	if (ret == VM_FAULT_NOPAGE)
+		folio_mark_accessed(page_folio(page));
+
+out:
+	dma_resv_unlock(gobj->resv);
+	return ret;
+}
+
+static void amdxdna_gem_mixedmap_vm_open(struct vm_area_struct *vma)
+{
+	struct drm_gem_object *gobj = vma->vm_private_data;
+	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
+
+	/*
+	 * Bump pages_use_count so the page array stays alive for the new
+	 * mapping copy created by fork().  Mirrors drm_gem_shmem_vm_open().
+	 */
+	dma_resv_lock(gobj->resv, NULL);
+	drm_WARN_ON_ONCE(gobj->dev, !refcount_inc_not_zero(&shmem->pages_use_count));
+	dma_resv_unlock(gobj->resv);
+
+	drm_gem_vm_open(vma);
+}
+
+static void amdxdna_gem_mixedmap_vm_close(struct vm_area_struct *vma)
+{
+	struct drm_gem_object *gobj = vma->vm_private_data;
+	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
+
+	dma_resv_lock(gobj->resv, NULL);
+	drm_gem_shmem_put_pages_locked(shmem);
+	dma_resv_unlock(gobj->resv);
+
+	drm_gem_vm_close(vma);
+}
+
+static const struct vm_operations_struct amdxdna_gem_mixedmap_vm_ops = {
+	.fault  = amdxdna_gem_mixedmap_fault,
+	.open   = amdxdna_gem_mixedmap_vm_open,
+	.close  = amdxdna_gem_mixedmap_vm_close,
+};
+
 static int amdxdna_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
 {
 	struct drm_gem_object *gobj = dma_buf->priv;
 	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
-	unsigned long num_pages = vma_pages(vma);
 	int ret;
 
-	vma->vm_ops = &drm_gem_shmem_vm_ops;
+	vma->vm_ops = &amdxdna_gem_mixedmap_vm_ops;
 	vma->vm_private_data = gobj;
 
 	drm_gem_object_get(gobj);
@@ -573,16 +665,9 @@ static int amdxdna_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struc
 
 	/* The buffer is based on memory pages. Fix the flag. */
 	vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
-	ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages,
-			      &num_pages);
-	if (ret)
-		goto close_vma;
 
 	return 0;
 
-close_vma:
-	vma->vm_ops->close(vma);
-	return ret;
 put_obj:
 	drm_gem_object_put(gobj);
 	return ret;
@@ -878,7 +963,7 @@ static const struct drm_gem_object_funcs amdxdna_gem_shmem_funcs = {
 	.vmap = amdxdna_gem_obj_vmap,
 	.vunmap = amdxdna_gem_obj_vunmap,
 	.mmap = amdxdna_gem_obj_mmap,
-	.vm_ops = &drm_gem_shmem_vm_ops,
+	.vm_ops = &amdxdna_gem_mixedmap_vm_ops,
 	.export = amdxdna_gem_prime_export,
 };
 
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Flawed VMA offset calculation in amdxdna_gem_mixedmap_fault causes data corruption on VMA splits and DMA-buf mmaps.

Pre-existing issues:
- [High] DMA-buf mmaps bypass HMM notifier registration, causing missing pre-faulting and unsafe page table teardown.
--

commit 5de79a60bd0ef403c4515e6d277ad3386e515ac9
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix unsafe use of handle_mm_fault()

This commit removes handle_mm_fault() from the mmap callback because the
VMA is not yet linked. For imported BOs, it marks the mapping invalid and
relies on the first command submission to fault in the pages. For shmem
BOs, it sets VM_MIXEDMAP and implements amdxdna_gem_mixed_vm_ops to handle
the page faults.

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

[ ... ]

> +static vm_fault_t amdxdna_gem_mixedmap_fault(struct vm_fault *vmf)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct drm_gem_object *gobj = vma->vm_private_data;
> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
> +	loff_t num_pages = gobj->size >> PAGE_SHIFT;
> +	vm_fault_t ret = VM_FAULT_SIGBUS;
> +	pgoff_t page_offset;
> +	struct page *page;
> +
> +	/*
> +	 * Partial free of vma is unexpected. Otherwise, the wrong page
> +	 * will be faulted in and the user application may crash itself.
> +	 */
> +	page_offset = vmf->pgoff - vma->vm_pgoff;

[Severity: High]
Will this calculation cause the wrong pages to be mapped on VMA splits or
non-zero offset DMA-buf mmaps?

Because the core MM computes vmf->pgoff as:
vmf->pgoff = vma->vm_pgoff + ((vmf->address - vma->vm_start) >> PAGE_SHIFT)

The subtraction here cancels out the VMA's logical offset, causing it to
always yield the offset relative to the current VMA start. If a user mmaps a
GEM object, unmaps the first half to split the VMA, and then triggers a page
fault on the second half (e.g., after MADV_DONTNEED), could this erroneously
map pages[0] into the middle of the object?

> +
> +	dma_resv_lock(gobj->resv, NULL);

[ ... ]

>  static int amdxdna_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>  {
>  	struct drm_gem_object *gobj = dma_buf->priv;
>  	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
> -	unsigned long num_pages = vma_pages(vma);
>  	int ret;

[Severity: High]
This is a pre-existing issue, but does amdxdna_gem_dmabuf_mmap() need to
call amdxdna_hmm_register() here?

Without this call, the VMA is not added to the HMM tracking list. If a user
exports a BO to a DMA-buf, mmaps the DMA-buf, and passes the virtual address
to the NPU, the driver might fail to pre-fault the pages before execution.

Additionally, if the user unmaps the VMA while the NPU is active, could the
lack of an MMU interval notifier bypass the driver's teardown stall
(dma_resv_wait_timeout()), allowing the CPU to tear down page tables while
the NPU is still executing and causing IOMMU translation faults?

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

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

[Max Zhen]

On 9/10/2026 Thu 14:13, Lizhi Hou wrote:
> 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 imported
> BOs, mark the mapping as invalid and rely on the first command submission
> to fault in the pages.
> 
> For shmem BOs, set the VM_MIXEDMAP flag and use vm_insert_pages().
> Implement amdxdna_gem_mixed_vm_ops to handle the page faults.
> 
> Fixes: e486147c912f ("accel/amdxdna: Add BO import and export")
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Reviewed-by: Max Zhen <max.zhen@amd.com>
> ---
> v2 & v3:
>    Fix sashiko comment.
> 
>   drivers/accel/amdxdna/amdxdna_gem.c | 151 ++++++++++++++++++++++------
>   1 file changed, 118 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 0d165b66c1fc..a12a762b3a6b 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -14,6 +14,7 @@
>   #include <linux/dma-direct.h>
>   #include <linux/iosys-map.h>
>   #include <linux/pagemap.h>
> +#include <linux/swap.h>
>   #include <linux/vmalloc.h>
>   
>   #include "amdxdna_cbuf.h"
> @@ -490,16 +491,16 @@ 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;
>   	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 {
> +	/*
> +	 * Until today there is not any use case to mmap with non-zero
> +	 * offset. Put an explicit check here.
> +	 */
> +	if (vma->vm_pgoff - drm_vma_node_start(&to_gobj(abo)->vma_node))
> +		return -EINVAL;
> +
> +	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 +509,28 @@ 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;
>   	}
>   
> -	do {
> -		vm_fault_t fault_ret;
> -
> -		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 = drm_gem_shmem_mmap(&abo->base, vma);
> +	if (ret) {
> +		XDNA_ERR(xdna, "Failed shmem mmap %d", ret);
> +		return ret;
> +	}
>   
> -		offset += PAGE_SIZE;
> -	} while (--num_pages);
> +	vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
> +	ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages, &num_pages);
> +	if (ret) {
> +		XDNA_ERR(xdna, "Failed to insert pages %d", ret);
> +		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;
> +	}
>   
>   	return 0;
>   }
> @@ -536,6 +542,10 @@ static int amdxdna_gem_obj_mmap(struct drm_gem_object *gobj,
>   	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
>   	int ret;
>   
> +	XDNA_DBG(xdna, "BO map_offset 0x%llx type %d userptr 0x%lx size 0x%lx",
> +		 drm_vma_node_offset_addr(&gobj->vma_node), abo->type,
> +		 vma->vm_start, gobj->size);
> +
>   	ret = amdxdna_hmm_register(abo, vma);
>   	if (ret)
>   		return ret;
> @@ -546,9 +556,6 @@ static int amdxdna_gem_obj_mmap(struct drm_gem_object *gobj,
>   		goto hmm_unreg;
>   	}
>   
> -	XDNA_DBG(xdna, "BO map_offset 0x%llx type %d userptr 0x%lx size 0x%lx",
> -		 drm_vma_node_offset_addr(&gobj->vma_node), abo->type,
> -		 vma->vm_start, gobj->size);
>   	return 0;
>   
>   hmm_unreg:
> @@ -556,14 +563,99 @@ static int amdxdna_gem_obj_mmap(struct drm_gem_object *gobj,
>   	return ret;
>   }
>   
> +/*
> + * VM operations for amdxdna shmem VMAs that use VM_MIXEDMAP.
> + *
> + * drm_gem_shmem_vm_ops cannot be used on VM_MIXEDMAP VMAs because its fault
> + * handler calls vmf_insert_pfn() → vmf_insert_pfn_prot() which contains:
> + *   BUG_ON((vma->vm_flags & VM_MIXEDMAP) && pfn_valid(pfn))
> + * All amdxdna shmem pages are ordinary struct pages so pfn_valid() is always
> + * true, making the combination fatal.
> + *
> + * These ops use vmf_insert_page() (struct-page based) instead, which is the
> + * correct API for VM_MIXEDMAP VMAs backed by real struct pages.  The open and
> + * close handlers replicate drm_gem_shmem_vm_open/close using only exported
> + * symbols.
> + */
> +static vm_fault_t amdxdna_gem_mixedmap_fault(struct vm_fault *vmf)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct drm_gem_object *gobj = vma->vm_private_data;
> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
> +	loff_t num_pages = gobj->size >> PAGE_SHIFT;
> +	vm_fault_t ret = VM_FAULT_SIGBUS;
> +	pgoff_t page_offset;
> +	struct page *page;
> +
> +	/*
> +	 * Partial free of vma is unexpected. Otherwise, the wrong page
> +	 * will be faulted in and the user application may crash itself.
> +	 */
> +	page_offset = vmf->pgoff - vma->vm_pgoff;
> +
> +	dma_resv_lock(gobj->resv, NULL);
> +
> +	if (!shmem->pages || shmem->madv < 0 || page_offset >= num_pages)
> +		goto out;
> +
> +	page = shmem->pages[page_offset];
> +	if (WARN_ON_ONCE(!page))
> +		goto out;
> +
> +	/*
> +	 * Use vmf_insert_page() (struct-page path) not vmf_insert_pfn()
> +	 * (PFN path) because this VMA carries VM_MIXEDMAP.
> +	 */
> +	ret = vmf_insert_page(vma, vmf->address, page);
> +	if (ret == VM_FAULT_NOPAGE)
> +		folio_mark_accessed(page_folio(page));
> +
> +out:
> +	dma_resv_unlock(gobj->resv);
> +	return ret;
> +}
> +
> +static void amdxdna_gem_mixedmap_vm_open(struct vm_area_struct *vma)
> +{
> +	struct drm_gem_object *gobj = vma->vm_private_data;
> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
> +
> +	/*
> +	 * Bump pages_use_count so the page array stays alive for the new
> +	 * mapping copy created by fork().  Mirrors drm_gem_shmem_vm_open().
> +	 */
> +	dma_resv_lock(gobj->resv, NULL);
> +	drm_WARN_ON_ONCE(gobj->dev, !refcount_inc_not_zero(&shmem->pages_use_count));
> +	dma_resv_unlock(gobj->resv);
> +
> +	drm_gem_vm_open(vma);
> +}
> +
> +static void amdxdna_gem_mixedmap_vm_close(struct vm_area_struct *vma)
> +{
> +	struct drm_gem_object *gobj = vma->vm_private_data;
> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
> +
> +	dma_resv_lock(gobj->resv, NULL);
> +	drm_gem_shmem_put_pages_locked(shmem);
> +	dma_resv_unlock(gobj->resv);
> +
> +	drm_gem_vm_close(vma);
> +}
> +
> +static const struct vm_operations_struct amdxdna_gem_mixedmap_vm_ops = {
> +	.fault  = amdxdna_gem_mixedmap_fault,
> +	.open   = amdxdna_gem_mixedmap_vm_open,
> +	.close  = amdxdna_gem_mixedmap_vm_close,
> +};
> +
>   static int amdxdna_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>   {
>   	struct drm_gem_object *gobj = dma_buf->priv;
>   	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
> -	unsigned long num_pages = vma_pages(vma);
>   	int ret;
>   
> -	vma->vm_ops = &drm_gem_shmem_vm_ops;
> +	vma->vm_ops = &amdxdna_gem_mixedmap_vm_ops;
>   	vma->vm_private_data = gobj;
>   
>   	drm_gem_object_get(gobj);
> @@ -573,16 +665,9 @@ static int amdxdna_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struc
>   
>   	/* The buffer is based on memory pages. Fix the flag. */
>   	vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
> -	ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages,
> -			      &num_pages);
> -	if (ret)
> -		goto close_vma;
>   
>   	return 0;
>   
> -close_vma:
> -	vma->vm_ops->close(vma);
> -	return ret;
>   put_obj:
>   	drm_gem_object_put(gobj);
>   	return ret;
> @@ -878,7 +963,7 @@ static const struct drm_gem_object_funcs amdxdna_gem_shmem_funcs = {
>   	.vmap = amdxdna_gem_obj_vmap,
>   	.vunmap = amdxdna_gem_obj_vunmap,
>   	.mmap = amdxdna_gem_obj_mmap,
> -	.vm_ops = &drm_gem_shmem_vm_ops,
> +	.vm_ops = &amdxdna_gem_mixedmap_vm_ops,
>   	.export = amdxdna_gem_prime_export,
>   };
>   

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

[Lizhi Hou]

On 9/10/26 14:33, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Flawed VMA offset calculation in amdxdna_gem_mixedmap_fault causes data corruption on VMA splits and DMA-buf mmaps.
>
> Pre-existing issues:
> - [High] DMA-buf mmaps bypass HMM notifier registration, causing missing pre-faulting and unsafe page table teardown.
> --
>
> commit 5de79a60bd0ef403c4515e6d277ad3386e515ac9
> Author: Lizhi Hou <lizhi.hou@amd.com>
>
> accel/amdxdna: Fix unsafe use of handle_mm_fault()
>
> This commit removes handle_mm_fault() from the mmap callback because the
> VMA is not yet linked. For imported BOs, it marks the mapping invalid and
> relies on the first command submission to fault in the pages. For shmem
> BOs, it sets VM_MIXEDMAP and implements amdxdna_gem_mixed_vm_ops to handle
> the page faults.
>
>> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
>> index 3f05fa8811af1..f07f63a194537 100644
>> --- a/drivers/accel/amdxdna/amdxdna_gem.c
>> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> [ ... ]
>
>> +static vm_fault_t amdxdna_gem_mixedmap_fault(struct vm_fault *vmf)
>> +{
>> +	struct vm_area_struct *vma = vmf->vma;
>> +	struct drm_gem_object *gobj = vma->vm_private_data;
>> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
>> +	loff_t num_pages = gobj->size >> PAGE_SHIFT;
>> +	vm_fault_t ret = VM_FAULT_SIGBUS;
>> +	pgoff_t page_offset;
>> +	struct page *page;
>> +
>> +	/*
>> +	 * Partial free of vma is unexpected. Otherwise, the wrong page
>> +	 * will be faulted in and the user application may crash itself.
>> +	 */
>> +	page_offset = vmf->pgoff - vma->vm_pgoff;

This should not be a bug. When I merge, I will replace this line as 
other drivers do:

     page_offset = linear_page_delta(vma, vmf->address);


Lizhi

> [Severity: High]
> Will this calculation cause the wrong pages to be mapped on VMA splits or
> non-zero offset DMA-buf mmaps?
>
> Because the core MM computes vmf->pgoff as:
> vmf->pgoff = vma->vm_pgoff + ((vmf->address - vma->vm_start) >> PAGE_SHIFT)
>
> The subtraction here cancels out the VMA's logical offset, causing it to
> always yield the offset relative to the current VMA start. If a user mmaps a
> GEM object, unmaps the first half to split the VMA, and then triggers a page
> fault on the second half (e.g., after MADV_DONTNEED), could this erroneously
> map pages[0] into the middle of the object?
>
>> +
>> +	dma_resv_lock(gobj->resv, NULL);
> [ ... ]
>
>>   static int amdxdna_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>>   {
>>   	struct drm_gem_object *gobj = dma_buf->priv;
>>   	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
>> -	unsigned long num_pages = vma_pages(vma);
>>   	int ret;
> [Severity: High]
> This is a pre-existing issue, but does amdxdna_gem_dmabuf_mmap() need to
> call amdxdna_hmm_register() here?
>
> Without this call, the VMA is not added to the HMM tracking list. If a user
> exports a BO to a DMA-buf, mmaps the DMA-buf, and passes the virtual address
> to the NPU, the driver might fail to pre-fault the pages before execution.
>
> Additionally, if the user unmaps the VMA while the NPU is active, could the
> lack of an MMU interval notifier bypass the driver's teardown stall
> (dma_resv_wait_timeout()), allowing the CPU to tear down page tables while
> the NPU is still executing and causing IOMMU translation faults?
>

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

[Lizhi Hou]

Applied to drm-misc-next

On 9/10/26 15:39, Max Zhen wrote:
>
>
> On 9/10/2026 Thu 14:13, Lizhi Hou wrote:
>> 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 imported
>> BOs, mark the mapping as invalid and rely on the first command 
>> submission
>> to fault in the pages.
>>
>> For shmem BOs, set the VM_MIXEDMAP flag and use vm_insert_pages().
>> Implement amdxdna_gem_mixed_vm_ops to handle the page faults.
>>
>> Fixes: e486147c912f ("accel/amdxdna: Add BO import and export")
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> Reviewed-by: Max Zhen <max.zhen@amd.com>
>> ---
>> v2 & v3:
>>    Fix sashiko comment.
>>
>>   drivers/accel/amdxdna/amdxdna_gem.c | 151 ++++++++++++++++++++++------
>>   1 file changed, 118 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c 
>> b/drivers/accel/amdxdna/amdxdna_gem.c
>> index 0d165b66c1fc..a12a762b3a6b 100644
>> --- a/drivers/accel/amdxdna/amdxdna_gem.c
>> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/dma-direct.h>
>>   #include <linux/iosys-map.h>
>>   #include <linux/pagemap.h>
>> +#include <linux/swap.h>
>>   #include <linux/vmalloc.h>
>>     #include "amdxdna_cbuf.h"
>> @@ -490,16 +491,16 @@ 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;
>>       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 {
>> +    /*
>> +     * Until today there is not any use case to mmap with non-zero
>> +     * offset. Put an explicit check here.
>> +     */
>> +    if (vma->vm_pgoff - drm_vma_node_start(&to_gobj(abo)->vma_node))
>> +        return -EINVAL;
>> +
>> +    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 +509,28 @@ 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;
>>       }
>>   -    do {
>> -        vm_fault_t fault_ret;
>> -
>> -        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 = drm_gem_shmem_mmap(&abo->base, vma);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "Failed shmem mmap %d", ret);
>> +        return ret;
>> +    }
>>   -        offset += PAGE_SIZE;
>> -    } while (--num_pages);
>> +    vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
>> +    ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages, 
>> &num_pages);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "Failed to insert pages %d", ret);
>> +        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;
>> +    }
>>         return 0;
>>   }
>> @@ -536,6 +542,10 @@ static int amdxdna_gem_obj_mmap(struct 
>> drm_gem_object *gobj,
>>       struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
>>       int ret;
>>   +    XDNA_DBG(xdna, "BO map_offset 0x%llx type %d userptr 0x%lx 
>> size 0x%lx",
>> +         drm_vma_node_offset_addr(&gobj->vma_node), abo->type,
>> +         vma->vm_start, gobj->size);
>> +
>>       ret = amdxdna_hmm_register(abo, vma);
>>       if (ret)
>>           return ret;
>> @@ -546,9 +556,6 @@ static int amdxdna_gem_obj_mmap(struct 
>> drm_gem_object *gobj,
>>           goto hmm_unreg;
>>       }
>>   -    XDNA_DBG(xdna, "BO map_offset 0x%llx type %d userptr 0x%lx 
>> size 0x%lx",
>> -         drm_vma_node_offset_addr(&gobj->vma_node), abo->type,
>> -         vma->vm_start, gobj->size);
>>       return 0;
>>     hmm_unreg:
>> @@ -556,14 +563,99 @@ static int amdxdna_gem_obj_mmap(struct 
>> drm_gem_object *gobj,
>>       return ret;
>>   }
>>   +/*
>> + * VM operations for amdxdna shmem VMAs that use VM_MIXEDMAP.
>> + *
>> + * drm_gem_shmem_vm_ops cannot be used on VM_MIXEDMAP VMAs because 
>> its fault
>> + * handler calls vmf_insert_pfn() → vmf_insert_pfn_prot() which 
>> contains:
>> + *   BUG_ON((vma->vm_flags & VM_MIXEDMAP) && pfn_valid(pfn))
>> + * All amdxdna shmem pages are ordinary struct pages so pfn_valid() 
>> is always
>> + * true, making the combination fatal.
>> + *
>> + * These ops use vmf_insert_page() (struct-page based) instead, 
>> which is the
>> + * correct API for VM_MIXEDMAP VMAs backed by real struct pages.  
>> The open and
>> + * close handlers replicate drm_gem_shmem_vm_open/close using only 
>> exported
>> + * symbols.
>> + */
>> +static vm_fault_t amdxdna_gem_mixedmap_fault(struct vm_fault *vmf)
>> +{
>> +    struct vm_area_struct *vma = vmf->vma;
>> +    struct drm_gem_object *gobj = vma->vm_private_data;
>> +    struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
>> +    loff_t num_pages = gobj->size >> PAGE_SHIFT;
>> +    vm_fault_t ret = VM_FAULT_SIGBUS;
>> +    pgoff_t page_offset;
>> +    struct page *page;
>> +
>> +    /*
>> +     * Partial free of vma is unexpected. Otherwise, the wrong page
>> +     * will be faulted in and the user application may crash itself.
>> +     */
>> +    page_offset = vmf->pgoff - vma->vm_pgoff;
>> +
>> +    dma_resv_lock(gobj->resv, NULL);
>> +
>> +    if (!shmem->pages || shmem->madv < 0 || page_offset >= num_pages)
>> +        goto out;
>> +
>> +    page = shmem->pages[page_offset];
>> +    if (WARN_ON_ONCE(!page))
>> +        goto out;
>> +
>> +    /*
>> +     * Use vmf_insert_page() (struct-page path) not vmf_insert_pfn()
>> +     * (PFN path) because this VMA carries VM_MIXEDMAP.
>> +     */
>> +    ret = vmf_insert_page(vma, vmf->address, page);
>> +    if (ret == VM_FAULT_NOPAGE)
>> +        folio_mark_accessed(page_folio(page));
>> +
>> +out:
>> +    dma_resv_unlock(gobj->resv);
>> +    return ret;
>> +}
>> +
>> +static void amdxdna_gem_mixedmap_vm_open(struct vm_area_struct *vma)
>> +{
>> +    struct drm_gem_object *gobj = vma->vm_private_data;
>> +    struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
>> +
>> +    /*
>> +     * Bump pages_use_count so the page array stays alive for the new
>> +     * mapping copy created by fork().  Mirrors 
>> drm_gem_shmem_vm_open().
>> +     */
>> +    dma_resv_lock(gobj->resv, NULL);
>> +    drm_WARN_ON_ONCE(gobj->dev, 
>> !refcount_inc_not_zero(&shmem->pages_use_count));
>> +    dma_resv_unlock(gobj->resv);
>> +
>> +    drm_gem_vm_open(vma);
>> +}
>> +
>> +static void amdxdna_gem_mixedmap_vm_close(struct vm_area_struct *vma)
>> +{
>> +    struct drm_gem_object *gobj = vma->vm_private_data;
>> +    struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
>> +
>> +    dma_resv_lock(gobj->resv, NULL);
>> +    drm_gem_shmem_put_pages_locked(shmem);
>> +    dma_resv_unlock(gobj->resv);
>> +
>> +    drm_gem_vm_close(vma);
>> +}
>> +
>> +static const struct vm_operations_struct amdxdna_gem_mixedmap_vm_ops 
>> = {
>> +    .fault  = amdxdna_gem_mixedmap_fault,
>> +    .open   = amdxdna_gem_mixedmap_vm_open,
>> +    .close  = amdxdna_gem_mixedmap_vm_close,
>> +};
>> +
>>   static int amdxdna_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct 
>> vm_area_struct *vma)
>>   {
>>       struct drm_gem_object *gobj = dma_buf->priv;
>>       struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
>> -    unsigned long num_pages = vma_pages(vma);
>>       int ret;
>>   -    vma->vm_ops = &drm_gem_shmem_vm_ops;
>> +    vma->vm_ops = &amdxdna_gem_mixedmap_vm_ops;
>>       vma->vm_private_data = gobj;
>>         drm_gem_object_get(gobj);
>> @@ -573,16 +665,9 @@ static int amdxdna_gem_dmabuf_mmap(struct 
>> dma_buf *dma_buf, struct vm_area_struc
>>         /* The buffer is based on memory pages. Fix the flag. */
>>       vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
>> -    ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages,
>> -                  &num_pages);
>> -    if (ret)
>> -        goto close_vma;
>>         return 0;
>>   -close_vma:
>> -    vma->vm_ops->close(vma);
>> -    return ret;
>>   put_obj:
>>       drm_gem_object_put(gobj);
>>       return ret;
>> @@ -878,7 +963,7 @@ static const struct drm_gem_object_funcs 
>> amdxdna_gem_shmem_funcs = {
>>       .vmap = amdxdna_gem_obj_vmap,
>>       .vunmap = amdxdna_gem_obj_vunmap,
>>       .mmap = amdxdna_gem_obj_mmap,
>> -    .vm_ops = &drm_gem_shmem_vm_ops,
>> +    .vm_ops = &amdxdna_gem_mixedmap_vm_ops,
>>       .export = amdxdna_gem_prime_export,
>>   };
>