Re: [PATCH v18 08/13] media: mediatek: jpeg: fix remove buffer removal timing for multi-core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] media: mtk-jpeg: M2M buffer list corruption due to late removal
--

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

media: mediatek: jpeg: fix remove buffer removal timing for multi-core

This commit moves the calls to v4l2_m2m_src_buf_remove() and
v4l2_m2m_dst_buf_remove() inside the spinlock to synchronize buffer handling.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index edd9e2d0ab05f..0907960db1436 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -1654,9 +1654,6 @@ static void mtk_jpegenc_worker(struct work_struct *work)
>  		goto enc_end;
>  	}
>  
> -	v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> -	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> -
>  	schedule_delayed_work(&comp_jpeg[hw_id]->job_timeout_work,
>  			      msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));

[Severity: Critical]
Does scheduling the timeout work before removing the buffers create a race
condition?

If this worker thread stalls for the timeout duration,
mtk_jpegenc_timeout_work() can execute concurrently and return the buffers to
userspace. If userspace then immediately requeues the buffer with VIDIOC_QBUF,
it will be added to the queue via list_add_tail().

Since this worker thread hasn't yet reached v4l2_m2m_src_buf_remove(), the
buffer's node would still be linked in the M2M rdy_queue, which could result
in list corruption from double linking.

>  
> @@ -1674,6 +1671,8 @@ static void mtk_jpegenc_worker(struct work_struct *work)
>  			     &src_buf->vb2_buf);
>  	mtk_jpeg_set_enc_params(ctx, comp_jpeg[hw_id]->reg_base);
>  	mtk_jpeg_enc_start(comp_jpeg[hw_id]->reg_base);
> +	v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);

[Severity: Critical]
Are we exposed to a similar list corruption race here by starting the hardware
before removing the buffers from the M2M queue?

mtk_jpeg_enc_start() asynchronously kicks off the hardware. If the hardware
finishes immediately, it can trigger mtk_jpegenc_hw_irq_handler() on another
CPU.

The lockless IRQ handler retrieves the buffers and calls v4l2_m2m_buf_done(),
returning them to userspace. If userspace immediately requeues the buffer,
VIDIOC_QBUF will add it to the list while it's still linked in the rdy_queue,
which can cause list corruption and kernel panics.

It looks like this ordering was handled correctly in mtk_jpegdec_worker(),
where mtk_jpeg_dec_start() is called after the buffers are removed. Should
mtk_jpeg_enc_start() follow the same pattern here?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791614327.git.kyrie.wu@mediatek.com?part=8