[PATCH V1] accel/amdxdna: Fix race between unmap and free BO

[PATCH V1] accel/amdxdna: Fix race between unmap and free BO

[Lizhi Hou]

A userspace process can concurrently trigger an unmap, which queues
hmm_unreg_work, and free BO. If amdxdna_hmm_unreg_work() executes
list_del(&mapp->node) and is preempted before calling amdxdna_umap_put(),
amdxdna_hmm_unregister_all() can fail to find the mapping in the list
and return without calling cancel_work_sync(). This allows object
destruction to proceed while the worker still holds a pointer to the BO
in the umap structure.

Moving the work item from struct amdxdna_umap to struct amdxdna_gem_obj.
The worker no longer holds a pointer to an individual mapping, so there
is no longer a window where the umap has been unlinked from umap_list but
work is still pending against it. The work item now lives in the BO
itself, so use a single cancel_work_sync() in the free path to drain the
work.

Fixes: 445d20910429 ("accel/amdxdna: Fix unexpected wait when flushing notifier_wq")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 95 +++++++++++++----------------
 drivers/accel/amdxdna/amdxdna_gem.h |  3 +-
 2 files changed, 42 insertions(+), 56 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index e861db6f9369..398d0a58b53a 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -164,30 +164,6 @@ void amdxdna_gem_heap_free(struct amdxdna_client *client, struct amdxdna_gem_obj
 	mutex_unlock(&client->mm_lock);
 }
 
-static struct amdxdna_gem_obj *
-amdxdna_gem_create_obj(struct drm_device *dev, size_t size)
-{
-	struct amdxdna_gem_obj *abo;
-
-	abo = kzalloc_obj(*abo);
-	if (!abo)
-		return ERR_PTR(-ENOMEM);
-
-	abo->pinned = false;
-	abo->assigned_hwctx = AMDXDNA_INVALID_CTX_HANDLE;
-	mutex_init(&abo->lock);
-
-	abo->mem.dma_addr = AMDXDNA_INVALID_ADDR;
-	abo->mem.uva = AMDXDNA_INVALID_ADDR;
-	abo->mem.size = size;
-	abo->open_ref = 0;
-	abo->internal = false;
-	INIT_LIST_HEAD(&abo->mem.umap_list);
-	xa_init_flags(&abo->heap_xa, XA_FLAGS_ALLOC);
-
-	return abo;
-}
-
 static void
 amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo)
 {
@@ -278,10 +254,8 @@ static bool amdxdna_hmm_invalidate(struct mmu_interval_notifier *mni,
 
 	if (range->event == MMU_NOTIFY_UNMAP) {
 		down_write(&xdna->notifier_lock);
-		if (!mapp->unmapped) {
-			queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
-			mapp->unmapped = true;
-		}
+		mapp->unmapped = true;
+		queue_work(xdna->notifier_wq, &abo->hmm_unreg_work);
 		up_write(&xdna->notifier_lock);
 	}
 
@@ -311,13 +285,13 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
 		if (!compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end))
 			continue;
 
-		queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
 		mapp->unmapped = true;
+		queue_work(xdna->notifier_wq, &abo->hmm_unreg_work);
 	}
 	up_write(&xdna->notifier_lock);
 }
 
-static void amdxdna_hmm_unregister_all(struct amdxdna_gem_obj *abo)
+static void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force)
 {
 	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
 	struct amdxdna_umap *mapp, *tmp;
@@ -325,16 +299,18 @@ static void amdxdna_hmm_unregister_all(struct amdxdna_gem_obj *abo)
 
 	down_write(&xdna->notifier_lock);
 	list_for_each_entry_safe(mapp, tmp, &abo->mem.umap_list, node) {
+		if (!force && !mapp->unmapped)
+			continue;
+
 		mapp->unmapped = true;
-		mapp->cleanup = true;
 		list_move(&mapp->node, &dead);
 	}
+	if (list_empty(&abo->mem.umap_list))
+		abo->mem.uva = AMDXDNA_INVALID_ADDR;
 	up_write(&xdna->notifier_lock);
 
-	list_for_each_entry_safe(mapp, tmp, &dead, node) {
-		cancel_work_sync(&mapp->hmm_unreg_work);
+	list_for_each_entry_safe(mapp, tmp, &dead, node)
 		amdxdna_umap_put(mapp);
-	}
 }
 
 static void amdxdna_umap_release(struct kref *ref)
@@ -353,24 +329,10 @@ void amdxdna_umap_put(struct amdxdna_umap *mapp)
 
 static void amdxdna_hmm_unreg_work(struct work_struct *work)
 {
-	struct amdxdna_umap *mapp = container_of(work, struct amdxdna_umap,
-						 hmm_unreg_work);
-	struct amdxdna_gem_obj *abo = mapp->abo;
-	struct amdxdna_dev *xdna;
+	struct amdxdna_gem_obj *abo = container_of(work, struct amdxdna_gem_obj,
+						   hmm_unreg_work);
 
-	xdna = to_xdna_dev(to_gobj(mapp->abo)->dev);
-	down_write(&xdna->notifier_lock);
-	if (mapp->cleanup) {
-		up_write(&xdna->notifier_lock);
-		return;
-	}
-
-	list_del(&mapp->node);
-	if (list_empty(&abo->mem.umap_list))
-		abo->mem.uva = AMDXDNA_INVALID_ADDR;
-	up_write(&xdna->notifier_lock);
-
-	amdxdna_umap_put(mapp);
+	amdxdna_hmm_unreg_umaps(abo, false);
 }
 
 static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
@@ -422,8 +384,6 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
 	mapp->abo = abo;
 	kref_init(&mapp->refcnt);
 
-	INIT_WORK(&mapp->hmm_unreg_work, amdxdna_hmm_unreg_work);
-
 	ret = mmu_interval_notifier_insert_locked(&mapp->notifier,
 						  current->mm,
 						  addr,
@@ -449,6 +409,31 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
 	return ret;
 }
 
+static struct amdxdna_gem_obj *
+amdxdna_gem_create_obj(struct drm_device *dev, size_t size)
+{
+	struct amdxdna_gem_obj *abo;
+
+	abo = kzalloc_obj(*abo);
+	if (!abo)
+		return ERR_PTR(-ENOMEM);
+
+	abo->pinned = false;
+	abo->assigned_hwctx = AMDXDNA_INVALID_CTX_HANDLE;
+	mutex_init(&abo->lock);
+
+	abo->mem.dma_addr = AMDXDNA_INVALID_ADDR;
+	abo->mem.uva = AMDXDNA_INVALID_ADDR;
+	abo->mem.size = size;
+	abo->open_ref = 0;
+	abo->internal = false;
+	INIT_LIST_HEAD(&abo->mem.umap_list);
+	xa_init_flags(&abo->heap_xa, XA_FLAGS_ALLOC);
+	INIT_WORK(&abo->hmm_unreg_work, amdxdna_hmm_unreg_work);
+
+	return abo;
+}
+
 static void amdxdna_gem_dev_obj_free(struct drm_gem_object *gobj)
 {
 	struct amdxdna_dev *xdna = to_xdna_dev(gobj->dev);
@@ -756,7 +741,9 @@ static void amdxdna_gem_obj_free(struct drm_gem_object *gobj)
 	struct amdxdna_dev *xdna = to_xdna_dev(gobj->dev);
 	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
 
-	amdxdna_hmm_unregister_all(abo);
+	/* No notifier survives this, so no new work can be queued. */
+	amdxdna_hmm_unreg_umaps(abo, true);
+	cancel_work_sync(&abo->hmm_unreg_work);
 
 	if (abo->pinned)
 		amdxdna_gem_unpin(abo);
diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
index 5dfefdcf1356..9b4aa21a37c9 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.h
+++ b/drivers/accel/amdxdna/amdxdna_gem.h
@@ -14,13 +14,11 @@
 struct amdxdna_umap {
 	struct mmu_interval_notifier	notifier;
 	struct hmm_range		range;
-	struct work_struct		hmm_unreg_work;
 	struct amdxdna_gem_obj		*abo;
 	struct list_head		node;
 	struct kref			refcnt;
 	bool				invalid;
 	bool				unmapped;
-	bool				cleanup;
 };
 
 struct amdxdna_mem {
@@ -44,6 +42,7 @@ struct amdxdna_gem_obj {
 	struct mutex			lock; /* Protects: pinned, mem.kva, open_ref */
 	struct amdxdna_mem		mem;
 	int				open_ref;
+	struct work_struct		hmm_unreg_work;
 
 	/* Below members are initialized when needed */
 	struct drm_mm_node		mm_node; /* For AMDXDNA_BO_DEV */
-- 
2.34.1

Re: [PATCH V1] accel/amdxdna: Fix race between unmap and free BO

[Max Zhen]

On 9/17/2026 Thu 08:48, Lizhi Hou wrote:
> A userspace process can concurrently trigger an unmap, which queues
> hmm_unreg_work, and free BO. If amdxdna_hmm_unreg_work() executes
> list_del(&mapp->node) and is preempted before calling amdxdna_umap_put(),
> amdxdna_hmm_unregister_all() can fail to find the mapping in the list
> and return without calling cancel_work_sync(). This allows object
> destruction to proceed while the worker still holds a pointer to the BO
> in the umap structure.
> 
> Moving the work item from struct amdxdna_umap to struct amdxdna_gem_obj.
> The worker no longer holds a pointer to an individual mapping, so there
> is no longer a window where the umap has been unlinked from umap_list but
> work is still pending against it. The work item now lives in the BO
> itself, so use a single cancel_work_sync() in the free path to drain the
> work.
> 
> Fixes: 445d20910429 ("accel/amdxdna: Fix unexpected wait when flushing notifier_wq")
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Reviewed-by: Max Zhen <max.zhen@amd.com>
> ---
>   drivers/accel/amdxdna/amdxdna_gem.c | 95 +++++++++++++----------------
>   drivers/accel/amdxdna/amdxdna_gem.h |  3 +-
>   2 files changed, 42 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index e861db6f9369..398d0a58b53a 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -164,30 +164,6 @@ void amdxdna_gem_heap_free(struct amdxdna_client *client, struct amdxdna_gem_obj
>   	mutex_unlock(&client->mm_lock);
>   }
>   
> -static struct amdxdna_gem_obj *
> -amdxdna_gem_create_obj(struct drm_device *dev, size_t size)
> -{
> -	struct amdxdna_gem_obj *abo;
> -
> -	abo = kzalloc_obj(*abo);
> -	if (!abo)
> -		return ERR_PTR(-ENOMEM);
> -
> -	abo->pinned = false;
> -	abo->assigned_hwctx = AMDXDNA_INVALID_CTX_HANDLE;
> -	mutex_init(&abo->lock);
> -
> -	abo->mem.dma_addr = AMDXDNA_INVALID_ADDR;
> -	abo->mem.uva = AMDXDNA_INVALID_ADDR;
> -	abo->mem.size = size;
> -	abo->open_ref = 0;
> -	abo->internal = false;
> -	INIT_LIST_HEAD(&abo->mem.umap_list);
> -	xa_init_flags(&abo->heap_xa, XA_FLAGS_ALLOC);
> -
> -	return abo;
> -}
> -
>   static void
>   amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo)
>   {
> @@ -278,10 +254,8 @@ static bool amdxdna_hmm_invalidate(struct mmu_interval_notifier *mni,
>   
>   	if (range->event == MMU_NOTIFY_UNMAP) {
>   		down_write(&xdna->notifier_lock);
> -		if (!mapp->unmapped) {
> -			queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
> -			mapp->unmapped = true;
> -		}
> +		mapp->unmapped = true;
> +		queue_work(xdna->notifier_wq, &abo->hmm_unreg_work);
>   		up_write(&xdna->notifier_lock);
>   	}
>   
> @@ -311,13 +285,13 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
>   		if (!compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end))
>   			continue;
>   
> -		queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
>   		mapp->unmapped = true;
> +		queue_work(xdna->notifier_wq, &abo->hmm_unreg_work);
>   	}
>   	up_write(&xdna->notifier_lock);
>   }
>   
> -static void amdxdna_hmm_unregister_all(struct amdxdna_gem_obj *abo)
> +static void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force)
>   {
>   	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
>   	struct amdxdna_umap *mapp, *tmp;
> @@ -325,16 +299,18 @@ static void amdxdna_hmm_unregister_all(struct amdxdna_gem_obj *abo)
>   
>   	down_write(&xdna->notifier_lock);
>   	list_for_each_entry_safe(mapp, tmp, &abo->mem.umap_list, node) {
> +		if (!force && !mapp->unmapped)
> +			continue;
> +
>   		mapp->unmapped = true;
> -		mapp->cleanup = true;
>   		list_move(&mapp->node, &dead);
>   	}
> +	if (list_empty(&abo->mem.umap_list))
> +		abo->mem.uva = AMDXDNA_INVALID_ADDR;
>   	up_write(&xdna->notifier_lock);
>   
> -	list_for_each_entry_safe(mapp, tmp, &dead, node) {
> -		cancel_work_sync(&mapp->hmm_unreg_work);
> +	list_for_each_entry_safe(mapp, tmp, &dead, node)
>   		amdxdna_umap_put(mapp);
> -	}
>   }
>   
>   static void amdxdna_umap_release(struct kref *ref)
> @@ -353,24 +329,10 @@ void amdxdna_umap_put(struct amdxdna_umap *mapp)
>   
>   static void amdxdna_hmm_unreg_work(struct work_struct *work)
>   {
> -	struct amdxdna_umap *mapp = container_of(work, struct amdxdna_umap,
> -						 hmm_unreg_work);
> -	struct amdxdna_gem_obj *abo = mapp->abo;
> -	struct amdxdna_dev *xdna;
> +	struct amdxdna_gem_obj *abo = container_of(work, struct amdxdna_gem_obj,
> +						   hmm_unreg_work);
>   
> -	xdna = to_xdna_dev(to_gobj(mapp->abo)->dev);
> -	down_write(&xdna->notifier_lock);
> -	if (mapp->cleanup) {
> -		up_write(&xdna->notifier_lock);
> -		return;
> -	}
> -
> -	list_del(&mapp->node);
> -	if (list_empty(&abo->mem.umap_list))
> -		abo->mem.uva = AMDXDNA_INVALID_ADDR;
> -	up_write(&xdna->notifier_lock);
> -
> -	amdxdna_umap_put(mapp);
> +	amdxdna_hmm_unreg_umaps(abo, false);
>   }
>   
>   static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
> @@ -422,8 +384,6 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
>   	mapp->abo = abo;
>   	kref_init(&mapp->refcnt);
>   
> -	INIT_WORK(&mapp->hmm_unreg_work, amdxdna_hmm_unreg_work);
> -
>   	ret = mmu_interval_notifier_insert_locked(&mapp->notifier,
>   						  current->mm,
>   						  addr,
> @@ -449,6 +409,31 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
>   	return ret;
>   }
>   
> +static struct amdxdna_gem_obj *
> +amdxdna_gem_create_obj(struct drm_device *dev, size_t size)
> +{
> +	struct amdxdna_gem_obj *abo;
> +
> +	abo = kzalloc_obj(*abo);
> +	if (!abo)
> +		return ERR_PTR(-ENOMEM);
> +
> +	abo->pinned = false;
> +	abo->assigned_hwctx = AMDXDNA_INVALID_CTX_HANDLE;
> +	mutex_init(&abo->lock);
> +
> +	abo->mem.dma_addr = AMDXDNA_INVALID_ADDR;
> +	abo->mem.uva = AMDXDNA_INVALID_ADDR;
> +	abo->mem.size = size;
> +	abo->open_ref = 0;
> +	abo->internal = false;
> +	INIT_LIST_HEAD(&abo->mem.umap_list);
> +	xa_init_flags(&abo->heap_xa, XA_FLAGS_ALLOC);
> +	INIT_WORK(&abo->hmm_unreg_work, amdxdna_hmm_unreg_work);
> +
> +	return abo;
> +}
> +
>   static void amdxdna_gem_dev_obj_free(struct drm_gem_object *gobj)
>   {
>   	struct amdxdna_dev *xdna = to_xdna_dev(gobj->dev);
> @@ -756,7 +741,9 @@ static void amdxdna_gem_obj_free(struct drm_gem_object *gobj)
>   	struct amdxdna_dev *xdna = to_xdna_dev(gobj->dev);
>   	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
>   
> -	amdxdna_hmm_unregister_all(abo);
> +	/* No notifier survives this, so no new work can be queued. */
> +	amdxdna_hmm_unreg_umaps(abo, true);
> +	cancel_work_sync(&abo->hmm_unreg_work);
>   
>   	if (abo->pinned)
>   		amdxdna_gem_unpin(abo);
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
> index 5dfefdcf1356..9b4aa21a37c9 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.h
> +++ b/drivers/accel/amdxdna/amdxdna_gem.h
> @@ -14,13 +14,11 @@
>   struct amdxdna_umap {
>   	struct mmu_interval_notifier	notifier;
>   	struct hmm_range		range;
> -	struct work_struct		hmm_unreg_work;
>   	struct amdxdna_gem_obj		*abo;
>   	struct list_head		node;
>   	struct kref			refcnt;
>   	bool				invalid;
>   	bool				unmapped;
> -	bool				cleanup;
>   };
>   
>   struct amdxdna_mem {
> @@ -44,6 +42,7 @@ struct amdxdna_gem_obj {
>   	struct mutex			lock; /* Protects: pinned, mem.kva, open_ref */
>   	struct amdxdna_mem		mem;
>   	int				open_ref;
> +	struct work_struct		hmm_unreg_work;
>   
>   	/* Below members are initialized when needed */
>   	struct drm_mm_node		mm_node; /* For AMDXDNA_BO_DEV */