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
