[Mesa-dev] [Nouveau] [PATCH] nouveau: fix fence waiting logic in screen destroy

[Ilia Mirkin]

On Fri, Mar 7, 2014 at 9:11 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 06/03/14 04:01, Ilia Mirkin wrote:
>> nouveau_fence_wait has the expectation that an external entity is
>> holding onto the fence being waited on, not that it is merely held onto
>> by the current pointer. Fixes a use-after-free in nouveau_fence_wait
>> when used on the screen's current fence.
>>
> IMHO one should flatten all the fence handling a bit to greatly improve
> readability and minimise chances of similar problems.
>
> Also there is yet another reason why this might be a good idea -
> nouveau_fence_signalled, and all it's callers.
>
> I'll send a few patches of what I have in mind wrt "flattening", later
> on today.

I'm not sure what you're planning, but if it's an intrusive change,
could you base it on top of this one (assuming you think that it's
correct)? That way this one can still be cherry-picked to stable mesa
versions.

>
> -Emil
>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75279
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>
>> Should the waiting be predicated on the current fence having been emitted? I
>> removed that from nv30 (which would just go ahead and _leak_ that fence it it
>> hadn't been emitted...). I figure it doesn't really matter enough to worry
>> about that. The bigger reason to do it, I guess, is to make sure that all the
>> fences on the list get processed. But perhaps it'd be OK to just nuke them
>> without regard for such details?
>>
>>  src/gallium/drivers/nouveau/nv30/nv30_screen.c | 14 ++++++++++----
>>  src/gallium/drivers/nouveau/nv50/nv50_screen.c | 11 +++++++++--
>>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c |  9 ++++++++-
>>  3 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> index 82f2c06..5378913 100644
>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> @@ -308,10 +308,16 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
>>     if (!nouveau_drm_screen_unref(&screen->base))
>>        return;
>>
>> -   if (screen->base.fence.current &&
>> -       screen->base.fence.current->state >= NOUVEAU_FENCE_STATE_EMITTED) {
>> -      nouveau_fence_wait(screen->base.fence.current);
>> -      nouveau_fence_ref (NULL, &screen->base.fence.current);
>> +   if (screen->base.fence.current) {
>> +      struct nouveau_fence *current = NULL;
>> +
>> +      /* nouveau_fence_wait will create a new current fence, so wait on the
>> +       * _current_ one, and remove both.
>> +       */
>> +      nouveau_fence_ref(screen->base.fence.current, &current);
>> +      nouveau_fence_wait(current);
>> +      nouveau_fence_ref(NULL, &current);
>> +      nouveau_fence_ref(NULL, &screen->base.fence.current);
>>     }
>>
>>     nouveau_object_del(&screen->query);
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> index ab0d63e..e8c7fe3 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> @@ -294,8 +294,15 @@ nv50_screen_destroy(struct pipe_screen *pscreen)
>>        return;
>>
>>     if (screen->base.fence.current) {
>> -      nouveau_fence_wait(screen->base.fence.current);
>> -      nouveau_fence_ref (NULL, &screen->base.fence.current);
>> +      struct nouveau_fence *current = NULL;
>> +
>> +      /* nouveau_fence_wait will create a new current fence, so wait on the
>> +       * _current_ one, and remove both.
>> +       */
>> +      nouveau_fence_ref(screen->base.fence.current, &current);
>> +      nouveau_fence_wait(current);
>> +      nouveau_fence_ref(NULL, &current);
>> +      nouveau_fence_ref(NULL, &screen->base.fence.current);
>>     }
>>     if (screen->base.pushbuf)
>>        screen->base.pushbuf->user_priv = NULL;
>> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> index 044847d..04f3088 100644
>> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> @@ -340,7 +340,14 @@ nvc0_screen_destroy(struct pipe_screen *pscreen)
>>        return;
>>
>>     if (screen->base.fence.current) {
>> -      nouveau_fence_wait(screen->base.fence.current);
>> +      struct nouveau_fence *current = NULL;
>> +
>> +      /* nouveau_fence_wait will create a new current fence, so wait on the
>> +       * _current_ one, and remove both.
>> +       */
>> +      nouveau_fence_ref(screen->base.fence.current, &current);
>> +      nouveau_fence_wait(current);
>> +      nouveau_fence_ref(NULL, &current);
>>        nouveau_fence_ref(NULL, &screen->base.fence.current);
>>     }
>>     if (screen->base.pushbuf)
>>
>