Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm_work_fence_cancel_sync() fails to synchronize with the dma-fence callback, allowing queue_work() to be called concurrently with driver teardown. - [High] The driver's ops->destroy() callback can unexpectedly be executed in hard IRQ context during the cancellation race. --
commit 781c5ea671786cbee47b9bed0159aa892cc5e2b7 Author: Srinivasan Shanmugam <[email protected]> drm: Add drm_work_fence helper This commit introduces drm_work_fence, a helper that queues a work item when a dma-fence signals. It manages dma-fence callbacks, kref lifetime, and workqueue dispatch. > diff --git a/drivers/gpu/drm/drm_work_fence.c > b/drivers/gpu/drm/drm_work_fence.c > new file mode 100644 > index 0000000000000..5e0cbc53edb80 > --- /dev/null > +++ b/drivers/gpu/drm/drm_work_fence.c [ ... ] > +static void drm_work_fence_destroy(struct kref *kref) > +{ > + struct drm_work_fence *wfence = > + container_of(kref, struct drm_work_fence, refcount); > + struct dma_fence *fence = wfence->fence; > + > + wfence->ops->destroy(wfence); [Severity: High] Can this ops->destroy() callback be executed in hard IRQ context? If a dma-fence callback races with driver teardown and queue_work() fails, the final reference might be dropped directly from the IRQ callback: drm_work_fence_cb() -> drm_work_fence_queue() If queue_work() returns false, drm_work_fence_put() drops the last reference here, triggering this destroy callback under the dma-fence spinlock with IRQs disabled. > + dma_fence_put(fence); /* NULL-safe */ > +} [ ... ] > +static void drm_work_fence_queue(struct drm_work_fence *wfence) > +{ > + if (!queue_work(wfence->wq, &wfence->work)) > + drm_work_fence_put(wfence); > +} [Severity: High] Is there a use-after-free risk if wfence->wq is accessed here during a concurrent driver teardown? This relies on drm_work_fence_cancel_sync() successfully synchronizing against the callback. However, if the synchronization fails, wfence->wq could be accessed after the workqueue has been destroyed by the caller. [ ... ] > +void drm_work_fence_cancel_sync(struct drm_work_fence *wfence) > +{ > + if (drm_work_fence_cancel(wfence)) > + return; > + if (disable_work_sync(&wfence->work)) > + drm_work_fence_put(wfence); > +} [Severity: High] Does this provide the expected synchronous cancellation guarantee, or does it leave a window for concurrent execution during teardown? If the dma-fence signals, the callback is dequeued but hasn't called queue_work() yet. If drm_work_fence_cancel_sync() is called concurrently: 1. drm_work_fence_cancel() calls dma_fence_remove_callback(), which returns false because the callback is already dequeued. 2. disable_work_sync() returns false immediately because the work is not yet queued. 3. The function returns, falsely indicating the work is fully canceled. This allows the driver teardown to proceed while the in-flight callback might then call queue_work() on a potentially destroyed workqueue. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
