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