Skip to content

amdgpu: Make amdgpu_cs_signal_semaphore() thread-safe

José Expósito requested to merge jexposit/drm:amdgpu_cs_signal_semaphore-lock-evasion into main

The issue was found by a static analysis tool:

Error: LOCK_EVASION (CWE-543):
libdrm-2.4.115/amdgpu/amdgpu_cs.c:596: thread1_checks_field:
    Thread1 uses the value read from field "context" in the
    condition "sem->signal_fence.context". It sees that the
    condition is false. Control is switched to Thread2.
libdrm-2.4.115/amdgpu/amdgpu_cs.c:596: thread2_checks_field:
    Thread2 uses the value read from field "context" in the
    condition "sem->signal_fence.context". It sees that the
    condition is false.
libdrm-2.4.115/amdgpu/amdgpu_cs.c:598: thread2_acquires_lock:
    Thread2 acquires lock "amdgpu_context.sequence_mutex".
libdrm-2.4.115/amdgpu/amdgpu_cs.c:599: thread2_modifies_field:
    Thread2 sets "context" to a new value. Note that this write can
    be reordered at runtime to occur before instructions that do
    not access this field within this locked region. After Thread2
    leaves the critical section, control is switched back to
    Thread1.
libdrm-2.4.115/amdgpu/amdgpu_cs.c:598: thread1_acquires_lock:
    Thread1 acquires lock "amdgpu_context.sequence_mutex".
libdrm-2.4.115/amdgpu/amdgpu_cs.c:599: thread1_overwrites_value_in_field:
    Thread1 sets "context" to a new value. Now the two threads have
    an inconsistent view of "context" and updates to fields of
    "context" or fields correlated with "context" may be lost.
libdrm-2.4.115/amdgpu/amdgpu_cs.c:596: use_same_locks_for_read_and_modify:
    Guard the modification of "context" and the read used to decide
    whether to modify "context" with the same set of locks.
#  597|                   return -EINVAL;
#  598|           pthread_mutex_lock(&ctx->sequence_mutex);
#  599|->         sem->signal_fence.context = ctx;
#  600|           sem->signal_fence.ip_type = ip_type;
#  601|           sem->signal_fence.ip_instance = ip_instance;

Check sem->signal_fence.context in the locked region to avoid a race condition.

AMD

Merge request reports