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

[Eric Engestrom]

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.

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.