Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] amdxdna: hardware DMA into freed memory due to interruptible wait in hmm_invalidate --
commit c5bcbac3bd594f37cab09ce7ecaa29fea4a1e25a Author: David Zhang <[email protected]> accel/amdxdna: Move HMM invalidate wait into common GEM code This commit moves the HMM invalidate wait for user mappings into the common GEM code within amdxdna_hmm_invalidate(). Devices that don't attach fences to user BOs will return from the wait immediately. > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > index f4832337ec31d..eebd93b6f2c31 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c [ ... ] > @@ -249,8 +251,12 @@ static bool amdxdna_hmm_invalidate(struct > mmu_interval_notifier *mni, > mmu_interval_set_seq(&mapp->notifier, cur_seq); > up_write(&xdna->notifier_lock); > > - if (xdna->dev_info->ops->hmm_invalidate) > - xdna->dev_info->ops->hmm_invalidate(abo, cur_seq); > + ret = dma_resv_wait_timeout(to_gobj(abo)->resv, DMA_RESV_USAGE_BOOKKEEP, > + true, MAX_SCHEDULE_TIMEOUT); > + if (!ret) > + XDNA_ERR(xdna, "Failed to wait for bo, ret %ld", ret); > + else if (ret == -ERESTARTSYS) > + XDNA_DBG(xdna, "Wait for bo interrupted by signal"); > > if (range->event == MMU_NOTIFY_UNMAP) { > down_write(&xdna->notifier_lock); [Severity: High] Does this interruptible wait allow hardware to DMA into freed memory? amdxdna_hmm_invalidate() is called by the OS memory subsystem when unmapping or reclaiming memory. The driver is expected to wait for the hardware to finish accessing the pages before returning true. However, dma_resv_wait_timeout() is called with intr=true, which makes the wait interruptible by signals. If the process receives a signal, the wait aborts early and returns -ERESTARTSYS. The callback logs a debug message but then proceeds to return true, which tells the OS that the device is no longer accessing the memory. Since the hardware job is still executing when the OS frees the physical pages, can this lead to memory corruption when the hardware DMAs into those pages? Should this wait use intr=false to be uninterruptible in this context? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
