Re: [PATCH 04/15] dma-buf: detach fence ops on signal

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Oct 16, 2025 at 04:57:37PM +0100, Tvrtko Ursulin wrote:
> 
> On 16/10/2025 09:56, Tvrtko Ursulin wrote:
> > 
> > On 13/10/2025 14:48, Christian König wrote:
> > > When neither a release nor a wait operation is specified it is possible
> > > to let the dma_fence live on independent of the module who issued it.
> > > 
> > > This makes it possible to unload drivers and only wait for all their
> > > fences to signal.
> > 
> > Have you looked at whether the requirement to not have the release and
> > wait callbacks will exclude some drivers from being able to benefit from
> > this?
> 
> I had a browse and this seems to be the situation:
> 
> Custom .wait:
>  - radeon, qxl, nouveau, i915
> 
> Those would therefore still be vulnerable to the unbind->unload sequence.
> Actually not sure about qxl, but other three are PCI so in theory at least.
> I915 at least supports unbind and unload.
> 
> Custom .release:
>  - vgem, nouveau, lima, pvr, i915, usb-gadget, industrialio, etnaviv, xe
> 
> Out of those there do not actually need a custom release and could probably
> be weaned off it:
>  - usb-gadget, industrialio, etnaviv, xe
> 
> (Xe would lose a debug assert and some would have their kfrees replaced with
> kfree_rcu. Plus build time asserts added the struct dma-fence remains first
> in the respective driver structs. It sounds feasible.)

FWIW, I pulled this series from Christian into Xe and attempted to
disconnect fences in Xe [1]. It seems to work in my local testing, but
let’s see what CI says.

I still needed a release callback [2] to maintain an external lock for
our HW fences and the dma-fence signaling IRQ, but it should now be
fully disconnected from the module. I coded this in about an hour, so
take it with a grain of salt.

Matt

[1] https://patchwork.freedesktop.org/series/156388/
[2] https://patchwork.freedesktop.org/patch/682962/?series=156388&rev=1

> 
> That would leave us with .release in:
>  - vgem, nouveau, lima, pvr, i915
> 
> Combined list of custom .wait + .release:
>  - radeon, qxl, nouveau, i915, lima, pvr, vgem
> 
> From those the ones which support unbind and module unload would remain
> potentially vulnerable to use after free.
> 
> It doesn't sound great to only solve it partially but maybe it is a
> reasonable next step. Where could we go from there to solve it for everyone?
> 
> Regards,
> 
> Tvrtko
> 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > ---
> > >   drivers/dma-buf/dma-fence.c | 16 ++++++++++++----
> > >   include/linux/dma-fence.h   |  4 ++--
> > >   2 files changed, 14 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > index 982f2b2a62c0..39f73edf3a33 100644
> > > --- a/drivers/dma-buf/dma-fence.c
> > > +++ b/drivers/dma-buf/dma-fence.c
> > > @@ -374,6 +374,14 @@ int dma_fence_signal_timestamp_locked(struct
> > > dma_fence *fence,
> > >                         &fence->flags)))
> > >           return -EINVAL;
> > > +    /*
> > > +     * When neither a release nor a wait operation is specified set
> > > the ops
> > > +     * pointer to NULL to allow the fence structure to become
> > > independent
> > > +     * who originally issued it.
> > > +     */
> > > +    if (!fence->ops->release && !fence->ops->wait)
> > > +        RCU_INIT_POINTER(fence->ops, NULL);
> > > +
> > >       /* Stash the cb_list before replacing it with the timestamp */
> > >       list_replace(&fence->cb_list, &cb_list);
> > > @@ -513,7 +521,7 @@ dma_fence_wait_timeout(struct dma_fence *fence,
> > > bool intr, signed long timeout)
> > >       rcu_read_lock();
> > >       ops = rcu_dereference(fence->ops);
> > >       trace_dma_fence_wait_start(fence);
> > > -    if (ops->wait) {
> > > +    if (ops && ops->wait) {
> > >           /*
> > >            * Implementing the wait ops is deprecated and not
> > > supported for
> > >            * issuer independent fences, so it is ok to use the ops
> > > outside
> > > @@ -578,7 +586,7 @@ void dma_fence_release(struct kref *kref)
> > >       }
> > >       ops = rcu_dereference(fence->ops);
> > > -    if (ops->release)
> > > +    if (ops && ops->release)
> > >           ops->release(fence);
> > >       else
> > >           dma_fence_free(fence);
> > > @@ -614,7 +622,7 @@ static bool __dma_fence_enable_signaling(struct
> > > dma_fence *fence)
> > >       rcu_read_lock();
> > >       ops = rcu_dereference(fence->ops);
> > > -    if (!was_set && ops->enable_signaling) {
> > > +    if (!was_set && ops && ops->enable_signaling) {
> > >           trace_dma_fence_enable_signal(fence);
> > >           if (!ops->enable_signaling(fence)) {
> > > @@ -1000,7 +1008,7 @@ void dma_fence_set_deadline(struct dma_fence
> > > *fence, ktime_t deadline)
> > >       rcu_read_lock();
> > >       ops = rcu_dereference(fence->ops);
> > > -    if (ops->set_deadline && !dma_fence_is_signaled(fence))
> > > +    if (ops && ops->set_deadline && !dma_fence_is_signaled(fence))
> > >           ops->set_deadline(fence, deadline);
> > >       rcu_read_unlock();
> > >   }
> > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > > index 38421a0c7c5b..e1ba1d53de88 100644
> > > --- a/include/linux/dma-fence.h
> > > +++ b/include/linux/dma-fence.h
> > > @@ -425,7 +425,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
> > >       rcu_read_lock();
> > >       ops = rcu_dereference(fence->ops);
> > > -    if (ops->signaled && ops->signaled(fence)) {
> > > +    if (ops && ops->signaled && ops->signaled(fence)) {
> > >           rcu_read_unlock();
> > >           dma_fence_signal_locked(fence);
> > >           return true;
> > > @@ -461,7 +461,7 @@ dma_fence_is_signaled(struct dma_fence *fence)
> > >       rcu_read_lock();
> > >       ops = rcu_dereference(fence->ops);
> > > -    if (ops->signaled && ops->signaled(fence)) {
> > > +    if (ops && ops->signaled && ops->signaled(fence)) {
> > >           rcu_read_unlock();
> > >           dma_fence_signal(fence);
> > >           return true;
> > 
>