[BUG] media: xilinx: dma - stop_streaming does not wait for a running DMA completion callback

[Samarth Narang <samarth.colleges@gmail.com>]

Hi,

I think xvip_dma_stop_streaming() can race with an in-progress DMA
completion callback, potentially causing a buffer to be returned twice
or accessed after it has been released. The line numbers below refer to
mainline at 36aad21d50d1.

In drivers/media/platform/xilinx/xilinx-dma.c, xvip_dma_stop_streaming()
calls dmaengine_terminate_all() (line 442) before taking queued_lock and
returning the buffers on queued_bufs with VB2_BUF_STATE_ERROR (lines
449-454). Meanwhile, xvip_dma_complete() takes the same lock, removes
the completed buffer from the list, and then calls vb2_buffer_done()
with VB2_BUF_STATE_DONE after releasing the lock (lines 283-296).

The Xilinx DMA provider invokes completion callbacks from a tasklet
after releasing the channel lock. Although xilinx_dma_terminate_all()
halts the channel and frees its descriptor lists, it does not wait for
a callback that is already running. That synchronization is handled by
xilinx_dma_synchronize(), which calls tasklet_kill()
(drivers/dma/xilinx/xilinx_dma.c, lines 2739-2743), but is not invoked
by dmaengine_terminate_all().

If a completion callback is already running when streaming is stopped,
the two paths can race over the same buffer. If stop_streaming()
acquires queued_lock first, it may remove the buffer and return it with
VB2_BUF_STATE_ERROR before the callback attempts to remove the same list
entry and return it again with VB2_BUF_STATE_DONE. Even when the
callback removes the entry first, it may still be using the buffer after
stop_streaming() returns, at which point videobuf2 may release it. This
is a callback synchronization issue rather than an outstanding hardware
DMA transfer, since the engine itself has already been halted.

Since stop_streaming() can sleep, I believe dmaengine_terminate_sync()
would address the race, following the approach used for stm32-dcmi in
ab32b2c3636b ("media: stm32-dcmi: remove deprecated
dmaengine_terminate_all()"):

--- a/drivers/media/platform/xilinx/xilinx-dma.c
+++ b/drivers/media/platform/xilinx/xilinx-dma.c
@@ -439,7 +439,7 @@ static void xvip_dma_stop_streaming(struct vb2_queue *vq)
 	xvip_pipeline_set_stream(pipe, false);
 
 	/* Stop and reset the DMA engine. */
-	dmaengine_terminate_all(dma->dma);
+	dmaengine_terminate_sync(dma->dma);
 
 	/* Cleanup the pipeline and mark it as being stopped. */
 	xvip_pipeline_cleanup(pipe);

There is also a related question about the error path in
xvip_dma_start_streaming() (lines 422-427), which returns previously
queued buffers with VB2_BUF_STATE_QUEUED without terminating the DMA
channel. As far as I can tell, their descriptors remain pending and
could be issued by a subsequent dma_async_issue_pending(). Is that the
intended behavior?

This issue was identified by a research static analyzer for DMA
lifecycle ordering and subsequently checked against the client and
provider implementations. I have not reproduced it on hardware, as I do
not have access to a Xilinx video pipeline and QEMU does not emulate the
relevant VDMA or video IP. The proposed change has been compile-tested
on arm64 with W=1.

If someone with suitable hardware can confirm the race or test the
change, I would be happy to submit it as a proper patch.

Thanks,
Samarth