Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 15, 2020 at 4:37 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Daniel Vetter (2020-07-15 15:03:34)
> > On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > There's a further problem in that we call INIT_LIST_HEAD on the
> > > dma_fence_cb before the signal callback. So even if list_empty_careful()
> > > confirms the dma_fence_cb to be completely decoupled, the containing
> > > struct may still be inuse.
> >
> > The kerneldoc of dma_fence_remove_callback() already has a very stern
> > warning that this will blow up if you don't hold a full reference or
> > otherwise control the lifetime of this stuff. So I don't think we have
> > to worry about any of that. Or do you mean something else?
>
> It's the struct dma_fence_cb itself that may be freed/reused. Consider
> dma_fence_default_wait(). That uses struct default_wait_cb on the stack,
> so in order to ensure that the callback is completed the list_empty
> check has to remain under the spinlock, or else
> dma_fence_default_wait_cb() can still be dereferencing wait->task as the
> function returns.

The current implementation of remove_callback doesn't work if you
don't own the callback structure. Or control its lifetime through some
other means.

So if we have callers removing other callback structures, that just
doesn't work, you can only remove your own.

From a quick spot check across a few callers we don't seem to have a
problem here, all current callers for this function are in various
wait functions (driver specific, or multi fence waits, stuff like
that).
-Daniel

> So currently it is:
>
> signed long
> dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
> {
>         struct default_wait_cb cb;
>         unsigned long flags;
>         signed long ret = timeout ? timeout : 1;
>
>         spin_lock_irqsave(fence->lock, flags);
>
>         if (intr && signal_pending(current)) {
>                 ret = -ERESTARTSYS;
>                 goto out;
>         }
>
>         if (!__dma_fence_enable_signaling(fence))
>                 goto out;
>
>         if (!timeout) {
>                 ret = 0;
>                 goto out;
>         }
>
>         cb.base.func = dma_fence_default_wait_cb;
>         cb.task = current;
>         list_add(&cb.base.node, &fence->cb_list);
>
>         while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
>                 if (intr)
>                         __set_current_state(TASK_INTERRUPTIBLE);
>                 else
>                         __set_current_state(TASK_UNINTERRUPTIBLE);
>                 spin_unlock_irqrestore(fence->lock, flags);
>
>                 ret = schedule_timeout(ret);
>
>                 spin_lock_irqsave(fence->lock, flags);
>                 if (ret > 0 && intr && signal_pending(current))
>                         ret = -ERESTARTSYS;
>         }
>
>         if (!list_empty(&cb.base.node))
>                 list_del(&cb.base.node);
>         __set_current_state(TASK_RUNNING);
>
> out:
>         spin_unlock_irqrestore(fence->lock, flags);
>         return ret;
> }
>
> but it could be written as:
>
> signed long
> dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
> {
>         struct default_wait_cb cb;
>         int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
>
>         cb.task = current;
>         if (dma_fence_add_callback(fence, &cb.base, dma_fence_default_wait_cb))
>                 return timeout ? timeout : 1;
>
>         for (;;) {
>                 set_current_state(state);
>
>                 if (dma_fence_is_signaled(fence)) {
>                         timeout = timeout ? timeout : 1;
>                         break;
>                 }
>
>                 if (signal_pending_state(state, current)) {
>                         timeout = -ERESTARTSYS;
>                         break;
>                 }
>
>                 if (!timeout)
>                         break;
>
>                 timeout = schedule_timeout(timeout);
>         }
>         __set_current_state(TASK_RUNNING);
>
>         dma_fence_remove_callback(fence, &cb.base);
>
>         return timeout;
> }
> -Chris



-- 
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