[PATCH] drm/amdgpu: Harden against potential deadlock

[PATCH] drm/amdgpu: Harden against potential deadlock

[Philipp Stanner]

amdgpu's KFD eviction fence's dma_fence ops->enable_signaling() contains
a dma_fence_is_signaled() check.

ops->enable_signaling() is currently consistently being called with the
dma_fence lock held. dma_fence_is_signaled() might try to take this lock
in its dma_fence_signal() call.

This deadlock can currently not occur because the amdkfd_fence_ops do
not implement ops->signaled. Should that ever be added, however,
deadlocks might occur occasionally once the fastpath check in
dma_fence_is_signaled() is skipped.

Replace dma_fence_is_signaled() with dma_fence_is_signaled_locked().

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
As detailed in my recent discussions with Christian [1], I also think
that drivers should be very tightly constrained as to what they do in
the fence_ops. I think at the very least they shouldn't use any
dma_fence functions there.

So we could also remove this check here completely;
__dma_fence_enable_signaling() already checks the signaled-flag, after
all, but I'm not sure whether that might break amdgpu.


P.

[1] https://lore.kernel.org/dri-devel/320b6e7c75da6f05c015e89df0e2c7d11f20d70a.camel@mailbox.org/
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
index 9b10d015671c..61621eebb2b0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
@@ -123,7 +123,8 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f)
 	if (!fence)
 		return false;
 
-	if (dma_fence_is_signaled(f))
+	/* The enable_signaling() ops is called under lock protection. */
+	if (dma_fence_is_signaled_locked(f))
 		return true;
 
 	if (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, fence->context_id, f))
-- 
2.56.0

Re: [PATCH] drm/amdgpu: Harden against potential deadlock

[Felix Kuehling]

On 2026-10-26 10:15, Philipp Stanner wrote:
> amdgpu's KFD eviction fence's dma_fence ops->enable_signaling() contains
> a dma_fence_is_signaled() check.
>
> ops->enable_signaling() is currently consistently being called with the
> dma_fence lock held. dma_fence_is_signaled() might try to take this lock
> in its dma_fence_signal() call.
>
> This deadlock can currently not occur because the amdkfd_fence_ops do
> not implement ops->signaled. Should that ever be added, however,
> deadlocks might occur occasionally once the fastpath check in
> dma_fence_is_signaled() is skipped.
>
> Replace dma_fence_is_signaled() with dma_fence_is_signaled_locked().
>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
> As detailed in my recent discussions with Christian [1], I also think
> that drivers should be very tightly constrained as to what they do in
> the fence_ops. I think at the very least they shouldn't use any
> dma_fence functions there.
>
> So we could also remove this check here completely;
> __dma_fence_enable_signaling() already checks the signaled-flag, after
> all, but I'm not sure whether that might break amdgpu.

Looking at the code, I think it should be safe to remove the check. 
There is another one in kgd2kfd_schedule_evict_and_restore_process, 
which is called from amdkfd_fence_enable_signaling, which is also redundant.

We don't implement ops->signaled, and we explicitly signal the fence 
from SW with dma_fence_signal or dma_fence_check_and_signal. So the 
signaled flag should be the only thing that needs to be checked. The 
flag is only set while holding the fence lock, and checked while holding 
the fence lock. So in theory, relying on the check in 
__dma_fence_enable_signaling should be safe.

Regards,
   Felix


>
>
> P.
>
> [1] https://lore.kernel.org/dri-devel/320b6e7c75da6f05c015e89df0e2c7d11f20d70a.camel@mailbox.org/
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
> index 9b10d015671c..61621eebb2b0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
> @@ -123,7 +123,8 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f)
>   	if (!fence)
>   		return false;
>   
> -	if (dma_fence_is_signaled(f))
> +	/* The enable_signaling() ops is called under lock protection. */
> +	if (dma_fence_is_signaled_locked(f))
>   		return true;
>   
>   	if (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, fence->context_id, f))

Re: [PATCH] drm/amdgpu: Harden against potential deadlock

[Christian König]

On 10/26/26 19:31, Felix Kuehling wrote:
> On 2026-10-26 10:15, Philipp Stanner wrote:
>> amdgpu's KFD eviction fence's dma_fence ops->enable_signaling() contains
>> a dma_fence_is_signaled() check.
>>
>> ops->enable_signaling() is currently consistently being called with the
>> dma_fence lock held. dma_fence_is_signaled() might try to take this lock
>> in its dma_fence_signal() call.
>>
>> This deadlock can currently not occur because the amdkfd_fence_ops do
>> not implement ops->signaled. Should that ever be added, however,
>> deadlocks might occur occasionally once the fastpath check in
>> dma_fence_is_signaled() is skipped.
>>
>> Replace dma_fence_is_signaled() with dma_fence_is_signaled_locked().
>>
>> Signed-off-by: Philipp Stanner <phasta@kernel.org>
>> ---
>> As detailed in my recent discussions with Christian [1], I also think
>> that drivers should be very tightly constrained as to what they do in
>> the fence_ops. I think at the very least they shouldn't use any
>> dma_fence functions there.
>>
>> So we could also remove this check here completely;
>> __dma_fence_enable_signaling() already checks the signaled-flag, after
>> all, but I'm not sure whether that might break amdgpu.
> 
> Looking at the code, I think it should be safe to remove the check. There is another one in kgd2kfd_schedule_evict_and_restore_process, which is called from amdkfd_fence_enable_signaling, which is also redundant.
> 
> We don't implement ops->signaled, and we explicitly signal the fence from SW with dma_fence_signal or dma_fence_check_and_signal. So the signaled flag should be the only thing that needs to be checked. The flag is only set while holding the fence lock, and checked while holding the fence lock. So in theory, relying on the check in __dma_fence_enable_signaling should be safe.

I actually had a patch to remove that one send out month ago already, it was just never merged.

The call absolutely doesn't make sense since the ops->signaled callback isn't implemented and the bit tested before the enable signaling callback called.

I can take care and push the removal to amd-staging-drm-next when I find time.

Regards,
Christian.

> 
> Regards,
>   Felix
> 
> 
>>
>>
>> P.
>>
>> [1] https://lore.kernel.org/dri-devel/320b6e7c75da6f05c015e89df0e2c7d11f20d70a.camel@mailbox.org/
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>> index 9b10d015671c..61621eebb2b0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
>> @@ -123,7 +123,8 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f)
>>       if (!fence)
>>           return false;
>>   -    if (dma_fence_is_signaled(f))
>> +    /* The enable_signaling() ops is called under lock protection. */
>> +    if (dma_fence_is_signaled_locked(f))
>>           return true;
>>         if (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, fence->context_id, f))

Re: [PATCH] drm/amdgpu: Harden against potential deadlock

[Philipp Stanner]

On Tue, 2026-10-27 at 12:51 +0100, Christian König wrote:
> On 10/26/26 19:31, Felix Kuehling wrote:
> > We don't implement ops->signaled, and we explicitly signal the
> > fence from SW with dma_fence_signal or dma_fence_check_and_signal.
> > So the signaled flag should be the only thing that needs to be
> > checked. The flag is only set while holding the fence lock, and
> > checked while holding the fence lock. So in theory, relying on the
> > check in __dma_fence_enable_signaling should be safe.
> 
> I actually had a patch to remove that one send out month ago already,
> it was just never merged.
> 
> The call absolutely doesn't make sense since the ops->signaled
> callback isn't implemented and the bit tested before the enable
> signaling callback called.
> 
> I can take care and push the removal to amd-staging-drm-next when I
> find time.


OK, but put me on Cc please. I could need that change for my other
dma_fence-ish work. I would post the removal as a v2, too, if I don't
hear from you mid-term

Greetings,
Philipp