Re: [RFC PATCH] dma-fence: Fix races of fence callbacks versus destructors by locking

[Philipp Stanner]

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 <dakr@xxxxxxxxxx>
> > Signed-off-by: Philipp Stanner <phasta@xxxxxxxxxx>
> > ---
> > 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;
> >  }
> >  
> >  /**