Re: [PATCH v15 04/12] media: mediatek: jpeg: Fix buffer completion on multi-core streaming stop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Calling `vb2_wait_for_all_buffers()` in `stop_streaming` causes an unrecoverable deadlock.

Pre-existing issues:
- [High] Multi-core worker threads intentionally call `v4l2_m2m_job_finish()` prematurely, breaking V4L2 M2M streamoff synchronization and causing UAF/Deadlock.
--

commit adc05e09fb60cf68d6bb34203f2c1f593ffa1ccb
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: jpeg: Fix buffer completion on multi-core streaming stop

Enhances the Mediatek JPEG driver's stability and reliability by ensuring
that all queued buffers are processed before stopping the streaming in
multi-core environments.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -850,8 +850,12 @@ static struct vb2_v4l2_buffer *mtk_jpeg_buf_remove(struct mtk_jpeg_ctx *ctx,
>  static void mtk_jpeg_enc_stop_streaming(struct vb2_queue *q)
>  {
>  	struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>  	struct vb2_v4l2_buffer *vb;
>  
> +	if (jpeg->variant->multi_core)
> +		vb2_wait_for_all_buffers(q);

[Severity: High]
Does this unconditional wait risk causing an unrecoverable deadlock?

If a userspace application queues buffers, starts streaming, and then closes
the device node before the hardware has processed them, mtk_jpeg_release()
will cancel the background worker thread via cancel_work_sync().

After the worker is cancelled, any unprocessed queued buffers will never be
sent to the hardware, and thus will never complete. Since
v4l2_m2m_ctx_release() eventually calls stop_streaming(), wouldn't
vb2_wait_for_all_buffers() block forever waiting for the driver to return
the buffers?

Since the cleanup loop that aborts these buffers is located after this wait,
it appears it will never be reached. Could this cause the process to hang
indefinitely in uninterruptible sleep while holding the driver-wide mutex?

> +
>  	while ((vb = mtk_jpeg_buf_remove(ctx, q->type)))
>  		v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
>  }

[ ... ]

> @@ -859,6 +863,7 @@ static void mtk_jpeg_enc_stop_streaming(struct vb2_queue *q)
>  static void mtk_jpeg_dec_stop_streaming(struct vb2_queue *q)
>  {
>  	struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>  	struct vb2_v4l2_buffer *vb;
>  
>  	/*
> @@ -866,6 +871,9 @@ static void mtk_jpeg_dec_stop_streaming(struct vb2_queue *q)
>  	 * Before STREAMOFF, we still have to return the old resolution and
>  	 * subsampling. Update capture queue when the stream is off.
>  	 */
> +	if (jpeg->variant->multi_core)
> +		vb2_wait_for_all_buffers(q);
> +

[Severity: High]
Could the same deadlock occur here in the decoder's stop_streaming path?

This isn't a bug introduced by this patch, but does the multi-core worker
thread prematurely finish the V4L2 M2M job?

Looking at mtk_jpegenc_worker() in
drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c:

mtk_jpegenc_worker() {
    ...
    mtk_jpeg_enc_start(comp_jpeg[hw_id]->reg_base);
    v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
    ...
}

And similarly in mtk_jpegdec_worker():

mtk_jpegdec_worker() {
    ...
    mtk_jpeg_dec_start(comp_jpeg[hw_id]->reg_base);
    v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
    ...
}

By calling v4l2_m2m_job_finish() immediately after starting the hardware,
long before the asynchronous hardware interrupts arrive, the TRANS_RUNNING
flag is cleared.

Does this deceive the V4L2 framework into believing the hardware is idle
while DMA operations are still ongoing?

Because TRANS_RUNNING is cleared, v4l2_m2m_cancel_job() fails to wait for
ongoing hardware operations during streamoff. Is this underlying breakage
in the streamoff synchronization the root cause of the use-after-free and
DMA corruption that the wait loop here is trying to fix?

>  	if (ctx->state == MTK_JPEG_SOURCE_CHANGE &&
>  	    V4L2_TYPE_IS_CAPTURE(q->type)) {
>  		struct mtk_jpeg_src_buf *src_buf;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702072614.10373-1-kyrie.wu@mediatek.com?part=4