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

[Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>]

Quoting Daniel Vetter (2020-07-15 15:03:34)
> On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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.

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx