Address data races involving 'should_stop' and prevent multi-session hardware clobbering by enforcing atomic tracking and strict owner validation during device teardown.
The esparser work queue reads 'sess->should_stop' outside of critical regions without serialization primitives, risking data races or visibility delays. Furthermore, vdec_close() and vdec_stop_streaming() blindly shut down hardware components (via poweroff and canvas frees) and nullify 'core->cur_sess' without confirming that the executing session actually owns the active hardware context. In multi-session scenarios, this allows a closing inactive session to inadvertently break a running session. To fix these synchronization and lifecycle issues with the following changes use thread-safe flagging: Wrap reads and writes of 'sess->should_stop' in READ_ONCE() and WRITE_ONCE() to prevent compiler optimizations from caching the condition variables across scheduling boundaries and optimizations across workqueue execution threads. Also safe context releasing: transition 'core->cur_sess' pointer clearings to use smp_store_release(). This ensures all prior internal memory structures are entirely flushed and visible to other execution cores. Cc: Nicolas Dufresne <[email protected]> Signed-off-by: Anand Moon <[email protected]> --- drivers/staging/media/meson/vdec/esparser.c | 2 +- drivers/staging/media/meson/vdec/vdec.c | 88 ++++++++++++++------- 2 files changed, 59 insertions(+), 31 deletions(-) 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; /* Queue completely empty: exit work loop cleanly */ 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 @@ -287,9 +287,13 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) struct amvdec_session *sess = vb2_get_drv_priv(q); struct amvdec_codec_ops *codec_ops = sess->fmt_out->codec_ops; struct amvdec_core *core = sess->core; + struct device *dev = core->dev_dec; struct vb2_v4l2_buffer *buf; int ret; + /* Reset workqueue loop shutdown signal to allow streaming */ + WRITE_ONCE(sess->should_stop, 0); + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) sess->streamon_out = 1; else @@ -336,7 +340,7 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) dma_alloc_coherent(sess->core->dev, sess->vififo_size, &sess->vififo_paddr, GFP_KERNEL); if (!sess->vififo_vaddr) { - dev_err(sess->core->dev, "Failed to request VIFIFO buffer\n"); + dev_err(dev, "Failed to request VIFIFO buffer\n"); ret = -ENOMEM; goto err_cleanup_session; } @@ -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: while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx))) @@ -441,6 +447,9 @@ static void vdec_stop_streaming(struct vb2_queue *q) enum amvdec_status old_status; bool full_cleanup = false; + /* Signal workqueue loop to abort instantly */ + WRITE_ONCE(sess->should_stop, 1); + /* flush buffers to kill background workqueue thread */ if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx))) @@ -493,26 +502,33 @@ static void vdec_stop_streaming(struct vb2_queue *q) cancel_work_sync(&sess->esparser_queue_work); mutex_lock(&core->lock); - vdec_poweroff(sess); - vdec_free_canvas(sess); + if (core->cur_sess == sess) { + vdec_poweroff(sess); + vdec_free_canvas(sess); + + if (sess->vififo_vaddr) { + dma_free_coherent(sess->core->dev, + sess->vififo_size, + sess->vififo_vaddr, + sess->vififo_paddr); + sess->vififo_vaddr = NULL; + sess->vififo_paddr = 0; + } - if (sess->vififo_vaddr) { - dma_free_coherent(sess->core->dev, sess->vififo_size, - sess->vififo_vaddr, sess->vififo_paddr); - sess->vififo_vaddr = NULL; - sess->vififo_paddr = 0; - } + vdec_reset_timestamps(sess); + vdec_reset_bufs_recycle(sess); - vdec_reset_timestamps(sess); - vdec_reset_bufs_recycle(sess); - core->cur_sess = NULL; + kfree(sess->priv); + sess->priv = NULL; - kfree(sess->priv); - sess->priv = NULL; + /* Safely clear hardware ownership since we were confirmed as the owner */ + smp_store_release(&core->cur_sess, NULL); + } } else { if (sess->status == STATUS_NEEDS_RESUME) sess->changed_format = 0; } + mutex_unlock(&core->lock); } @@ -802,7 +818,7 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) if (cmd->cmd == V4L2_DEC_CMD_START) { v4l2_m2m_clear_state(sess->m2m_ctx); - sess->should_stop = 0; + WRITE_ONCE(sess->should_stop, 0); return 0; } @@ -812,7 +828,7 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) dev_dbg(dev, "Received V4L2_DEC_CMD_STOP\n"); - sess->should_stop = 1; + WRITE_ONCE(sess->should_stop, 1); v4l2_m2m_mark_stopped(sess->m2m_ctx); @@ -998,6 +1014,9 @@ static int vdec_close(struct file *file) struct amvdec_session *sess = file_to_amvdec_session(file); struct amvdec_core *core = sess->core; + /* Signal workqueue loop to abort instantly */ + WRITE_ONCE(sess->should_stop, 1); + /* Synchronize and flush pending hardware interrupt service routines */ synchronize_irq(core->vdec_irq); /* Ensure esparser ISR finishes executing */ @@ -1012,21 +1031,30 @@ static int vdec_close(struct file *file) mutex_lock(&core->lock); - vdec_poweroff(sess); - vdec_free_canvas(sess); - core->cur_sess = NULL; + if (core->cur_sess == sess) { + vdec_poweroff(sess); + vdec_free_canvas(sess); - if (sess->vififo_vaddr) { - dma_free_coherent(core->dev, sess->vififo_size, - sess->vififo_vaddr, sess->vififo_paddr); - sess->vififo_vaddr = NULL; - sess->vififo_paddr = 0; + if (sess->vififo_vaddr) { + dma_free_coherent(core->dev, + sess->vififo_size, + sess->vififo_vaddr, + sess->vififo_paddr); + sess->vififo_vaddr = NULL; + sess->vififo_paddr = 0; + } + vdec_reset_timestamps(sess); + vdec_reset_bufs_recycle(sess); } - vdec_reset_timestamps(sess); - vdec_reset_bufs_recycle(sess); + 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); + mutex_unlock(&core->lock); v4l2_m2m_ctx_release(sess->m2m_ctx); -- 2.50.1
