[Mesa-dev] [PATCH 1/2] st/mesa: Take local references for sync object fences

[Nicolai Hähnle]

On 12.10.2016 11:31, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer at amd.com>
>
> Fixes a race condition:
>
> Thread A			Thread B
> --------			--------
>
> Test if so->fence != NULL
> => true
> 				Set so->fence = NULL
> Dereference so->fence
> => NULL dereference
>
> Also, taking a reference prevents the fence from being destroyed.

Nice :)


> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98172
> Cc: mesa-stable at lists.freedesktop.org
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
> ---
>  src/mesa/state_tracker/st_cb_syncobj.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_syncobj.c b/src/mesa/state_tracker/st_cb_syncobj.c
> index 123925a..de01880 100644
> --- a/src/mesa/state_tracker/st_cb_syncobj.c
> +++ b/src/mesa/state_tracker/st_cb_syncobj.c
> @@ -81,17 +81,22 @@ static void st_check_sync(struct gl_context *ctx, struct gl_sync_object *obj)
>     struct pipe_context *pipe = st_context(ctx)->pipe;
>     struct pipe_screen *screen = pipe->screen;
>     struct st_sync_object *so = (struct st_sync_object*)obj;
> +   struct pipe_fence_handle *fence = NULL;
> +
> +   screen->fence_reference(screen, &fence, so->fence);

This should probably really use p_atomic_read (or some kind of READ_ONCE 
macro if we had it) to make the intention clear, but I'm not sure 
whether that would compile everywhere since so->fence is a pointer.

In practice it doesn't matter, because the function call is a sufficient 
barrier. So either way,

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

>
>     /* If the fence doesn't exist, assume it's signalled. */
> -   if (!so->fence) {
> +   if (!fence) {
>        so->b.StatusFlag = GL_TRUE;
>        return;
>     }
>
> -   if (screen->fence_finish(screen, pipe, so->fence, 0)) {
> +   if (screen->fence_finish(screen, pipe, fence, 0)) {
>        screen->fence_reference(screen, &so->fence, NULL);
>        so->b.StatusFlag = GL_TRUE;
>     }
> +
> +   screen->fence_reference(screen, &fence, NULL);
>  }
>
>  static void st_client_wait_sync(struct gl_context *ctx,
> @@ -101,9 +106,12 @@ static void st_client_wait_sync(struct gl_context *ctx,
>     struct pipe_context *pipe = st_context(ctx)->pipe;
>     struct pipe_screen *screen = pipe->screen;
>     struct st_sync_object *so = (struct st_sync_object*)obj;
> +   struct pipe_fence_handle *fence = NULL;
> +
> +   screen->fence_reference(screen, &fence, so->fence);
>
>     /* If the fence doesn't exist, assume it's signalled. */
> -   if (!so->fence) {
> +   if (!fence) {
>        so->b.StatusFlag = GL_TRUE;
>        return;
>     }
> @@ -120,11 +128,12 @@ static void st_client_wait_sync(struct gl_context *ctx,
>      * Assume GL_SYNC_FLUSH_COMMANDS_BIT is always set, because applications
>      * forget to set it.
>      */
> -   if (so->fence &&
> -       screen->fence_finish(screen, pipe, so->fence, timeout)) {
> +   if (screen->fence_finish(screen, pipe, fence, timeout)) {
>        screen->fence_reference(screen, &so->fence, NULL);
>        so->b.StatusFlag = GL_TRUE;
>     }
> +
> +   screen->fence_reference(screen, &fence, NULL);
>  }
>
>  static void st_server_wait_sync(struct gl_context *ctx,
>