Re: [PATCH] media: imx-jpeg: cancel timeout worker when streaming stops

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A race condition between the frame-done IRQ handler and the timeout callback can lead to a NULL pointer dereference and hardware state corruption due to incorrectly processing a timeout for a finished job.
- [High] Missing v4l2_m2m_job_finish() on error paths in mxc_jpeg_device_run() causes the V4L2 mem2mem framework to hang permanently.
--

commit d5e489df2b36369b3ad5503c5a3b9a1b62e681de
Author: Fan Wu <fanwu01@zju.edu.cn>

media: imx-jpeg: cancel timeout worker when streaming stops

The commit adds a synchronous cancellation of the timeout worker to
mxc_jpeg_stop_streaming(). This prevents a use-after-free race condition
where the delayed work might execute after the context has been freed
when closing the file descriptor.

Fixes: cfed9632ca8e ("media: imx-jpeg: Add a timeout mechanism for each frame")
Cc: stable@vger.kernel.org

> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 725e941528848..ea2fad5dd646d 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -1798,6 +1798,8 @@ static void mxc_jpeg_stop_streaming(struct vb2_queue *q)
>  
>  	dev_dbg(ctx->mxc_jpeg->dev, "Stop streaming ctx=%p", ctx);
>  
> +	cancel_delayed_work_sync(&ctx->task_timer);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does a race condition
still exist between the frame-done IRQ handler and the timeout callback?

When a job completes, mxc_jpeg_dec_irq() calls cancel_delayed_work(),
which is non-synchronous and doesn't wait if the timeout worker is
already executing and spinning on hw_lock:

drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c:mxc_jpeg_dec_irq() {
    ...
    cancel_delayed_work(&ctx->task_timer);
    ...
}

If the IRQ handler finishes the job, the framework can immediately queue
a new job for a different context, acquiring hw_lock and setting
jpeg->slot_data.used = true.

Can the timeout worker for the original ctx then erroneously see
used == true?

drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c:mxc_jpeg_device_run_timeout() {
    ...
    if (ctx->mxc_jpeg->slot_data.used) {
        ...
        mxc_jpeg_job_finish(ctx, VB2_BUF_STATE_ERROR, true);
    ...
}

This could cause mxc_jpeg_job_finish() to be called when ctx has no
buffers left, leading to v4l2_m2m_next_src_buf() returning NULL which
is then dereferenced in v4l2_m2m_buf_done().


[Severity: High]
This is also a pre-existing issue, but can an error in
mxc_jpeg_device_run() cause a permanent hang?

If mxc_jpeg_alloc_slot_data() fails, the code drops hw_lock and returns
without clearing the V4L2 TRANS_RUNNING state or calling
v4l2_m2m_job_finish():

drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c:mxc_jpeg_device_run() {
    ...
    if (!mxc_jpeg_alloc_slot_data(jpeg)) {
        dev_err(dev, "Cannot allocate slot data\n");
        goto end;
    }
    ...
}

Because schedule_delayed_work() is skipped, the timeout worker never runs
to rescue the job.

Would this cause v4l2_m2m_cancel_job() to spin-wait indefinitely for
TRANS_RUNNING to clear when the device is closed?

drivers/media/v4l2-core/v4l2-mem2mem.c:v4l2_m2m_cancel_job() {
    ...
    wait_event(m2m_ctx->finished,
               !(m2m_ctx->job_flags & TRANS_RUNNING));
    ...
}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623103031.3051-1-fanwu01@zju.edu.cn?part=1