Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Incomplete fix for skipping unmapped entries due to a TOCTOU race. Concurrent unmaps can still spuriously fail jobs. - [High] Denial of Service due to infinite invalidation retry loop bypassing absolute deadline. -- --- Patch [1]: [PATCH V1] accel/amdxdna: Skip unmapped range in aie2_populate_range() --- commit 1e3ba3d3372ac85262e7eb721d68d3f6974ff9b1 Author: Lizhi Hou <[email protected]> accel/amdxdna: Skip unmapped range in aie2_populate_range() This commit addresses an issue where jobs fail incorrectly for buffer objects with multiple memory mappings. It modifies aie2_populate_range() to skip entries that have already been unmapped, allowing the driver to successfully process the remaining valid mappings. > diff --git a/drivers/accel/amdxdna/aie2_ctx.c > b/drivers/accel/amdxdna/aie2_ctx.c > index 73f8001eef4f2..baf9a8b90a4d6 100644 > --- a/drivers/accel/amdxdna/aie2_ctx.c > +++ b/drivers/accel/amdxdna/aie2_ctx.c > @@ -1053,6 +1053,16 @@ static int aie2_populate_range(struct amdxdna_gem_obj > *abo) > found = false; > down_write(&xdna->notifier_lock); > list_for_each_entry(mapp, &abo->mem.umap_list, node) { > + /* > + * Skip entries that have already been unmapped. > + * > + * If userspace unmaps the address and later submits I/O using > + * it, the IOMMU will reject the access and report a fault. > + * Ignore such entries here. > + */ > + if (mapp->unmapped) > + continue; > + > if (mapp->invalid && kref_get_unless_zero(&mapp->refcnt)) { > found = true; > break; > } [Severity: High] This is a pre-existing regression, but this patch does not fully fix the problem due to a time-of-check to time-of-use race. Could a concurrent unmap cause amdxdna_hmm_invalidate() to set mapp->invalid to true before mapp->unmapped is updated? If aie2_populate_range() runs in drivers/accel/amdxdna/aie2_ctx.c after the invalidation but before mapp->unmapped is set to true, it will bypass this check and pick up the invalid VMA. It would then later fail when checking mmget_not_zero(): mm = mapp->notifier.mm; if (!mmget_not_zero(mm)) { amdxdna_umap_put(mapp); return -EFAULT; } Does this return -EFAULT and abort the entire job instead of gracefully skipping the unmapped entry? > } > > if (!found) { > + /* > + * This also covers the case where all mappings have been > + * removed. There are no invalid mappings left to process. > + * Any subsequent I/O using the unmapped address will be > + * rejected by the IOMMU. > + */ > abo->mem.map_invalid = false; > up_write(&xdna->notifier_lock); > return 0; [Severity: High] This is a pre-existing regression, but is it possible for this code to enter an infinite invalidation retry loop? Looking at aie2_populate_range() in drivers/accel/amdxdna/aie2_ctx.c: timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); again: found = false; [ ... ] if (mmu_interval_read_retry(&mapp->notifier, mapp->range.notifier_seq)) { up_write(&xdna->notifier_lock); amdxdna_umap_put(mapp); mmput(mm); goto again; } Because timeout is initialized before the again label, the relative timeout used by hmm_range_fault_unlocked_timeout() is continuously reset on each retry. If there is a continuous stream of memory invalidations, such as a userspace thread looping madvise(MADV_DONTNEED) on a mapping, could this trap the kernel thread in an infinite loop? Could this hold job_sem and cause a Denial of Service by preventing further job submissions? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
