Re: [PATCH v16 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] The newly added calls to `vb2_wait_for_all_buffers(q)` in `mtk_jpeg_enc_stop_streaming()` and `mtk_jpeg_dec_stop_streaming()` introduce an unkillable deadlock (local denial of service) when there are unmatched buffers in the m2m queues.

Pre-existing issues:
- [High] A race condition between the hardware IRQ handler and the job timeout worker leads to a NULL pointer dereference and kernel panic.
--

--- Patch [4]: [PATCH v16 04/12] media: mediatek: jpeg: Fix buffer completion on multi-core streaming stop ---
commit 613fe46e184bc6ab593928ce530f8144c0d9af31
Author: Kyrie Wu <kyrie.wu@mediatek.com>
Subject: 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. It introduces a call to vb2_wait_for_all_buffers()
in the stop_streaming functions to prevent data loss.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 7856a556ce68e..d0fb68bc884b4 100644
> --- 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 introduce a deadlock if userspace queues an unmatched number of
buffers, such as only OUTPUT buffers but no CAPTURE buffers, and then calls
VIDIOC_STREAMOFF?

When VIDIOC_STREAMOFF is called, the V4L2 framework invokes
v4l2_m2m_cancel_job(), which removes the context from the job queue. This
means no hardware worker will ever process the parked buffers.

Because the worker is cancelled, the buffers remain in the queue. Calling
vb2_wait_for_all_buffers() will block uninterruptibly waiting for the
owned_by_drv_count to drop to 0, which is structurally impossible here and
results in a hung task.

> +
>  	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]
Does this share the same hang issue as mtk_jpeg_enc_stop_streaming() above?

Waiting for buffers here after v4l2_m2m_cancel_job() has removed the context
from the job queue could similarly lead to an unkillable task if there are
unmatched buffers.

> +
>  	if (ctx->state == MTK_JPEG_SOURCE_CHANGE &&
>  	    V4L2_TYPE_IS_CAPTURE(q->type)) {

[ ... ]

[Severity: High]
This is a pre-existing issue, but can this code encounter a NULL pointer
dereference if a hardware timeout occurs exactly when the hardware IRQ fires?

If the timeout occurs concurrently with the hardware IRQ, mtk_jpeg_enc_irq()
calls cancel_delayed_work().

Since this does not wait for the work to finish if it has already started
executing on another CPU, both mtk_jpeg_enc_done() and
mtk_jpeg_job_timeout_work() can execute concurrently:

CPU1 in mtk_jpeg_job_timeout_work():
    src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
    dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);

CPU2 in mtk_jpeg_enc_done():
    src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
    dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);

Because these removals are not externally coordinated, one thread will
retrieve NULL pointers for the buffers.

This thread then unconditionally dereferences the NULL dst_buf in
vb2_set_plane_payload() or v4l2_m2m_buf_done(), leading to a panic.

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