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

[sashiko-bot@kernel.org]

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

New issues:
- [High] List corruption and race condition due to delayed buffer removal from the V4L2 M2M ready queues.

Pre-existing issues:
- [High] Potential Use-After-Free due to scheduling timeout work before acquiring hardware locks.
- [High] Lack of synchronization between the hardware IRQ handler and job timeout worker leads to double completion.
- [High] Use-After-Free of `mtk_jpeg_ctx` during file descriptor release.
- [High] Unkillable deadlock in `stop_streaming` when buffers are queued but M2M job is not started.
--

--- Patch [8]: [PATCH v16 08/12] media: mediatek: jpeg: fix remove buffer removal timing for multi-core ---
commit ddec7ab7308507663bea72ead9e975996dce1888
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-protected scope in the jpeg
encoder and decoder workers. This is intended to ensure buffers are properly
synchronized before removal from their queues to avoid buffer loss.

> 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
> @@ -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));
>  
>  	spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags);

[Severity: High]
This is a pre-existing issue, but does scheduling the timeout work before
acquiring hw_lock risk a use-after-free?

If the worker thread is preempted for more than the timeout duration just
after calling schedule_delayed_work(), the timeout work could execute,
reset the hardware, and return the buffers to userspace via
v4l2_m2m_buf_done().

Userspace might then free or reuse these buffers. When this worker thread
eventually resumes, it would blindly configure the hardware with the
invalidated buffer pointers and start the hardware operation, which could
lead to memory corruption. This appears to affect both mtk_jpegenc_worker()
and mtk_jpegdec_worker().

> @@ -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: High]
Can this lead to list corruption if the hardware finishes very quickly?

Because v4l2_m2m_src_buf_remove() and v4l2_m2m_dst_buf_remove() are called
after mtk_jpeg_enc_start(), the hardware can raise an interrupt before the
buffers are removed from the ready queue.

Since the IRQ handler does not acquire hw_lock, it can run concurrently on
another CPU and transition the buffer to userspace via v4l2_m2m_buf_done().
If userspace quickly dequeues and requeues the buffer, list_add_tail() will
be invoked to insert it back into the ready queue.

Because this worker thread has not yet removed the original list node,
could this corrupt the doubly-linked lists? I notice this order is used in
mtk_jpegenc_worker(), but mtk_jpegdec_worker() correctly calls the remove
functions before starting the hardware.

>  	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);

[Severity: High]
This is a pre-existing issue, but does calling v4l2_m2m_job_finish()
synchronously here open a window for a use-after-free of the context?

If userspace closes the file descriptor while an encode or decode job is
running, mtk_jpeg_release() invokes v4l2_m2m_ctx_release(), which tries
to wait for running jobs by checking the TRANS_RUNNING flag.

Since v4l2_m2m_job_finish() is called immediately after starting the
hardware, the flag is cleared before the hardware actually finishes. This
causes mtk_jpeg_release() to proceed without waiting and free the context.

When the hardware interrupt eventually fires, the IRQ handler dereferences
the freed context:

mtk_jpegenc_hw_irq_handler()
    mtk_jpegenc_put_buf()
        ctx = jpeg->hw_param.curr_ctx;
        spin_lock_irqsave(&ctx->done_queue_lock, flags);
        
Can this lead to memory corruption when the lock is accessed on the freed
context?

[ ... ]

While reviewing this patch, I noticed a few other pre-existing issues in
related code.

[Severity: High]
This is a pre-existing issue, but is there a race condition between the
hardware IRQ handler and the job timeout worker?

In mtk_jpeg_enc_hw.c, the IRQ handler uses the asynchronous
cancel_delayed_work():

mtk_jpegenc_hw_irq_handler() {
    ...
    cancel_delayed_work(&jpeg->job_timeout_work);
    
    ctx = jpeg->hw_param.curr_ctx;
    ...
}

If the hardware interrupt fires at the exact moment the timeout worker
begins executing, cancel_delayed_work() returns false and does not wait.
Because neither the IRQ handler nor the timeout worker acquires hw_lock,
both will execute concurrently.

Both paths will read the same buffer pointers from hw_param and invoke
v4l2_m2m_buf_done(), which could result in a double completion of the V4L2
buffers and a double increment of the hw_rdy atomic counter.

[Severity: High]
This is a pre-existing issue, but can mtk_jpeg_enc_stop_streaming()
deadlock if buffers are queued but the M2M job is not started?

In mtk_jpeg_core.c:mtk_jpeg_enc_stop_streaming():

    if (jpeg->variant->multi_core)
        vb2_wait_for_all_buffers(q);

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

If userspace queues buffers on only one queue (e.g., OUTPUT but not
CAPTURE), the V4L2 mem2mem core places the buffers in the driver's ready
queue but does not schedule the job.

If userspace then calls STREAMOFF, the wait loop precedes the cleanup
loop. vb2_wait_for_all_buffers() will block waiting for the driver to
return all buffers, but the code that returns the buffers is located after
the wait call.

Could this cause the process calling STREAMOFF to block forever in
uninterruptible sleep? The same logic appears to be present in
mtk_jpeg_dec_stop_streaming() as well.

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