Re: [PATCH v7 14/19] media: meson: vdec: Correct atomic counter placement in dst_buf_done

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

New issues:
- [Critical] The patch introduces an unhandled double decrement of `sess->esparser_queued_bufs`, causing the atomic counter to underflow, corrupting stream state evaluation, and deadlocking the driver.
- [High] Using `break` inside `scoped_guard` fails to exit the worker loop, resulting in a 100% CPU infinite loop deadlock.
- [High] Returning 0 on `!vaddr` in `esparser_pad_start_code()` skips writing the mandatory start code pattern, leading to hardware out-of-bounds reads.
- [Medium] The commit message erroneously claims to relocate a decrement from the 'entry point' of the function, presenting a false premise for the change.

Pre-existing issues:
- [High] Missing decrement of `esparser_queued_bufs` on error paths permanently leaks logical buffer slots.
--

--- Patch [14]: [PATCH v7 14/19] media: meson: vdec: Correct atomic counter placement in dst_buf_done ---
commit 3bacd92d37472439ff6f7550b68eb503f9d760f0
Author: Anand Moon <[email protected]>

media: meson: vdec: Correct atomic counter placement in dst_buf_done

This commit fixes an end-of-stream boundary condition by relocating
the atomic decrement helper to execute after state evaluation. It
aims to ensure accurate stream termination signaling.

> Previously, decrementing sess->esparser_queued_bufs at the entry point
> of the function corrupted the conditional checks for the final video
> frame.

[Severity: Medium]
Is this description accurate? Looking at the baseline code, dst_buf_done()
never contained an atomic decrement at its entry point or anywhere else
inside the function. The decrement only existed in the caller functions.

> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index f02c21d5a9c18..303236f0647c0 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -314,6 +314,9 @@ static void dst_buf_done(struct amvdec_session *sess,
>  	dev_dbg(dev, "Buffer %u done, ts = %llu, flags = %08X\n",
>  		vbuf->vb2_buf.index, timestamp, flags);
>  	vbuf->field = field;
> +
> +	atomic_dec(&sess->esparser_queued_bufs);
> +

[Severity: Critical]
Does this introduce a double decrement? Since the callers like
amvdec_dst_buf_done() and amvdec_dst_buf_done_offset() still contain their
original atomic_dec() calls, this new decrement causes the counter to
underflow.

When the counter becomes negative, the EOS condition
atomic_read(&sess->esparser_queued_bufs) <= 1 evaluates to true, which
can falsely trigger V4L2_EVENT_EOS on the wrong buffer and cause a
permanent -EAGAIN deadlock.

>  	v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE);
>  
>  	/* Buffer done probably means the vififo got freed */


[Severity: High]
This is a pre-existing issue, but does the error path in amvdec_dst_buf_done()
permanently leak logical buffer slots?

drivers/staging/media/meson/vdec/vdec_helpers.c:amvdec_dst_buf_done() {
        ...
        if (list_empty(&sess->timestamps)) {
                v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
                spin_unlock_irqrestore(&sess->ts_spinlock, flags);
                return;
        }
        ...
}

When list_empty() is true, the function returns early without decrementing
sess->esparser_queued_bufs. If this triggers enough times, the counter
artificially hits the max limit, preventing new buffers from being queued.


[Severity: High]
Does the break statement in esparser_queue_all_src() inadvertently cause an
infinite loop?

drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() {
        while (1) {
                scoped_guard(mutex, &sess->lock) {
                        ...
                        if (!vbuf)
                                break;
                        ...
                }
                ...
        }
}

Because scoped_guard() is implemented using an internal for-loop, calling
break here only exits the scoped_guard's internal scope, not the outer
while (1) loop. This causes the worker thread to permanently hang in an
infinite busy-wait loop.


[Severity: High]
Can the failure to propagate an error in esparser_pad_start_code() cause
hardware out-of-bounds reads?

drivers/staging/media/meson/vdec/esparser.c:esparser_pad_start_code() {
        ...
        if (!sess || READ_ONCE(sess->should_stop) || !sess->priv || !vaddr)
                return 0;
        ...
}

If a client provides an unmapped buffer, !vaddr is true and this returns 0,
skipping the mandatory 512-byte start code padding.

Its caller esparser_queue() ignores this and passes the physical address to
esparser_write_data(), which unconditionally adds 512 to the payload size.
Since the padding wasn't written, the hardware DMA engine reads past the
valid buffer boundary.

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

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