Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Livelock in aie2_populate_range() retry loop due to lack of absolute timeout. - [High] Data race when concurrently populating shared BO mappings. -- --- Patch [3]: [PATCH v2 3/4] fixup! accel/amdxdna: use hmm_range_fault_unlocked_timeout() for range population --- commit 0ad43904da8ddad180e9ceac49d9b0988660e0ec Author: Stanislav Kinsburskii <[email protected]> fixup! accel/amdxdna: use hmm_range_fault_unlocked_timeout() for range population aie2_populate_range() now uses hmm_range_fault_unlocked_timeout() to let HMM own mmap_lock acquisition and retry handling while populating an invalid user mapping. The timeout passed to that helper is a relative HMM retry budget, not an absolute deadline. > diff --git a/drivers/accel/amdxdna/aie2_ctx.c > b/drivers/accel/amdxdna/aie2_ctx.c > index 548ba4315554f..21f2817751f9e 100644 > --- a/drivers/accel/amdxdna/aie2_ctx.c > +++ b/drivers/accel/amdxdna/aie2_ctx.c > @@ -1062,13 +1062,9 @@ static int aie2_populate_range(struct amdxdna_gem_obj > *abo) > return -EFAULT; > } > > - ret = hmm_range_fault_unlocked_timeout(&mapp->range, > - max_t(long, timeout - jiffies, 1)); > - if (ret) { > - if (ret == -EBUSY) > - ret = -ETIME; > + ret = hmm_range_fault_unlocked_timeout(&mapp->range, timeout); [Severity: High] This is a pre-existing issue, but is there a data race when concurrently populating shared BO mappings? In aie2_cmd_submit(), the job submission explicitly drops reservations and locks before calling aie2_populate_range(): drivers/accel/amdxdna/aie2_ctx.c:aie2_cmd_submit() { ... if (abo->mem.map_invalid) { up_read(&xdna->notifier_lock); drm_gem_unlock_reservations(job->bos, job->bo_cnt, &acquire_ctx); ... ret = aie2_populate_range(abo); ... } If two threads concurrently submit jobs using a shared buffer object, they could both see abo->mem.map_invalid as true, drop their locks, and simultaneously call aie2_populate_range() for the same BO. Could this cause concurrent uncoordinated modifications to the same mapp->range structure, causing a data race on range->notifier_seq and concurrent writes to the range->hmm_pfns array during the call to hmm_range_fault_unlocked_timeout() here? > + if (ret) > goto put_mm; > - } > > down_write(&xdna->notifier_lock); > if (mmu_interval_read_retry(&mapp->notifier, mapp->range.notifier_seq)) > { [Severity: High] This is also a pre-existing issue, but does this create a livelock if an unprivileged user repeatedly invalidates a memory range? The code executes a 'goto again' if mmu_interval_read_retry() returns true, passing a fresh timeout budget to hmm_range_fault_unlocked_timeout() on every iteration. Because there is no overarching absolute deadline within this loop, can the thread spin indefinitely here? The outer absolute timeout in aie2_cmd_submit() wouldn't provide protection because aie2_populate_range() never returns while spinning. [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/178406760622.1106335.2379450382728057793.stgit@skinsburskii?part=3
