Re: [PATCH v7 06/19] media: meson: vdec: Fix race condition by canceling work sync

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling `cancel_work_sync(&sess->esparser_queue_work)` inside `vdec_stop_streaming()` introduces a guaranteed AB-BA deadlock during `VIDIOC_STREAMOFF`.
- [High] `cancel_work_sync()` and `synchronize_irq()` are invoked before the hardware is powered off, leaving a race window where late interrupts can schedule work on a soon-to-be-freed session.
- [High] `vdec_close()` unconditionally powers off shared hardware state and clears the active session, allowing a rogue client to crash an ongoing decoding session.
- [Medium] The patch fails to remove the scheduling of `esparser_queue_work` during destination buffer completion, contradicting its own commit message.
--

--- Patch [6]: [PATCH v7 06/19] media: meson: vdec: Fix race condition by canceling work sync ---
commit 9631e0314174ce9dbe778bf9146832ff6e5a0364
Author: Anand Moon <[email protected]>

media: meson: vdec: Fix race condition by canceling work sync

This commit attempts to synchronize workqueue states during teardown to
eliminate race conditions. It adds synchronous cancellation of
esparser_queue_work in vdec_stop_streaming() and vdec_close().

Closes: https://lore.kernel.org/all/[email protected]/

> Remove the scheduling of esparser queue work during destination buffer
> completion, as freeing the vififo is handled elsewhere.

[Severity: Medium]
Did this actually get removed? Looking at dst_buf_done() in vdec_helpers.c,
it still contains:

    /* Buffer done probably means the vififo got freed */
    schedule_work(&sess->esparser_queue_work);

This seems to contradict the commit message and means the workqueue is still
scheduled asynchronously during destination buffer completion.

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 7689ffdb2e500..6fe9722577179 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -484,11 +484,15 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  			}
>  		}
>  
> +		mutex_unlock(&core->lock);
>  		/* Synchronize and flush pending hardware interrupt service routines */
>  		synchronize_irq(core->vdec_irq);
>  		/* Ensure esparser ISR finishes executing */
>  		synchronize_irq(core->esparser_irq);
>  
> +		cancel_work_sync(&sess->esparser_queue_work);

[Severity: High]
Can this cause a deadlock?

When userspace calls the streamoff ioctl, the v4l2 core acquires the queue
lock (&sess->lock) before calling this vdec_stop_streaming() callback.
Since cancel_work_sync() waits for the worker thread to finish, and the
worker esparser_queue_all_src() attempts to acquire mutex_lock(&sess->lock),
won't these threads wait on each other indefinitely if the worker is
already running?

[Severity: High]
Could this teardown ordering lead to a use-after-free?

Because cancel_work_sync() and synchronize_irq() are called before
vdec_poweroff() disables the hardware, the hardware is still active
and can generate interrupts. An interrupt could fire immediately after
cancel_work_sync() returns, causing dst_buf_done() to schedule the work
again. If the sess structure is subsequently freed, won't the newly
scheduled work dereference a freed pointer?

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

[ ... ]

> @@ -999,6 +1003,8 @@ static int vdec_close(struct file *file)
>  	/* Ensure esparser ISR finishes executing */
>  	synchronize_irq(core->esparser_irq);
>  
> +	cancel_work_sync(&sess->esparser_queue_work);

[Severity: High]
Does this have the same teardown ordering race as vdec_stop_streaming()?

The cancel_work_sync() call happens before vdec_poweroff(sess). If
the hardware generates a late interrupt, could the work be scheduled
again right before the session is freed?

> +
>  	if (!IS_ERR_OR_NULL(sess->recycle_thread)) {
>  		kthread_stop(sess->recycle_thread);
>  		sess->recycle_thread = NULL;

Later in vdec_close(), the hardware is powered down and the session is cleared:

    mutex_lock(&core->lock);

    vdec_poweroff(sess);
    vdec_free_canvas(sess);
    core->cur_sess = NULL;

[Severity: High]
Does this unconditionally terminate the hardware state of the active session?

Because this lacks an if (core->cur_sess == sess) check (which is safely used
in vdec_stop_streaming), if a secondary client opens and immediately closes
the device node, won't it forcefully power off the hardware and clear the
session while the primary client is actively streaming?

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

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