Re: [PATCH v7 13/19] media: meson: vdec: Fix race conditions in job abort sequence

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Clearing the active session pointer while the hardware is still running causes unacknowledged interrupts and a CPU-locking interrupt storm.
- [High] Premature clearing of the active session pointer in job_abort bypasses hardware teardown and resource cleanup, causing severe memory and resource leaks.
- [High] AB-BA lock inversion deadlock between VIDIOC_STREAMOFF and the esparser_queue_work worker thread.
- [High] Releasing session ownership without stopping the hardware allows concurrent DMA overwrites and hardware state corruption.
- [High] Infinite loop inside esparser_queue_all_src() causing 100% CPU lockup when the queue is empty.
--

--- Patch [13]: [PATCH v7 13/19] media: meson: vdec: Fix race conditions in job abort sequence ---
commit bc32f101c548ab648eb44ae46f9cc2ea72d4c339
Author: Anand Moon <[email protected]>

media: meson: vdec: Fix race conditions in job abort sequence

Implement proper cleanup inside vdec_m2m_job_abort to safely stop
the hardware job sequence during a streaming abort or teardown.

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index b3e1d99e8889f..ac86a9c4febff 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -167,6 +167,17 @@ static void vdec_m2m_device_run(void *priv)
>  static void vdec_m2m_job_abort(void *priv)
>  {
>  	struct amvdec_session *sess = priv;
> +	struct amvdec_core *core = sess->core;
> +
> +	WRITE_ONCE(sess->should_stop, 1);
> +
> +	cancel_work_sync(&sess->esparser_queue_work);

[Severity: High]
Does this introduce an AB-BA deadlock with VIDIOC_STREAMOFF?

The V4L2 core acquires the queue lock (sess->lock) during VIDIOC_STREAMOFF
and calls vdec_m2m_job_abort(), which blocks here indefinitely waiting for
the worker. 

Meanwhile, the worker thread unconditionally attempts to acquire sess->lock
in esparser_queue_all_src():

drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() {
    ...
    scoped_guard(mutex, &sess->lock) {
    ...
}

> +
> +	mutex_lock(&core->lock);
> +	if (core->cur_sess == sess)
> +		/* Safely clear hardware ownership since we were confirmed as the owner */
> +		smp_store_release(&core->cur_sess, NULL);

[Severity: High]
Does clearing cur_sess here prematurely bypass hardware teardown?

During a normal VIDIOC_STREAMOFF, vdec_m2m_job_abort() is called before
the stop_streaming hook. If cur_sess is cleared here, the later teardown
in vdec_stop_streaming() will skip powering off the hardware and freeing
the DMA buffer (vififo_size) because core->cur_sess == sess evaluates to
false:

drivers/staging/media/meson/vdec/vdec.c:vdec_stop_streaming() {
    ...
    if (core->cur_sess == sess) {
        vdec_poweroff(sess);
        vdec_free_canvas(sess);
    ...
}

[Severity: Critical]
Can this cause an interrupt storm and CPU lockup?

Since the physical hardware isn't stopped, it may complete its active task
and assert a level-triggered interrupt. 

The ISR checks cur_sess, reads NULL, and immediately returns IRQ_HANDLED
without acknowledging or clearing the hardware register:

drivers/staging/media/meson/vdec/vdec.c:vdec_isr() {
    ...
    if (!sess)
        return IRQ_HANDLED;
    ...
}

Will this cause the unacknowledged interrupt to immediately fire again,
locking up the CPU?

[Severity: High]
Could this allow concurrent DMA overwrites and hardware state corruption?

By clearing core->cur_sess here, the V4L2 framework considers the hardware
idle and can schedule a new session. 

Since the hardware is actually still running, a newly scheduled session
might blindly overwrite the active hardware's registers in esparser_queue(),
corrupting the ongoing DMA operations:

drivers/staging/media/meson/vdec/esparser.c:esparser_queue() {
    ...
    amvdec_write_parser(core, PARSER_VIDEO_START_PTR, payload_paddr);
    ...
}

> +	mutex_unlock(&core->lock);
>  
>  	v4l2_m2m_job_finish(sess->core->m2m_dev, sess->m2m_ctx);
>  }

[Severity: High]
Does the esparser_queue_all_src() worker have an infinite loop?

When looking into the interaction with esparser_queue_work, the outer
loop in esparser_queue_all_src() appears to never terminate when the
queue is empty:

drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() {
    while (1) {
        struct vb2_v4l2_buffer *vbuf = NULL;
        bool processed_frame = false;
        ...
        scoped_guard(mutex, &sess->lock) {
            ...
            vbuf = v4l2_m2m_next_src_buf(sess->m2m_ctx);
            if (!vbuf)
                break;
            ...
        }
        ...
    }
}

Since scoped_guard is implemented as a hidden for-loop, the break
statement only exits the scoped_guard's internal loop, not the outer
while(1) loop. 

Because processed_frame remains false, the code calls cond_resched() and
spins indefinitely. Could this cause a 100% CPU lockup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic