Re: [PATCH v7 08/19] media: meson: vdec: Fix concurrent execution races and unsafe teardown

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Unconditional nullification of `core->cur_sess` in `vdec_close()` allows concurrent sessions to clobber hardware ownership, triggering interrupt storms and severe memory leaks.
- [High] AB-BA deadlock between `vdec_stop_streaming()` and `esparser_queue_all_src()` workqueue due to `cancel_work_sync()`.
- [High] Inverted loop logic in `esparser_queue_all_src()` causes infinite CPU busy-wait on hardware backpressure and premature exit on success.
- [Medium] Memory leak of `sess->priv` on the newly added `vdec_start_streaming()` error path.
--

--- Patch [8]: [PATCH v7 08/19] media: meson: vdec: Fix concurrent execution races and unsafe teardown ---
commit 4587e6bbd091d6645be624dc185c0fadded8649b
Author: Anand Moon <[email protected]>

media: meson: vdec: Fix concurrent execution races and unsafe teardown

Address data races involving 'should_stop' and prevent multi-session
hardware clobbering by enforcing atomic tracking and strict owner
validation during device teardown.

> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> index e5d3d817b9b2b..959673742e699 100644
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c
> @@ -379,7 +379,7 @@ void esparser_queue_all_src(struct work_struct *work)
>  
>  		scoped_guard(mutex, &sess->lock) {
>  			/* Safe atomic tracking check: exit loop if session is shutting down */
> -			if (sess->should_stop)
> +			if (READ_ONCE(sess->should_stop))
>  				return;

[Severity: High]
Could the workqueue get stuck in an infinite loop due to how scoped_guard
expands?

Because scoped_guard(mutex, &sess->lock) expands to a for loop, if the
inner code hits an empty queue or -EAGAIN and calls break, it will only
exit the scoped_guard loop, leaving processed_frame as false.

The code then evaluates if (processed_frame) break; which fails, executes
cond_resched(), and loops the outer while (1) infinitely.

Conversely, if a frame is successfully processed (processed_frame is true),
does the loop break entirely, causing the workqueue to exit after processing
just a single frame?

[ ... ]

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 6fe9722577179..83a9b1238972a 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c

[ ... ]

> @@ -388,10 +392,12 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  		sess->streamon_cap = 0;
>  
>  	mutex_lock(&core->lock);
> -	if (core->cur_sess == sess)
> -		core->cur_sess = NULL;
> -	if (sess->status != STATUS_NEEDS_RESUME)
> -		sess->status = STATUS_STOPPED;
> +	if (core->cur_sess == sess) {
> +		/* Safely clear hardware ownership since we were confirmed as the owner */
> +		smp_store_release(&core->cur_sess, NULL);
> +		if (sess->status != STATUS_NEEDS_RESUME)
> +			sess->status = STATUS_STOPPED;
> +	}
>  	mutex_unlock(&core->lock);
>  err_unlock_no_hw:

[Severity: Medium]
Is there a memory leak of sess->priv on the error paths in
vdec_start_streaming()?

If vdec_poweron() succeeds (which dynamically allocates sess->priv), but
a later step like kthread_run() fails, the code jumps to err_poweroff.
While the hardware state and vififo_vaddr are rolled back, kfree(sess->priv)
appears to be missing.

If VIDIOC_STREAMON repeatedly fails, could this leak kernel memory?

[ ... ]

> @@ -493,26 +502,33 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  		cancel_work_sync(&sess->esparser_queue_work);

[Severity: High]
Can this cause an AB-BA deadlock between the V4L2 core and the workqueue?

When userspace calls VIDIOC_STREAMOFF, the V4L2 core acquires the queue lock
(sess->lock), then calls vdec_stop_streaming(), which calls
cancel_work_sync() here.

Meanwhile, the esparser_queue_all_src() workqueue loop uses
scoped_guard(mutex, &sess->lock) to acquire the exact same lock. If the
workqueue is active, it will block waiting for sess->lock, while
vdec_stop_streaming() blocks indefinitely waiting for the workqueue to exit.

>  		mutex_lock(&core->lock);
>  
> -		vdec_poweroff(sess);
> -		vdec_free_canvas(sess);

[ ... ]

> @@ -1012,21 +1031,30 @@ static int vdec_close(struct file *file)

[ ... ]

>  	kfree(sess->priv);
>  	sess->priv = NULL;
>  
> +	/* Unconditionally set our local status to stopped */
> +	sess->status = STATUS_STOPPED;
> +	/* Safely clear hardware ownership since we were confirmed as the owner */
> +	smp_store_release(&core->cur_sess, NULL);
> +

[Severity: Critical]
Does this unconditionally clear the hardware ownership, even if this session
doesn't own it?

If session A is streaming and session B is closed, vdec_close() for session
B will execute smp_store_release(&core->cur_sess, NULL) outside of the
if (core->cur_sess == sess) check.

If core->cur_sess becomes NULL while session A's hardware is active, could
the next hardware interrupt trigger vdec_isr() which sees !core->cur_sess
and returns IRQ_HANDLED without clearing the hardware interrupt status?

Since the interrupt is level-triggered, could this result in an immediate
interrupt storm that hangs the CPU, and permanently leak session A's
vififo_vaddr DMA buffers when session A is eventually closed?

>  	mutex_unlock(&core->lock);
>  
>  	v4l2_m2m_ctx_release(sess->m2m_ctx);

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

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