On Mon, 2026-06-08 at 17:35 +0200, Christian König wrote: > On 6/8/26 16:24, Philipp Stanner wrote: > > The dma_fence backend_ops can access a fence. Hereby, a driver callback > > will be running which likely will access driver specific data through > > container_of(). If now, simultaneously, a driver signals the fence and > > afterwards expects to run a driver specific destructor (using the same > > data accessed through container_of()), there can be a race. > > > > A driver very likely trusts that once it has signaled a fence, no one > > will be accessing it anymore. Moreover, it might already want to free up > > resources, making UAF bugs possible. > > > > The race occurs because there are only pragmatic checks for the signaled > > flag of a fence, without taking the fence lock. RCU guards exist, but > > their purpose is to guard accesses through the backend_ops callbacks > > against the driver (which implements the TEXT segment these callbacks > > live in) from unloading. > > > > Proper synchronization can be ensured by taking the fence lock. RCU is > > still simultaneously required to guard against the unload. > > > > Fix the races by taking the lock for all non-deprecated backend_ops > > callbacks. > > That sounds like the fundamentally wrong approach to me. > > The lock protects the dma_fence signaling state and *NOT* any driver > state, so it should not be used to protect any driver state. > > Drivers need to make sure that they protect their driver state with > separate lock and don't rely on the dma_fence lock for this. This is > actually the core of why we want to deprecate the shared dma_fence > spinlock.
It's not so much about protecting data, it's about correctness: A driver that calls dma_fence_signal(f) expects that after signalling, no callback will be running into the driver again. It's a fix synchronization point. Only the fence lock can grant such synchronization. Positive effects would be: 1. Drivers can do their cleanup immediately, without having to wait for a grace period 2. Drivers could be sure that their driver_fence data, allocated together with fence and accessed through container_of(fence), is not being accessed anymore. I see only advantages. Safer, faster. :) P. > > Regards, > Christian. > > > > > Conveniently, this also fixes a race where backend_ops->set_deadline() > > might try to set a deadline for an already signaled fence. > > > > Suggested-by: Danilo Krummrich <[email protected]> > > Signed-off-by: Philipp Stanner <[email protected]> > > --- > > We discovered this problem through our Rust abstractions, but it can > > also occur in C. > > > > The by far cleanest solution seems to be to use the fence lock. This RFC > > serves to discuss whether there is anything preventing that. > > > > (Patch so far just compile tested, to have some groundlayer for the > > rough idea, to discuss it first) > > --- > > drivers/dma-buf/dma-fence.c | 39 ++++++++++++++++++++++++++++--------- > > include/linux/dma-fence.h | 17 ++++++++++++---- > > 2 files changed, 43 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > index c7ea1e75d38a..b74f02f3cca8 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -629,7 +629,8 @@ EXPORT_SYMBOL(dma_fence_free); > > static bool __dma_fence_enable_signaling(struct dma_fence *fence) > > { > > const struct dma_fence_ops *ops; > > - bool was_set; > > + bool was_set, success; > > + unsigned long flags; > > > > dma_fence_assert_held(fence); > > > > @@ -644,7 +645,10 @@ static bool __dma_fence_enable_signaling(struct > > dma_fence *fence) > > if (!was_set && ops && ops->enable_signaling) { > > trace_dma_fence_enable_signal(fence); > > > > - if (!ops->enable_signaling(fence)) { > > + dma_fence_lock_irqsave(fence, flags); > > + success = ops->enable_signaling(fence); > > + dma_fence_unlock_irqrestore(fence, flags); > > + if (!success) { > > rcu_read_unlock(); > > dma_fence_signal_locked(fence); > > return false; > > @@ -1020,11 +1024,20 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout); > > void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline) > > { > > const struct dma_fence_ops *ops; > > + unsigned long flags; > > > > rcu_read_lock(); > > ops = rcu_dereference(fence->ops); > > - if (ops && ops->set_deadline && !dma_fence_is_signaled(fence)) > > + if (!ops || !ops->set_deadline) { > > + rcu_read_unlock(); > > + return; > > + } > > + > > + dma_fence_lock_irqsave(fence, flags); > > + if (!dma_fence_is_signaled_locked(fence)) > > ops->set_deadline(fence, deadline); > > + > > + dma_fence_unlock_irqrestore(fence, flags); > > rcu_read_unlock(); > > } > > EXPORT_SYMBOL(dma_fence_set_deadline); > > @@ -1166,14 +1179,18 @@ EXPORT_SYMBOL(dma_fence_init64); > > */ > > const char __rcu *dma_fence_driver_name(struct dma_fence *fence) > > { > > + const char __rcu *name = "detached-driver"; > > const struct dma_fence_ops *ops; > > + unsigned long flags; > > > > /* RCU protection is required for safe access to returned string */ > > ops = rcu_dereference(fence->ops); > > + dma_fence_lock_irqsave(fence, flags); > > if (!dma_fence_test_signaled_flag(fence)) > > - return (const char __rcu *)ops->get_driver_name(fence); > > - else > > - return (const char __rcu *)"detached-driver"; > > + name = ops->get_driver_name(fence); > > + dma_fence_unlock_irqrestore(fence, flags); > > + > > + return name; > > } > > EXPORT_SYMBOL(dma_fence_driver_name); > > > > @@ -1199,13 +1216,17 @@ EXPORT_SYMBOL(dma_fence_driver_name); > > */ > > const char __rcu *dma_fence_timeline_name(struct dma_fence *fence) > > { > > + const char __rcu *name = "signaled-timeline"; > > const struct dma_fence_ops *ops; > > + unsigned long flags; > > > > /* RCU protection is required for safe access to returned string */ > > ops = rcu_dereference(fence->ops); > > + dma_fence_lock_irqsave(fence, flags); > > if (!dma_fence_test_signaled_flag(fence)) > > - return (const char __rcu *)ops->get_driver_name(fence); > > - else > > - return (const char __rcu *)"signaled-timeline"; > > + name = ops->get_driver_name(fence); > > + dma_fence_unlock_irqrestore(fence, flags); > > + > > + return name; > > } > > EXPORT_SYMBOL(dma_fence_timeline_name); > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > > index b52ab692b22e..b93c3f7f69fb 100644 > > --- a/include/linux/dma-fence.h > > +++ b/include/linux/dma-fence.h > > @@ -547,20 +547,29 @@ static inline bool > > dma_fence_is_signaled(struct dma_fence *fence) > > { > > const struct dma_fence_ops *ops; > > + unsigned long flags; > > + bool signaled; > > > > if (dma_fence_test_signaled_flag(fence)) > > return true; > > > > rcu_read_lock(); > > ops = rcu_dereference(fence->ops); > > - if (ops && ops->signaled && ops->signaled(fence)) { > > + if (!ops || !ops->signaled) { > > rcu_read_unlock(); > > - dma_fence_signal(fence); > > - return true; > > + return false; > > } > > + > > + dma_fence_lock_irqsave(fence, flags); > > + signaled = ops->signaled(fence); > > + > > + if (signaled) > > + dma_fence_signal_locked(fence); > > + > > + dma_fence_unlock_irqrestore(fence, flags); > > rcu_read_unlock(); > > > > - return false; > > + return signaled; > > } > > > > /**
