Re: [PATCH v6 2/8] media: meson: vdec: Fix concurrent STREAMON / STREAMOFF race conditions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Premature clearing of `core->cur_sess` causes concurrent hardware accesses.
- [High] Unconditionally setting `sess->status = STATUS_STOPPED` during `STREAMOFF` breaks dynamic resolution changes (DRC).
--

commit bb4b11ce2f674461952a82e3ffa257bb828dacb8
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Fix concurrent STREAMON / STREAMOFF race conditions

The Meson VDEC driver’s start/stop streaming paths previously
updated core->cur_sess and sess->status without synchronization,
leaving a race window between concurrent STREAMON/STREAMOFF calls.
This patch introduces proper locking discipline to prevent TOCTOU races
and ensure consistent session lifecycle management.

Closes: https://lore.kernel.org/all/20260525104345.C8D501F00A3C@smtp.kernel.org/

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 4ffebba2341d4..7233000e22323 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c

[ ... ]

> @@ -399,10 +418,23 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  	struct amvdec_codec_ops *codec_ops = sess->fmt_out->codec_ops;
>  	struct amvdec_core *core = sess->core;
>  	struct vb2_v4l2_buffer *buf;
> +	enum amvdec_status old_status;
>  
> -	if (sess->status == STATUS_RUNNING ||
> -	    sess->status == STATUS_INIT ||
> -	    (sess->status == STATUS_NEEDS_RESUME &&
> +	/*
> +	 * Safely snapshot the status and clear the hardware owner inside
> +	 * the mutex to prevent data races with concurrent STREAMON requests.
> +	 */
> +	mutex_lock(&core->lock);
> +	old_status = sess->status;
> +	if (core->cur_sess == sess)
> +		core->cur_sess = NULL;

[Severity: High]
Does prematurely clearing core->cur_sess here open a race window for concurrent
hardware accesses?

Since the lock is dropped immediately after this block, another session can
call vdec_start_streaming(), acquire the lock, claim core->cur_sess, and call
vdec_poweron().

Meanwhile, the old session continues executing vdec_stop_streaming() and
reaches vdec_poweroff(sess), which calls vdec_wait_inactive(sess) and can
sleep for up to 50ms while the hardware shuts down.

Does this result in vdec_poweron() and vdec_poweroff() executing
concurrently on the same hardware without synchronization?

(Note: A later commit in this series, "media: meson: vdec: Fix NULL pointer
dereference in ISR handlers", addresses an ISR dereference during this
window, but it does not appear to resolve this underlying concurrent
initialization and teardown race.)

> +	sess->status = STATUS_STOPPED;

[Severity: High]
Does unconditionally setting sess->status = STATUS_STOPPED break dynamic
resolution changes (DRC)?

During DRC, the hardware sets sess->status = STATUS_NEEDS_RESUME, and
userspace responds by stopping the capture queue (STREAMOFF). If we overwrite
sess->status to STATUS_STOPPED here, the subsequent STREAMON from userspace
will check this in vdec_start_streaming():

    if (sess->status == STATUS_NEEDS_RESUME &&
        q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
        sess->changed_format) {
        codec_ops->resume(sess);
        sess->status = STATUS_RUNNING;
        return 0;
    }

Because the status is now STATUS_STOPPED, does this bypass the fast resume
path and fall through to a full re-initialization?

This appears to cause the reallocation of the 16MB vififo DMA buffer (leaking
the original 16MB allocation which was never freed) and spawn a duplicate
recycle_thread (leaking the running thread).

> +	mutex_unlock(&core->lock);
> +
> +	/* Evaluate the hardware state using our snapshot */
> +	if (old_status == STATUS_RUNNING ||
> +	    old_status == STATUS_INIT ||
> +	    (old_status == STATUS_NEEDS_RESUME &&
>  	     (!sess->streamon_out || !sess->streamon_cap))) {
>  		if (vdec_codec_needs_recycle(sess))
>  			kthread_stop(sess->recycle_thread);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530094326.11892-1-linux.amoon@gmail.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic