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 <[email protected]> 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/[email protected]?part=1
