[PATCH V1] accel/amdxdna: Fix unexpected wait when flushing notifier_wq

[PATCH V1] accel/amdxdna: Fix unexpected wait when flushing notifier_wq

[Lizhi Hou]

In amdxdna_gem_obj_free(), flush_workqueue(xdna->notifier_wq) waits for
all pending work items on the device-global notifier workqueue, rather
than only the work items associated with the BO being freed.

If another BO has a pending hmm_unreg_work, freeing an unrelated BO can
be unnecessarily blocked until that work completes.

mmu_interval_notifier_remove() is deferred to a workqueue because it
cannot be called from the MMU notifier callback itself. The BO free path
is not a notifier callback, so call mmu_interval_notifier_remove()
directly there and avoid the workqueue.

Fixes: e486147c912f ("accel/amdxdna: Add BO import and export")
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 60 +++++++++++++++++++----------
 drivers/accel/amdxdna/amdxdna_gem.h |  1 +
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 1c63eff0a4a8..d6aa862e2d5b 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -301,33 +301,40 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
 
 	down_write(&xdna->notifier_lock);
 	list_for_each_entry(mapp, &abo->mem.umap_list, node) {
-		if (!vma || compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) {
-			if (!mapp->unmapped) {
-				queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
-				mapp->unmapped = true;
-			}
-			if (vma)
-				break;
-		}
+		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;
 	}
 	up_write(&xdna->notifier_lock);
 }
 
-static void amdxdna_umap_release(struct kref *ref)
+static void amdxdna_hmm_unregister_all(struct amdxdna_gem_obj *abo)
 {
-	struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt);
-	struct amdxdna_gem_obj *abo = mapp->abo;
-	struct amdxdna_dev *xdna;
-
-	mmu_interval_notifier_remove(&mapp->notifier);
+	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
+	struct amdxdna_umap *mapp, *tmp;
+	LIST_HEAD(dead);
 
-	xdna = to_xdna_dev(to_gobj(mapp->abo)->dev);
 	down_write(&xdna->notifier_lock);
-	list_del(&mapp->node);
-	if (list_empty(&abo->mem.umap_list))
-		abo->mem.uva = AMDXDNA_INVALID_ADDR;
+	list_for_each_entry_safe(mapp, tmp, &abo->mem.umap_list, node) {
+		mapp->unmapped = true;
+		mapp->cleanup = true;
+		list_move(&mapp->node, &dead);
+	}
 	up_write(&xdna->notifier_lock);
 
+	list_for_each_entry_safe(mapp, tmp, &dead, node) {
+		cancel_work_sync(&mapp->hmm_unreg_work);
+		amdxdna_umap_put(mapp);
+	}
+}
+
+static void amdxdna_umap_release(struct kref *ref)
+{
+	struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt);
+
+	mmu_interval_notifier_remove(&mapp->notifier);
 	kvfree(mapp->range.hmm_pfns);
 	kfree(mapp);
 }
@@ -341,6 +348,20 @@ 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;
+
+	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);
 }
@@ -643,8 +664,7 @@ 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(abo, NULL);
-	flush_workqueue(xdna->notifier_wq);
+	amdxdna_hmm_unregister_all(abo);
 
 	if (abo->pinned)
 		amdxdna_gem_unpin(abo);
diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
index 1e90e32bf3cd..fb033ced1045 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.h
+++ b/drivers/accel/amdxdna/amdxdna_gem.h
@@ -20,6 +20,7 @@ struct amdxdna_umap {
 	struct kref			refcnt;
 	bool				invalid;
 	bool				unmapped;
+	bool				cleanup;
 };
 
 struct amdxdna_mem {
-- 
2.34.1

Re: [PATCH V1] accel/amdxdna: Fix unexpected wait when flushing notifier_wq

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing flush_workqueue() with cancel_work_sync() inside a list iteration creates a race condition leading to a Use-After-Free of the GEM object (`abo`) when a work item has already unlinked itself.
--

--- Patch [1]: [PATCH V1] accel/amdxdna: Fix unexpected wait when flushing notifier_wq ---
commit 833f3279232d8856fda2a2788bbc7c5f9bd924f8
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix unexpected wait when flushing notifier_wq

This commit replaces flush_workqueue() in amdxdna_gem_obj_free() with a
new amdxdna_hmm_unregister_all() function. This avoids waiting for unrelated
work items on the global device workqueue by iterating through the BO's
umap_list and canceling the unregister works directly.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index b01f5d95fac83..a439e8c556465 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
[ ... ]
> @@ -301,33 +301,40 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
[ ... ]
> +static void amdxdna_hmm_unregister_all(struct amdxdna_gem_obj *abo)
> +{
> +	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
> +	struct amdxdna_umap *mapp, *tmp;
> +	LIST_HEAD(dead);
>  
> -	xdna = to_xdna_dev(to_gobj(mapp->abo)->dev);
>  	down_write(&xdna->notifier_lock);
> -	list_del(&mapp->node);
> -	if (list_empty(&abo->mem.umap_list))
> -		abo->mem.uva = AMDXDNA_INVALID_ADDR;
> +	list_for_each_entry_safe(mapp, tmp, &abo->mem.umap_list, node) {
> +		mapp->unmapped = true;
> +		mapp->cleanup = true;
> +		list_move(&mapp->node, &dead);
> +	}
>  	up_write(&xdna->notifier_lock);
>  
> +	list_for_each_entry_safe(mapp, tmp, &dead, node) {
> +		cancel_work_sync(&mapp->hmm_unreg_work);
> +		amdxdna_umap_put(mapp);
> +	}
> +}
> +
[ ... ]
> @@ -341,6 +348,20 @@ 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;
> +
> +	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);
>  }
[ ... ]
> @@ -643,8 +664,7 @@ 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(abo, NULL);
> -	flush_workqueue(xdna->notifier_wq);
> +	amdxdna_hmm_unregister_all(abo);

[Severity: High]
Does replacing flush_workqueue() with amdxdna_hmm_unregister_all() here open a
race condition leading to a use-after-free of the GEM object?

If the worker thread amdxdna_hmm_unreg_work() has already unlinked mapp from
abo->mem.umap_list via list_del(&mapp->node) and released the lock, but hasn't
completed amdxdna_umap_put(mapp):

Worker thread:
amdxdna_hmm_unreg_work()
	list_del(&mapp->node);
	up_write(&xdna->notifier_lock);
    <-- preempted here before amdxdna_umap_put(mapp) finishes unregistering

Free path:
amdxdna_gem_obj_free()
	amdxdna_hmm_unregister_all(abo)

Because mapp is no longer in abo->mem.umap_list, amdxdna_hmm_unregister_all()
will not find it and won't call cancel_work_sync(&mapp->hmm_unreg_work).
The amdxdna_gem_obj_free() function will then proceed to free the GEM object
(abo) without waiting for the work item to finish.

If an MMU event such as munmap triggers the notifier before the worker
finishes unregistering it, could it dereference the freed abo?

drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_hmm_invalidate() {
	struct amdxdna_umap *mapp = container_of(mni, struct amdxdna_umap, notifier);
	struct amdxdna_gem_obj *abo = mapp->abo;
	...
	xdna = to_xdna_dev(to_gobj(abo)->dev);
}

Is there a way to safely ensure any detached but still running work items are
fully completed before freeing the GEM object?

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

Re: [PATCH V1] accel/amdxdna: Fix unexpected wait when flushing notifier_wq

[Max Zhen]

On 8/12/2026 Wed 14:44, Lizhi Hou wrote:
> In amdxdna_gem_obj_free(), flush_workqueue(xdna->notifier_wq) waits for
> all pending work items on the device-global notifier workqueue, rather
> than only the work items associated with the BO being freed.
> 
> If another BO has a pending hmm_unreg_work, freeing an unrelated BO can
> be unnecessarily blocked until that work completes.
> 
> mmu_interval_notifier_remove() is deferred to a workqueue because it
> cannot be called from the MMU notifier callback itself. The BO free path
> is not a notifier callback, so call mmu_interval_notifier_remove()
> directly there and avoid the workqueue.
> 
> 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>
> ---
>   drivers/accel/amdxdna/amdxdna_gem.c | 60 +++++++++++++++++++----------
>   drivers/accel/amdxdna/amdxdna_gem.h |  1 +
>   2 files changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 1c63eff0a4a8..d6aa862e2d5b 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -301,33 +301,40 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
>   
>   	down_write(&xdna->notifier_lock);
>   	list_for_each_entry(mapp, &abo->mem.umap_list, node) {
> -		if (!vma || compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) {
> -			if (!mapp->unmapped) {
> -				queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
> -				mapp->unmapped = true;
> -			}
> -			if (vma)
> -				break;
> -		}
> +		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;
>   	}
>   	up_write(&xdna->notifier_lock);
>   }
>   
> -static void amdxdna_umap_release(struct kref *ref)
> +static void amdxdna_hmm_unregister_all(struct amdxdna_gem_obj *abo)
>   {
> -	struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt);
> -	struct amdxdna_gem_obj *abo = mapp->abo;
> -	struct amdxdna_dev *xdna;
> -
> -	mmu_interval_notifier_remove(&mapp->notifier);
> +	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
> +	struct amdxdna_umap *mapp, *tmp;
> +	LIST_HEAD(dead);
>   
> -	xdna = to_xdna_dev(to_gobj(mapp->abo)->dev);
>   	down_write(&xdna->notifier_lock);
> -	list_del(&mapp->node);
> -	if (list_empty(&abo->mem.umap_list))
> -		abo->mem.uva = AMDXDNA_INVALID_ADDR;
> +	list_for_each_entry_safe(mapp, tmp, &abo->mem.umap_list, node) {
> +		mapp->unmapped = true;
> +		mapp->cleanup = true;
> +		list_move(&mapp->node, &dead);
> +	}
>   	up_write(&xdna->notifier_lock);
>   
> +	list_for_each_entry_safe(mapp, tmp, &dead, node) {
> +		cancel_work_sync(&mapp->hmm_unreg_work);
> +		amdxdna_umap_put(mapp);
> +	}
> +}
> +
> +static void amdxdna_umap_release(struct kref *ref)
> +{
> +	struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt);
> +
> +	mmu_interval_notifier_remove(&mapp->notifier);
>   	kvfree(mapp->range.hmm_pfns);
>   	kfree(mapp);
>   }
> @@ -341,6 +348,20 @@ 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;
> +
> +	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);
>   }
> @@ -643,8 +664,7 @@ 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(abo, NULL);
> -	flush_workqueue(xdna->notifier_wq);
> +	amdxdna_hmm_unregister_all(abo);
>   
>   	if (abo->pinned)
>   		amdxdna_gem_unpin(abo);
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
> index 1e90e32bf3cd..fb033ced1045 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.h
> +++ b/drivers/accel/amdxdna/amdxdna_gem.h
> @@ -20,6 +20,7 @@ struct amdxdna_umap {
>   	struct kref			refcnt;
>   	bool				invalid;
>   	bool				unmapped;
> +	bool				cleanup;
>   };
>   
>   struct amdxdna_mem {