mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH V3] accel/amdxdna: Fix unsafe use of handle_mm_fault()
@ 2026-09-10 21:13 Lizhi Hou
  2026-09-10 22:39 ` Max Zhen
  0 siblings, 1 reply; 3+ messages in thread
From: Lizhi Hou @ 2026-09-10 21:13 UTC (permalink / raw)
  To: ogabbay, quic_jhugo, mario.limonciello, karol.wachowski,
	dri-devel, max.zhen
  Cc: Lizhi Hou, linux-kernel, sonal.santan

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V3] accel/amdxdna: Fix unsafe use of handle_mm_fault()
  2026-09-10 21:13 [PATCH V3] accel/amdxdna: Fix unsafe use of handle_mm_fault() Lizhi Hou
@ 2026-09-10 22:39 ` Max Zhen
  2026-09-11 15:25   ` Lizhi Hou
  0 siblings, 1 reply; 3+ messages in thread
From: Max Zhen @ 2026-09-10 22:39 UTC (permalink / raw)
  To: Lizhi Hou, ogabbay, quic_jhugo, mario.limonciello,
	karol.wachowski, dri-devel
  Cc: linux-kernel, sonal.santan



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,
>   };
>   


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V3] accel/amdxdna: Fix unsafe use of handle_mm_fault()
  2026-09-10 22:39 ` Max Zhen
@ 2026-09-11 15:25   ` Lizhi Hou
  0 siblings, 0 replies; 3+ messages in thread
From: Lizhi Hou @ 2026-09-11 15:25 UTC (permalink / raw)
  To: Max Zhen, ogabbay, quic_jhugo, mario.limonciello,
	karol.wachowski, dri-devel
  Cc: linux-kernel, sonal.santan

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,
>>   };
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-09-11 15:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-10 21:13 [PATCH V3] accel/amdxdna: Fix unsafe use of handle_mm_fault() Lizhi Hou
2026-09-10 22:39 ` Max Zhen
2026-09-11 15:25   ` Lizhi Hou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®