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

[Nicolai Hähnle]

On 29.09.2017 12:58, Eric Engestrom wrote:
> On Thursday, 2017-09-28 20:23:16 +0200, Nicolai Hähnle wrote:
>> 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.
> 
> Sorry, reading this again this morning, my message feels aggressive,
> which was no my intention, so apologies for that.

No hard feelings :)

I admit that it does look weird.

Cheers,
Nicolai

> 
> The change itself looks weird, but I understand how this can work.
> Not sure this is the best solution, but this code isn't my area of
> expertise, so I'll leave this to you :)
> 
>>
>> 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.