[Mesa-dev] [Mesa-stable] [PATCH] util/queue: fix a race condition in the fence code

[Gustaw Smolarczyk]

2017-09-28 18:52 GMT+02:00 Marek Olšák <maraeo at gmail.com>:
> A clearer comment would be: "Don't destroy the fence when it's in the
> middle of util_queue_fence_signal (signalled but not unlocked yet
> because util_queue_fence_is_signalled doesn't lock). Instead, wait
> until util_queue_fence_signal returns and then destroy it."
>
> Reviewed-by: Marek Olšák <marek.olsak at amd.com>
>
> Marek
>
> On Thu, Sep 28, 2017 at 6:10 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> A tempting alternative fix would be adding a lock/unlock pair in
>> util_queue_fence_is_signalled. However, that wouldn't actually
>> improve anything in the semantics of util_queue_fence_is_signalled,
>> while making that test much more heavy-weight. So this lock/unlock
>> pair in util_queue_fence_destroy for "flushing out" other threads
>> that may still be in util_queue_fence_signal looks like the better
>> fix.
>>
>> Cc: mesa-stable at lists.freedesktop.org
>> ---
>>  src/util/u_queue.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/src/util/u_queue.c b/src/util/u_queue.c
>> index 449da7dc9ab..82a761e8420 100644
>> --- a/src/util/u_queue.c
>> +++ b/src/util/u_queue.c
>> @@ -113,20 +113,33 @@ util_queue_fence_init(struct util_queue_fence *fence)
>>     memset(fence, 0, sizeof(*fence));
>>     (void) mtx_init(&fence->mutex, mtx_plain);
>>     cnd_init(&fence->cond);
>>     fence->signalled = true;
>>  }
>>
>>  void
>>  util_queue_fence_destroy(struct util_queue_fence *fence)
>>  {
>>     assert(fence->signalled);
>> +
>> +   /* Protect against the following race:
>> +    *
>> +    * util_queue_fence_signal (begin)
>> +    *   fence->signalled = true;
>> +    *                                             util_queue_fence_is_signalled
>> +    *                                             util_queue_fence_destroy
>> +    *   cnd_broadcast(&fence->cond); <-- use-after-free
>> +    * util_queue_fence_signal (end)
>> +    */
>> +   mtx_lock(&fence->mutex);
>> +   mtx_unlock(&fence->mutex);

What about the ones that wait in util_queue_fence_wait? I think it's
possible for the following:

-----------------------

         util_queue_fence_wait (begin)

         ...

         cnd_wait(&fence->cond, &fence->mutex);
util_queue_fence_signal (begin)
mtx_lock(&fence->mutex);
fence->signalled = true;
cnd_broadcast(&fence->cond);
mtx_unlock(&fence->mutex);
util_queue_fence_signal (end)
                                        util_queue_fence_is_signalled
                                        util_queue_fence_destroy (begin)
                                        mtx_lock(&fence->mutex);
                                        mtx_unlock(&fence->mutex);
                                        cnd_destroy(&fence->cond);
                                        mtx_destroy(&fence->mutex);
                                        util_queue_fence_destroy (end)

         mtx_unlock(&fence->mutex); <--- use after free

         util_queue_fence_wait (end)
-----------------------

Unless it is defined that the race between mtx_unlock and mtx_destroy
is automagically fixed (like mtx_destroy performs a lock operation
inside of itself; pthread_mutex_* don't do that IIUC).

Regards,
Gustaw