Re: [PATCH 2/2] dma-buf: add warning when dma_fence is signaled from IOCTL

["Christian König" <christian.koenig@amd.com>]

On 13.08.25 10:16, Philipp Stanner wrote:
> On Tue, 2025-08-12 at 16:34 +0200, Christian König wrote:
>> From: Christian König <ckoenig@able.fritz.box>
> 
> Is this the correct mail addr? :)

^^ No it isn't and how the heck did that happen?

Looks like my gitconfig is somehow changed, but I have no idea why.

>>
>> We have the re-occurring problem that people try to invent a
>> DMA-fences implementation which signals fences based on an userspace
>> IOCTL.
>>
>> This is well known as source of hard to track down crashes and is
>> documented to be an invalid approach. The problem is that it seems
>> to work during initial testing and only long term tests points out
>> why this can never work correctly.
>>
>> So give at least a warning when people try to signal a fence from
>> task context and not from interrupts or a work item. This check is
>> certainly not perfect but better than nothing.
>>
>> Signed-off-by: Christian König <ckoenig@able.fritz.box>
>> ---
>>  drivers/dma-buf/dma-fence.c | 59 +++++++++++++++++++++++++++----------
>>  include/linux/dma-fence.h   |  9 ++++--
>>  2 files changed, 51 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index 3f78c56b58dc..2bce620eacac 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -345,33 +345,23 @@ void __dma_fence_might_wait(void)
>>  }
>>  #endif
>>  
>> -
>>  /**
>> - * dma_fence_signal_timestamp_locked - signal completion of a fence
>> + * dma_fence_signal_internal - internal signal completion of a fence
>>   * @fence: the fence to signal
>>   * @timestamp: fence signal timestamp in kernel's CLOCK_MONOTONIC time domain
>>   *
>> - * Signal completion for software callbacks on a fence, this will unblock
>> - * dma_fence_wait() calls and run all the callbacks added with
>> - * dma_fence_add_callback(). Can be called multiple times, but since a fence
>> - * can only go from the unsignaled to the signaled state and not back, it will
>> - * only be effective the first time. Set the timestamp provided as the fence
>> - * signal timestamp.
>> - *
>> - * Unlike dma_fence_signal_timestamp(), this function must be called with
>> - * &dma_fence.lock held.
>> + * Internal signal the dma_fence without error checking. Should *NEVER* be used
>> + * by drivers or external code directly.
> 
> s/Internal/Internally
> 
>>   *
>>   * Returns 0 on success and a negative error value when @fence has been
>>   * signalled already.
>>   */
>> -int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>> -				      ktime_t timestamp)
>> +int dma_fence_signal_internal(struct dma_fence *fence, ktime_t timestamp)
>>  {
>>  	struct dma_fence_cb *cur, *tmp;
>>  	struct list_head cb_list;
>>  
>>  	lockdep_assert_held(fence->lock);
>> -
>>  	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
>>  				      &fence->flags)))
>>  		return -EINVAL;
>> @@ -390,7 +380,46 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>  
>>  	return 0;
>>  }
>> -EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
>> +EXPORT_SYMBOL(dma_fence_signal_internal);
> 
> If it must only be used internally, can it be kept private, without
> exporting the symbol?

I have gone back and force about that as well.

We would then need to uninline dma_fence_is_signaled().

>> +
>> +/**
>> + * dma_fence_signal_timestamp_locked - signal completion of a fence
>> + * @fence: the fence to signal
>> + * @timestamp: fence signal timestamp in kernel's CLOCK_MONOTONIC time domain
>> + *
>> + * Signal completion for software callbacks on a fence, this will unblock
>> + * dma_fence_wait() calls and run all the callbacks added with
>> + * dma_fence_add_callback(). Can be called multiple times, but since a fence
>> + * can only go from the unsignaled to the signaled state and not back, it will
>> + * only be effective the first time. Set the timestamp provided as the fence
>> + * signal timestamp.
>> + *
>> + * Unlike dma_fence_signal_timestamp(), this function must be called with
>> + * &dma_fence.lock held.
>> + *
>> + * Returns 0 on success and a negative error value when @fence has been
>> + * signalled already.
>> + */
>> +int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>> +				      ktime_t timestamp)
>> +{
>> +	/*
>> +	 * We have the re-occurring problem that people try to invent a
>> +	 * DMA-fences implementation which signals fences based on an userspace
>> +	 * IOCTL.
>> +	 *
>> +	 * This is well known as source of hard to track down crashes and is
>> +	 * documented to be an invalid approach. The problem is that it seems
>> +	 * to work during initial testing and only long term tests points out
>> +	 * why this can never work correctly.
>> +	 *
>> +	 * So give at least a warning when people try to signal a fence from
>> +	 * task context and not from interrupts or a work item. This check is
>> +	 * certainly not perfect but better than nothing.
>> +	 */
>> +	WARN_ON_ONCE(!in_interrupt() && !current_work());
>> +	return dma_fence_signal_internal(fence, timestamp);
>> +}
> 
> So this now is the point to decide what we want: do you want to *allow*
> drivers to do it, or want to *prohibit* it?
> 
> If you want to prohibit it, then (additionally) returning an error code
> here would make sense.

I'm actually trying to remove the return value for quite a while now.

IIRC nobody is actually using it any more since it doesn't really signal an error condition.

Thanks,
Christian.

> 
> 
> P.
> 
>>  
>>  /**
>>   * dma_fence_signal_timestamp - signal completion of a fence
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index 64639e104110..8dbcd66989b8 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -369,6 +369,7 @@ int dma_fence_signal_locked(struct dma_fence *fence);
>>  int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
>>  int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>  				      ktime_t timestamp);
>> +int dma_fence_signal_internal(struct dma_fence *fence, ktime_t timestamp);
>>  signed long dma_fence_default_wait(struct dma_fence *fence,
>>  				   bool intr, signed long timeout);
>>  int dma_fence_add_callback(struct dma_fence *fence,
>> @@ -422,7 +423,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>>  		return true;
>>  
>>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
>> -		dma_fence_signal_locked(fence);
>> +		dma_fence_signal_internal(fence, ktime_get());
>>  		return true;
>>  	}
>>  
>> @@ -448,11 +449,15 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>>  static inline bool
>>  dma_fence_is_signaled(struct dma_fence *fence)
>>  {
>> +	unsigned long flags;
>> +
>>  	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>  		return true;
>>  
>>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
>> -		dma_fence_signal(fence);
>> +		spin_lock_irqsave(fence->lock, flags);
>> +		dma_fence_signal_internal(fence, ktime_get());
>> +		spin_unlock_irqrestore(fence->lock, flags);
>>  		return true;
>>  	}
>>  
>