Re: [PATCH 2/4] sync_file: add replace and export some functionality

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Mar 15, 2017 at 02:19:16PM +1000, Dave Airlie wrote:
> >
> > uabi semantics question: Should we wake up everyone when the fence gets
> > replaced? What's the khr semaphore expectation here?
> 
> There are no real semantics for this case, you either wait the semaphore and
> replace it with NULL, or signal it where you replace the fence with a new fence.
> 
> Nobody should be using the sync_fence interfaces to poll on these fence fds.
> 
> So not crashing for non-vulkan behaviour is what we need.
> 
> The spec doesn't know anything other than this is an opaque fd,
> it shouldn't be doing operations on it, execpt importing it.
> 
> >>  static int sync_file_set_fence(struct sync_file *sync_file,
> >>                              struct dma_fence **fences, int num_fences)
> >>  {
> >> @@ -292,8 +328,10 @@ static void sync_file_free(struct kref *kref)
> >>       struct sync_file *sync_file = container_of(kref, struct sync_file,
> >>                                                    kref);
> >>
> >> -     if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
> >> -             dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
> >> +     if (sync_file->fence) {
> >> +             if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
> >> +                     dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
> >> +     }
> >>       dma_fence_put(sync_file->fence);
> >>       kfree(sync_file);
> >>  }
> >
> > I think you've missed _poll, won't that oops now?
> 
> I don't think it will, all the stuff do inside the poll handler is
> protected by the mutex
> or do you think we should hook the new fence if the old fence has poll enabled?

Yeah, or at least wake them up somehow and restart it. Or well at least
think what would happen when that does, and whether we care. If vk says
you get undefined behaviour when you replace the fence of a semaphore when
the old one hasn't signalled yet, then we don't need to do anything.

But if they spec some behaviour poll returning "ready" way after you've
replaced the fence and the new one is definitely not ready is a bit
confusing semantics. Or maybe that's exactly the semantics vulkan once,
but then we need to be careful with restarts and stuff.

wrt the oops I think there's a possibility of a use-after-free: Thus far
the wait inherited the fence reference from the sync_file (since poll
stops before the file ref is gone), but now the fence could disappear
underneath it. add_callback itself doesn't grab a reference on its own.

So either we need synchronous replacement for poll or grab a reference
there. I think at least ... coffee not yet entirely working. In any case,
this kind of nasty corner case is perfect for some vgem testing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel