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

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

[Samarth Narang]

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

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

[Laurent Pinchart]

Hello,

This is an automated reply.

You have sent a patch or bug report that appears to have been generated
by an LLM. The commit message, code and/or problem description show
little to no real understanding of the issue and changes being proposed.

Such patches or reports require a large amount of work to handle. They
rely on maintainers to propose - and often even implement - a proper
design. The probability that the author will learn from the experience
to improve their future contributions is close to zero.

As a result, to make good use of maintainer time and keep the kernel
development process sustainable, your contribution has been rejected.

If you believe the change you proposed is important, take time to study
the problem, learn, and develop a solution without AI assistance.
Answering this e-mail or generating a new version of a patch by feeding
this reply to an LLM will only result in your e-mails being ignored.

Please note that if your patch attempted to address an important
problem, maintainers or other contributors can submit and merge a
similar or entirely different fix without crediting you.

Your contributions to the Linux kernel will be welcome if you genuinely
desire to invest your time and learn.

Thank you,

Laurent's mail bot

On Sat, 31 Oct 2026 20:26:53 +0000, Samarth Narang <samarth.colleges@gmail.com> wrote:
> 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
> 
>