Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] dma-buf: UAF race condition trap in ops->release deprecation doc - [High] dma-buf: strict memory layout constraint imposed by ops->release deprecation - [Low] dma-buf: misleading wait callback documentation for consumers --
commit ee4b5cb11d9ee9ea134098d9b200f3aba6ab22fe Author: Philipp Stanner <[email protected]> dma-buf/dma-fence: Mark two callbacks as deprecated This commit deprecates the dma_fence ops->release() and ops->wait() callbacks to facilitate driver-unload support. It adds documentation to guide users away from implementing these callbacks and suggests alternatives. > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index dd07d128adc3b..59af0159cd258 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h [ ... ] > @@ -236,6 +238,10 @@ struct dma_fence_ops { > * Implementing this callback prevents the fence from detaching after > * signaling and so it is necessary for the module providing the > * dma_fence_ops to stay loaded as long as the dma_fence exists. > + * > + * Deprecated for the reason mentioned above. No new users must be > + * implemented. Consumers of a fence can instead notify themselves by > + * registering a callback on the fence. > */ [Severity: Low] Is this advice to consumers accurate? Consumers typically call the frontend dma_fence_wait() function. If the producer drops the ops->wait callback, dma_fence_wait() automatically falls back to dma_fence_default_wait(), which handles registering dma_fence_default_wait_cb natively. It seems consumers do not need to change their code or manually register callbacks, but this documentation instructs them to reinvent the core's default wait logic. > signed long (*wait)(struct dma_fence *fence, > bool intr, signed long timeout); > @@ -243,6 +249,8 @@ struct dma_fence_ops { > /** > * @release: > * > + * DEPRECATED! > + * > * Called on destruction of fence to release additional resources. > * Can be called from irq context. This callback is optional. If it is > * NULL, then dma_fence_free() is instead called as the default [Severity: High] Does forbidding new implementations of ops->release() impose an unstated memory layout constraint? If a driver omits ops->release(), dma_fence_release() automatically falls back to dma_fence_free(), which calls kfree_rcu(fence, rcu). The kfree_rcu() function requires the exact base pointer returned by the allocator. Drivers commonly embed struct dma_fence inside a larger custom structure. If the fence is at a non-zero offset, passing its pointer to kfree_rcu() will attempt to free an interior pointer, which can corrupt the memory allocator. Could this default behavior be problematic without warning developers about this layout requirement? > @@ -254,6 +262,12 @@ struct dma_fence_ops { > * > * If the callback is implemented the memory backing the dma_fence > * object must be freed RCU safe. > + * > + * Deprecated because it prevents the producer of a fence from > + * unloading. No new users must be implemented. Parties with a > + * hypothetical need for this callback can instead simply and directly > + * perform their custom release operations one RCU grace period after > + * they have signaled the fence. > */ [Severity: High] Could this suggested cleanup pattern lead to a use-after-free? Signaling a fence does not guarantee all kref references are dropped, as consumers like sync_file or other drivers might still hold references. If a producer frees the containing structure asynchronously after signaling instead of waiting for the kref to reach zero, any consumer that subsequently calls dma_fence_put() will trigger dma_fence_release() on freed memory. Even if the structure is left to dma_fence_free(), tearing down associated sub-resources before kref reaches zero creates race conditions with consumers. > void (*release)(struct dma_fence *fence); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
