[PATCH v6 3/4] dma-fence: Add safe access helpers and document the rules
Tvrtko Ursulin
tvrtko.ursulin at igalia.com
Wed Jun 11 14:08:55 UTC 2025
On 11/06/2025 12:43, Christian König wrote:
> On 6/10/25 18:42, Tvrtko Ursulin wrote:
>> Dma-fence objects currently suffer from a potential use after free problem
>> where fences exported to userspace and other drivers can outlive the
>> exporting driver, or the associated data structures.
>>
>> The discussion on how to address this concluded that adding reference
>> counting to all the involved objects is not desirable, since it would need
>> to be very wide reaching and could cause unloadable drivers if another
>> entity would be holding onto a signaled fence reference potentially
>> indefinitely.
>>
>> This patch enables the safe access by introducing and documenting a
>> contract between fence exporters and users. It documents a set of
>> contraints and adds helpers which a) drivers with potential to suffer from
>> the use after free must use and b) users of the dma-fence API must use as
>> well.
>>
>> Premise of the design has multiple sides:
>>
>> 1. Drivers (fence exporters) MUST ensure a RCU grace period between
>> signalling a fence and freeing the driver private data associated with it.
>>
>> The grace period does not have to follow the signalling immediately but
>> HAS to happen before data is freed.
>>
>> 2. Users of the dma-fence API marked with such requirement MUST contain
>> the complete access to the data within a single code block guarded by
>> rcu_read_lock() and rcu_read_unlock().
>>
>> The combination of the two ensures that whoever sees the
>> DMA_FENCE_FLAG_SIGNALED_BIT not set is guaranteed to have access to a
>> valid fence->lock and valid data potentially accessed by the fence->ops
>> virtual functions, until the call to rcu_read_unlock().
>>
>> 3. Module unload (fence->ops) disappearing is for now explicitly not
>> handled. That would required a more complex protection, possibly needing
>> SRCU instead of RCU to handle callers such as dma_fence_release() and
>> dma_fence_wait_timeout(), where race between
>> dma_fence_enable_sw_signaling, signalling, and dereference of
>> fence->ops->wait() would need a sleeping SRCU context.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>
> Reviewed-by: Christian König <christian.koenig at amd.com>
Thanks Christian!
I have pinged xe maintainers for acks to merge the whole series via
drm-misc-next and pending positive reply I can push it all.
Regards,
Tvrtko
>
>> ---
>> drivers/dma-buf/dma-fence.c | 111 ++++++++++++++++++++++++++++---
>> include/linux/dma-fence.h | 31 ++++++---
>> include/trace/events/dma_fence.h | 38 +++++++++--
>> 3 files changed, 157 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index 74f9e4b665e3..3f78c56b58dc 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -511,12 +511,20 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
>>
>> dma_fence_enable_sw_signaling(fence);
>>
>> - trace_dma_fence_wait_start(fence);
>> + if (trace_dma_fence_wait_start_enabled()) {
>> + rcu_read_lock();
>> + trace_dma_fence_wait_start(fence);
>> + rcu_read_unlock();
>> + }
>> if (fence->ops->wait)
>> ret = fence->ops->wait(fence, intr, timeout);
>> else
>> ret = dma_fence_default_wait(fence, intr, timeout);
>> - trace_dma_fence_wait_end(fence);
>> + if (trace_dma_fence_wait_end_enabled()) {
>> + rcu_read_lock();
>> + trace_dma_fence_wait_end(fence);
>> + rcu_read_unlock();
>> + }
>> return ret;
>> }
>> EXPORT_SYMBOL(dma_fence_wait_timeout);
>> @@ -533,16 +541,23 @@ void dma_fence_release(struct kref *kref)
>> struct dma_fence *fence =
>> container_of(kref, struct dma_fence, refcount);
>>
>> + rcu_read_lock();
>> trace_dma_fence_destroy(fence);
>>
>> - if (WARN(!list_empty(&fence->cb_list) &&
>> - !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags),
>> - "Fence %s:%s:%llx:%llx released with pending signals!\n",
>> - dma_fence_driver_name(fence),
>> - dma_fence_timeline_name(fence),
>> - fence->context, fence->seqno)) {
>> + if (!list_empty(&fence->cb_list) &&
>> + !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
>> + const char __rcu *timeline;
>> + const char __rcu *driver;
>> unsigned long flags;
>>
>> + driver = dma_fence_driver_name(fence);
>> + timeline = dma_fence_timeline_name(fence);
>> +
>> + WARN(1,
>> + "Fence %s:%s:%llx:%llx released with pending signals!\n",
>> + rcu_dereference(driver), rcu_dereference(timeline),
>> + fence->context, fence->seqno);
>> +
>> /*
>> * Failed to signal before release, likely a refcounting issue.
>> *
>> @@ -556,6 +571,8 @@ void dma_fence_release(struct kref *kref)
>> spin_unlock_irqrestore(fence->lock, flags);
>> }
>>
>> + rcu_read_unlock();
>> +
>> if (fence->ops->release)
>> fence->ops->release(fence);
>> else
>> @@ -982,11 +999,21 @@ EXPORT_SYMBOL(dma_fence_set_deadline);
>> */
>> void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
>> {
>> + const char __rcu *timeline;
>> + const char __rcu *driver;
>> +
>> + rcu_read_lock();
>> +
>> + timeline = dma_fence_timeline_name(fence);
>> + driver = dma_fence_driver_name(fence);
>> +
>> seq_printf(seq, "%s %s seq %llu %ssignalled\n",
>> - dma_fence_driver_name(fence),
>> - dma_fence_timeline_name(fence),
>> + rcu_dereference(driver),
>> + rcu_dereference(timeline),
>> fence->seqno,
>> dma_fence_is_signaled(fence) ? "" : "un");
>> +
>> + rcu_read_unlock();
>> }
>> EXPORT_SYMBOL(dma_fence_describe);
>>
>> @@ -1055,3 +1082,67 @@ dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
>> BIT(DMA_FENCE_FLAG_SEQNO64_BIT));
>> }
>> EXPORT_SYMBOL(dma_fence_init64);
>> +
>> +/**
>> + * dma_fence_driver_name - Access the driver name
>> + * @fence: the fence to query
>> + *
>> + * Returns a driver name backing the dma-fence implementation.
>> + *
>> + * IMPORTANT CONSIDERATION:
>> + * Dma-fence contract stipulates that access to driver provided data (data not
>> + * directly embedded into the object itself), such as the &dma_fence.lock and
>> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
>> + * after the fence has been signalled. Drivers are allowed to free that data,
>> + * and some do.
>> + *
>> + * To allow safe access drivers are mandated to guarantee a RCU grace period
>> + * between signalling the fence and freeing said data.
>> + *
>> + * As such access to the driver name is only valid inside a RCU locked section.
>> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
>> + * by the &rcu_read_lock and &rcu_read_unlock pair.
>> + */
>> +const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
>> +{
>> + RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>> + "RCU protection is required for safe access to returned string");
>> +
>> + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> + return fence->ops->get_driver_name(fence);
>> + else
>> + return "detached-driver";
>> +}
>> +EXPORT_SYMBOL(dma_fence_driver_name);
>> +
>> +/**
>> + * dma_fence_timeline_name - Access the timeline name
>> + * @fence: the fence to query
>> + *
>> + * Returns a timeline name provided by the dma-fence implementation.
>> + *
>> + * IMPORTANT CONSIDERATION:
>> + * Dma-fence contract stipulates that access to driver provided data (data not
>> + * directly embedded into the object itself), such as the &dma_fence.lock and
>> + * memory potentially accessed by the &dma_fence.ops functions, is forbidden
>> + * after the fence has been signalled. Drivers are allowed to free that data,
>> + * and some do.
>> + *
>> + * To allow safe access drivers are mandated to guarantee a RCU grace period
>> + * between signalling the fence and freeing said data.
>> + *
>> + * As such access to the driver name is only valid inside a RCU locked section.
>> + * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block guarded
>> + * by the &rcu_read_lock and &rcu_read_unlock pair.
>> + */
>> +const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
>> +{
>> + RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>> + "RCU protection is required for safe access to returned string");
>> +
>> + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> + return fence->ops->get_driver_name(fence);
>> + else
>> + return "signaled-timeline";
>> +}
>> +EXPORT_SYMBOL(dma_fence_timeline_name);
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index 10a849cb2d3f..64639e104110 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -378,15 +378,28 @@ bool dma_fence_remove_callback(struct dma_fence *fence,
>> struct dma_fence_cb *cb);
>> void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>>
>> -static inline const char *dma_fence_driver_name(struct dma_fence *fence)
>> -{
>> - return fence->ops->get_driver_name(fence);
>> -}
>> -
>> -static inline const char *dma_fence_timeline_name(struct dma_fence *fence)
>> -{
>> - return fence->ops->get_timeline_name(fence);
>> -}
>> +/**
>> + * DOC: Safe external access to driver provided object members
>> + *
>> + * All data not stored directly in the dma-fence object, such as the
>> + * &dma_fence.lock and memory potentially accessed by functions in the
>> + * &dma_fence.ops table, MUST NOT be accessed after the fence has been signalled
>> + * because after that point drivers are allowed to free it.
>> + *
>> + * All code accessing that data via the dma-fence API (or directly, which is
>> + * discouraged), MUST make sure to contain the complete access within a
>> + * &rcu_read_lock and &rcu_read_unlock pair.
>> + *
>> + * Some dma-fence API handles this automatically, while other, as for example
>> + * &dma_fence_driver_name and &dma_fence_timeline_name, leave that
>> + * responsibility to the caller.
>> + *
>> + * To enable this scheme to work drivers MUST ensure a RCU grace period elapses
>> + * between signalling the fence and freeing the said data.
>> + *
>> + */
>> +const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
>> +const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>>
>> /**
>> * dma_fence_is_signaled_locked - Return an indication if the fence
>> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
>> index 84c83074ee81..4814a65b68dc 100644
>> --- a/include/trace/events/dma_fence.h
>> +++ b/include/trace/events/dma_fence.h
>> @@ -34,14 +34,44 @@ DECLARE_EVENT_CLASS(dma_fence,
>> __entry->seqno)
>> );
>>
>> -DEFINE_EVENT(dma_fence, dma_fence_emit,
>> +/*
>> + * Safe only for call sites which are guaranteed to not race with fence
>> + * signaling,holding the fence->lock and having checked for not signaled, or the
>> + * signaling path itself.
>> + */
>> +DECLARE_EVENT_CLASS(dma_fence_unsignaled,
>> +
>> + TP_PROTO(struct dma_fence *fence),
>> +
>> + TP_ARGS(fence),
>> +
>> + TP_STRUCT__entry(
>> + __string(driver, fence->ops->get_driver_name(fence))
>> + __string(timeline, fence->ops->get_timeline_name(fence))
>> + __field(unsigned int, context)
>> + __field(unsigned int, seqno)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(driver);
>> + __assign_str(timeline);
>> + __entry->context = fence->context;
>> + __entry->seqno = fence->seqno;
>> + ),
>> +
>> + TP_printk("driver=%s timeline=%s context=%u seqno=%u",
>> + __get_str(driver), __get_str(timeline), __entry->context,
>> + __entry->seqno)
>> +);
>> +
>> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_emit,
>>
>> TP_PROTO(struct dma_fence *fence),
>>
>> TP_ARGS(fence)
>> );
>>
>> -DEFINE_EVENT(dma_fence, dma_fence_init,
>> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_init,
>>
>> TP_PROTO(struct dma_fence *fence),
>>
>> @@ -55,14 +85,14 @@ DEFINE_EVENT(dma_fence, dma_fence_destroy,
>> TP_ARGS(fence)
>> );
>>
>> -DEFINE_EVENT(dma_fence, dma_fence_enable_signal,
>> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_enable_signal,
>>
>> TP_PROTO(struct dma_fence *fence),
>>
>> TP_ARGS(fence)
>> );
>>
>> -DEFINE_EVENT(dma_fence, dma_fence_signaled,
>> +DEFINE_EVENT(dma_fence_unsignaled, dma_fence_signaled,
>>
>> TP_PROTO(struct dma_fence *fence),
>>
>
More information about the Intel-gfx
mailing list