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. -Emil > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75279 > Signed-off-by: Ilia Mirkin <imir...@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, ¤t); > + nouveau_fence_wait(current); > + nouveau_fence_ref(NULL, ¤t); > + 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, ¤t); > + nouveau_fence_wait(current); > + nouveau_fence_ref(NULL, ¤t); > + 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, ¤t); > + nouveau_fence_wait(current); > + nouveau_fence_ref(NULL, ¤t); > nouveau_fence_ref(NULL, &screen->base.fence.current); > } > if (screen->base.pushbuf) > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
