Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] Using `list_last_entry()` for error-path cleanup in `amdxdna_hmm_unregister()` introduces severe race conditions and out-of-bounds memory accesses. --
commit c58a67e26be2ca21d6b5952f0fd92febb2fa73f8 Author: Lizhi Hou <[email protected]> accel/amdxdna: Fix VMA access race This commit removes VMA accesses from aie2_populate_range() and amdxdna_umap_release() to fix a potential use-after-free where a saved VMA pointer may have already been freed. It also updates amdxdna_hmm_unregister() to unregister mappings based on a boolean flag instead of a VMA pointer. > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c [ ... ] > @@ -284,21 +284,26 @@ static const struct mmu_interval_notifier_ops > amdxdna_hmm_ops = { > .invalidate = amdxdna_hmm_invalidate, > }; > > -static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo, > - struct vm_area_struct *vma) > +static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo, bool > unreg_all) > { > struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); > struct amdxdna_umap *mapp; > > + if (!unreg_all) { > + down_read(&xdna->notifier_lock); > + mapp = list_last_entry(&abo->mem.umap_list, struct > amdxdna_umap, node); [Severity: Critical] Can this cause an out-of-bounds access if the list is empty? Looking at amdxdna_hmm_register(), it can return 0 without adding an entry to umap_list: amdxdna_hmm_register() { if (!amdxdna_pasid_on(abo->client)) { abo->mem.uva = addr; return 0; } ... } If amdxdna_insert_pages() subsequently fails in amdxdna_gem_obj_mmap(), the error path calls amdxdna_hmm_unregister(abo, false). Since the list is empty, would list_last_entry() evaluate to an invalid pointer offset, leading to memory corruption when setting mapp->unmapped = true below? Additionally, is there a race condition here with concurrent mappings? If multiple processes mmap the same GEM object concurrently, and one thread's amdxdna_insert_pages() fails, could it incorrectly retrieve and unregister the other thread's mapping from the end of the list? > + queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work); > + mapp->unmapped = true; > + up_read(&xdna->notifier_lock); > + > + return; > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
