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
