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 <[email protected]> > > > --- > > > 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; > > >
