Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Missing work cancellation for soft IH ring (and ih1/ih2) causes use-after-free and NULL pointer dereference on device teardown. -- commit b817b565b60c4bf0d6715436c240de40fe4ce255 Author: Timur Kristóf <[email protected]> drm/amdgpu: Use system unbound workqueue for soft IH ring This commit allows the kernel to dispatch the soft IH work on other CPUs. This prevents the soft IH ring from filling up before it starts processing work items during retry page fault storms. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > index 8112ffc85995e..2522285e1ab23 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > @@ -514,7 +514,7 @@ void amdgpu_irq_delegate(struct amdgpu_device *adev, > unsigned int num_dw) > { > amdgpu_ih_ring_write(adev, &adev->irq.ih_soft, entry->iv_entry, num_dw); > - schedule_work(&adev->irq.ih_soft_work); > + queue_work(system_unbound_wq, &adev->irq.ih_soft_work); [Severity: High] This isn't a bug introduced by this patch, but changing the workqueue to system_unbound_wq removes CPU-binding, which might increase the likelihood of the work running concurrently with the device teardown thread on SMP systems. During teardown, amdgpu_irq_fini_hw() frees the ring buffer and shadow pointers via amdgpu_ih_ring_fini() without flushing or cancelling the pending ih_soft_work: drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:amdgpu_irq_fini_hw() { ... free_irq(adev->irq.irq, adev); ... amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft); ... } If ih_soft_work (amdgpu_ih_process) was preempted right after evaluating that the device is not yet shutting down, and then resumes after the teardown path has freed the ring, could it dereference the freed ih->wptr_cpu and ih->ring? drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c:amdgpu_ih_process() { ... if (!ih->enabled || adev->shutdown) return -ENOENT; /* Preempted here, teardown frees the ring */ wptr = amdgpu_ih_get_wptr(adev, ih); ... } Should there be a cancel_work_sync(&adev->irq.ih_soft_work) or similar synchronization added during the teardown sequence before the ring buffers are freed? > } > > /** -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
