Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Lack of a teardown/cancellation API allows Use-After-Free of driver resources and module memory if a fence signals after context teardown. - [Medium] Deferred dynamic initialization of `work_struct` leads to uninitialized memory access on teardown, data races, and breaks lockdep tracking. --
commit 4ecbf619a439e885246ba05ae0272714c1c83eb8 Author: Srinivasan Shanmugam <[email protected]> drm: Add common drm_user_fence helper Introduce a common DRM user fence helper providing the kref-managed, MM-borrowing dma-fence-callback-to-workqueue pattern used by drivers that must access userspace memory from a kthread context when a GPU fence signals. > diff --git a/drivers/gpu/drm/drm_user_fence.c > b/drivers/gpu/drm/drm_user_fence.c > index 0000000000000..bd76e3d03120e 100644 > --- /dev/null > +++ b/drivers/gpu/drm/drm_user_fence.c [ ... ] > +static void drm_user_fence_work(struct work_struct *w) > +{ > + struct drm_user_fence *ufence = > + container_of(w, struct drm_user_fence, work); > + bool mm_ok = false; > + > + if (mmget_not_zero(ufence->mm)) { > + kthread_use_mm(ufence->mm); > + mm_ok = true; > + } > + > + ufence->ops->worker(ufence, mm_ok); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] If the driver module has been unloaded after tearing down its context, will this jump into unmapped driver module text and cause a kernel panic? This can happen if a foreign dma-fence signals after driver teardown, as there is no way to safely detach or cancel the user fence. > + > + if (mm_ok) { > + kthread_unuse_mm(ufence->mm); > + mmput(ufence->mm); > + } > + > + drm_user_fence_put(ufence); > +} > + > +static void drm_user_fence_cb(struct dma_fence *fence, struct dma_fence_cb > *cb) > +{ > + struct drm_user_fence *ufence = > + container_of(cb, struct drm_user_fence, cb); > + > + INIT_WORK(&ufence->work, drm_user_fence_work); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Medium] Will calling INIT_WORK() dynamically here instead of during object initialization hide the work_struct state from teardown paths and break lockdep tracking? The work_struct remains completely uninitialized between the call to drm_user_fence_init() and the fence signaling, preventing the use of standard workqueue teardown functions prior to signaling. > + queue_work(ufence->wq, &ufence->work); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Could this dereference a freed workqueue if a foreign dma-fence signals after the driver has torn down its context? Since drm_user_fence_add_callback() takes a kref on the ufence and there is no cancellation API, the ufence remains alive on the foreign dma-fence's callback list even after the driver device is unbound or the module is unloaded. > +} > + > +/** > + * drm_user_fence_init - Initialize a user fence > + * @ufence: user fence to initialize > + * @wq: workqueue to run the worker on (must be ordered if sequencing > matters) > + * @ops: driver operations > + * > + * Must be called from process context. Grabs a reference to current->mm. > + */ > +void drm_user_fence_init(struct drm_user_fence *ufence, > + struct workqueue_struct *wq, > + const struct drm_user_fence_ops *ops) > +{ > + kref_init(&ufence->refcount); > + ufence->mm = current->mm; > + mmgrab(ufence->mm); > + ufence->wq = wq; > + ufence->ops = ops; > +} [Severity: Medium] Should INIT_WORK() be called here instead? Initializing the work_struct here would prevent lockdep class divergence and ensure the work_struct is in a valid state for teardown functions. > +EXPORT_SYMBOL_GPL(drm_user_fence_init); > + > +/** > + * drm_user_fence_add_callback - Attach a user fence to a dma-fence > + * @ufence: user fence > + * @fence: dma-fence to watch; caller retains ownership of this reference > + * > + * When @fence signals, a work item is queued that calls ops->worker() with > + * the process MM active. If @fence has already signaled the work item is > + * queued immediately. > + * > + * Return: 0 on success, negative errno on error. > + */ > +int drm_user_fence_add_callback(struct drm_user_fence *ufence, > + struct dma_fence *fence) > +{ > + int err; > + > + drm_user_fence_get(ufence); > + err = dma_fence_add_callback(fence, &ufence->cb, drm_user_fence_cb); > + if (err == -ENOENT) { > + /* fence already signaled — queue work immediately */ > + INIT_WORK(&ufence->work, drm_user_fence_work); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Medium] Does calling INIT_WORK() dynamically in two different locations cause lockdep class divergence? Since INIT_WORK() allocates a static lock class key at the call site, the work_struct will receive different lockdep keys depending on whether the fence was already signaled or not. > + queue_work(ufence->wq, &ufence->work); > + err = 0; > + } else if (err) { > + drm_user_fence_put(ufence); > + } > + > + return err; > +} > +EXPORT_SYMBOL_GPL(drm_user_fence_add_callback); > diff --git a/include/drm/drm_user_fence.h b/include/drm/drm_user_fence.h > index 0000000000000..de8e3f47be186 100644 > --- /dev/null > +++ b/include/drm/drm_user_fence.h [ ... ] > +/** > + * struct drm_user_fence - embeddable DRM user fence > + * > + * Drivers embed this in their own structure and implement > + * &drm_user_fence_ops. Call drm_user_fence_init() at creation and > + * drm_user_fence_add_callback() to arm on a dma-fence. > + */ > +struct drm_user_fence { > + /** @refcount: Reference count. */ > + struct kref refcount; > + /** @mm: Process MM grabbed at init time. */ > + struct mm_struct *mm; > + /** @work: Work item queued when the dma-fence signals. */ > + struct work_struct work; > + /** @cb: dma-fence callback. */ > + struct dma_fence_cb cb; > + /** @wq: Workqueue to run @work on. */ > + struct workqueue_struct *wq; > + /** @ops: Driver operations. */ > + const struct drm_user_fence_ops *ops; > +}; [Severity: High] Does this API need a cancellation mechanism (e.g., drm_user_fence_cancel())? Without a way to safely detach the ufence from a foreign dma-fence's callback list, the ufence remains alive and bound to driver resources even after the driver tears down its context, which can lead to a use-after-free. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
