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

[Nicolai Hähnle]

On 28.09.2017 18:37, Eric Engestrom wrote:
> On Thursday, 2017-09-28 16:10:51 +0000, Nicolai Hähnle 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);
> 
> How's this protecting anything?

util_queue_fence_signal locks fence->mutex. So the main part of 
util_queue_fence_destroy that actually destroy stuff will be delayed 
until after the end of util_queue_fence_signal.


> This feels completely wrong...

I agree it's unusual, but it's pretty much explained by the commit 
message IMO.

Cheers,
Nicolai


> 
>> +
>>      cnd_destroy(&fence->cond);
>>      mtx_destroy(&fence->mutex);
>>   }
>>   
>>   /****************************************************************************
>>    * util_queue implementation
>>    */
>>   
>>   struct thread_input {
>>      struct util_queue *queue;
>> -- 
>> 2.11.0
>>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.