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

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

[Fan Wu]

Each per-fd context ctx owns a delayed_work (ctx->task_timer, callback
mxc_jpeg_device_run_timeout) armed via schedule_delayed_work() at the end
of mxc_jpeg_device_run() to recover a stalled encode/decode job. The only
existing cancellation is cancel_delayed_work() in the frame-done IRQ
handler, which de-queues a pending work item but does not wait for a
callback that has already started, and it only runs when a frame completes.

When the fd is closed while a job is in flight (the frame-done IRQ has not
fired yet), nothing syncs the worker before mxc_jpeg_release() frees ctx
with kfree() after v4l2_m2m_ctx_release(). A queued or executing
mxc_jpeg_device_run_timeout() can then recover ctx through
container_of(&ctx->task_timer) and dereference it (ctx->mxc_jpeg,
slot_data, dev_warn) after ctx has been freed.

Cancel the worker from mxc_jpeg_stop_streaming(). The cancel cannot live
in mxc_jpeg_release(): mxc_jpeg_device_run() arms the timer while holding
only hw_lock, not the mxc_jpeg->lock mutex that release holds, so a cancel
in release could still race a concurrent mxc_jpeg_device_run() that
re-arms the timer afterwards. mxc_jpeg_stop_streaming() instead runs inside
v4l2_m2m_ctx_release() -> vb2_queue_release(), i.e. after
v4l2_m2m_cancel_job() has set TRANS_ABORT and waited for any in-flight job
to finish (so __v4l2_m2m_try_queue() will not queue and v4l2_m2m_try_run()
will not run any further job for this context, which prevents
mxc_jpeg_device_run() from re-arming the timer) and before the m2m context
is freed. cancel_delayed_work_sync() removes a pending work item and waits
for a running callback, so the worker can no longer race with the
subsequent kfree(). The cancel is placed before the buffer-release loop so
a concurrently running timeout callback cannot race with it over the same
buffers. If the frame-done IRQ canceled a still-pending timer, this cancel
is a no-op; if the timeout callback has already started, it waits for the
callback to finish. The same mxc_jpeg_stop_streaming() call is also
reached from VIDIOC_STREAMOFF, which drains the worker early, although
STREAMOFF itself does not free ctx -- the use-after-free arises only
when the fd is later closed.

This bug was found by static analysis.

Fixes: cfed9632ca8e ("media: imx-jpeg: Add a timeout mechanism for each frame")
Cc: stable@vger.kernel.org
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 9e4a813489c0..d85a9d196269 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -1735,6 +1735,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);
+
 	/* Release all active buffers */
 	for (;;) {
 		if (V4L2_TYPE_IS_OUTPUT(q->type))
-- 
2.34.1

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

[sashiko-bot]

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