Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Premature clearing of `job_timeout_detected` flag before flushing `context_abort_work` silently drops the ban on the guilty context during a job timeout (TDR). Pre-existing issues: - [High] Lockless `xa_load` of `file_priv` in the MMU event queue IRQ handler leads to a Use-After-Free. -- commit b8245a180aec634640c11be76ef2b20b2a900039 Author: Jakub Pawlak <[email protected]> accel/ivpu: Use separate flag for job timeout This patch introduces a separate flag, job_timeout_detected, to distinguish engine resets caused by job timeouts from other reasons. The flag is cleared in ivpu_prepare_for_reset() to ensure a clean state after recovery. > diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c > index 0f215392d42de..647684219e9d3 100644 > --- a/drivers/accel/ivpu/ivpu_drv.c > +++ b/drivers/accel/ivpu/ivpu_drv.c > @@ -528,6 +528,7 @@ void ivpu_prepare_for_reset(struct ivpu_device *vdev) > { > ivpu_hw_irq_disable(vdev); > disable_irq(vdev->irq); > + atomic_set(&vdev->job_timeout_detected, 0); > flush_work(&vdev->irq_dct_work); > flush_work(&vdev->context_abort_work); [Severity: High] Is it safe to clear the job_timeout_detected flag before flushing context_abort_work? If a concurrent recovery calls ivpu_prepare_for_reset() and sets the flag to 0, and context_abort_work runs simultaneously, could it reach the atomic_cmpxchg() in reset_engine_and_mark_faulty_contexts(), see a 0, and silently skip banning the guilty context? Should this flag be cleared after flushing the work queues instead? [ ... ] > diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c > index b3de5dd29d1e8..084d825f744dd 100644 > --- a/drivers/accel/ivpu/ivpu_job.c > +++ b/drivers/accel/ivpu/ivpu_job.c [ ... ] > @@ -1260,10 +1259,10 @@ static int > reset_engine_and_mark_faulty_contexts(struct ivpu_device *vdev) > return ret; > > /* > - * If faults are detected, ignore guilty contexts from engine reset as > NPU may not be stuck > - * and could return currently running good context and faulty contexts > are already marked > + * If job timeout is detected, read guilty context from engine reset, > for other reasons > + * faulty context is already known > */ > - if (atomic_cmpxchg(&vdev->faults_detected, 1, 0) == 1) > + if (atomic_cmpxchg(&vdev->job_timeout_detected, 1, 0) == 0) > return 0; > > num_impacted_contexts = > resp.payload.engine_reset_done.num_impacted_contexts; [ ... ] > diff --git a/drivers/accel/ivpu/ivpu_mmu.c b/drivers/accel/ivpu/ivpu_mmu.c > index 41efd8985fa67..b2025274f91de 100644 > --- a/drivers/accel/ivpu/ivpu_mmu.c > +++ b/drivers/accel/ivpu/ivpu_mmu.c > @@ -964,7 +964,6 @@ void ivpu_mmu_irq_evtq_handler(struct ivpu_device *vdev) > file_priv = xa_load(&vdev->context_xa, ssid); > if (file_priv) { > if (!READ_ONCE(file_priv->has_mmu_faults)) { > - atomic_set(&vdev->faults_detected, 1); > ivpu_mmu_dump_event(vdev, event); > WRITE_ONCE(file_priv->has_mmu_faults, true); > } [Severity: High] This is a pre-existing issue, but does the lockless xa_load() here risk a use-after-free? Looking at file_priv_release() in drivers/accel/ivpu/ivpu_drv.c, file_priv is freed immediately via kfree() without waiting for an RCU grace period after being removed from the xarray: drivers/accel/ivpu/ivpu_drv.c:file_priv_release() { ... ivpu_user_limits_put(vdev, file_priv->user_limits); mutex_destroy(&file_priv->ms_lock); mutex_destroy(&file_priv->lock); kfree(file_priv); } If a user closes the file descriptor concurrently with this IRQ handler, could file_priv be freed while we are still accessing file_priv->has_mmu_faults via READ_ONCE() or WRITE_ONCE()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
