From: Nicolas Dufresne nicolas.dufresne@collabora.com
[ Upstream commit d43d7db3c8a1868dcbc6cb8de90a3cdf309d6cbb ]
Setting up the control handler calls into .s_ctrl ops. While validating the controls the ops may need to access some of the context state, which could lead to a crash if not properly initialized.
Signed-off-by: Nicolas Dufresne nicolas.dufresne@collabora.com Signed-off-by: Hans Verkuil hverkuil@xs4all.nl Signed-off-by: Sasha Levin sashal@kernel.org ---
Now let me analyze the specific changes proposed in the commit: ## Analysis **YES** This commit should be backported to stable kernel trees. Here's my extensive analysis: ### Core Issue Analysis The commit addresses a critical initialization order bug that can lead to crashes. The problem occurs in the `rkvdec_open()` function at lines 822-830 in the current code: 1. **Current problematic order:** - Line 822: `rkvdec_init_ctrls(ctx)` is called first - Line 826-830: `v4l2_m2m_ctx_init()` is called second 2. **The problem:** According to the commit message and my analysis of the kernel documentation, `v4l2_ctrl_handler_setup()` (called inside `rkvdec_init_ctrls()`) calls `.s_ctrl` for all controls unconditionally to initialize hardware to default values. The `.s_ctrl` handlers may need to access the m2m context state, but if the m2m context (`ctx->fh.m2m_ctx`) isn't initialized yet, this can cause a crash or undefined behavior. ### Code Changes Analysis The fix is minimal and surgical: - **Lines moved:** The initialization order is swapped - m2m context initialization moves before control handler setup - **Error handling updated:** The error handling paths are correctly updated to match the new initialization order - **No functional changes:** The fix doesn't change driver functionality, only initialization sequence ### Why This Should Be Backported 1. **Fixes a real crash bug:** This addresses a potential crash scenario that affects users 2. **Small and contained:** The change is minimal - just reordering initialization and updating error paths 3. **Low regression risk:** Moving m2m init before control init is safer since m2m context doesn't depend on controls, but controls may depend on m2m context 4. **Follows established patterns:** Similar commit #2 in the historical examples shows a "YES" backport for proper initialization order in V4L2 drivers 5. **Critical subsystem:** Media drivers crashing can affect user applications ### Comparison with Historical Commits - **Similar to commit #2 (YES):** Also fixes initialization order in V4L2 media driver to prevent crashes - **Similar to commit #4 (YES):** Media driver fix that's low-risk and contained - **Unlike commits #1, #3, #5 (NO):** This actually fixes a bug rather than just cleanup/code style improvements ### Stable Tree Criteria Met ✅ **Important bugfix:** Prevents crashes ✅ **Minimal risk:** Simple reordering with no side effects ✅ **Small change:** Only affects initialization sequence ✅ **No architectural changes:** Pure bug fix ✅ **Confined to subsystem:** Only affects rkvdec driver The fix follows the stable tree rules perfectly - it's an important crash prevention fix with minimal regression risk, making it an ideal backport candidate.
drivers/staging/media/rkvdec/rkvdec.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c index d16cf4115d03a..6cf551cc861a4 100644 --- a/drivers/staging/media/rkvdec/rkvdec.c +++ b/drivers/staging/media/rkvdec/rkvdec.c @@ -821,24 +821,24 @@ static int rkvdec_open(struct file *filp) rkvdec_reset_decoded_fmt(ctx); v4l2_fh_init(&ctx->fh, video_devdata(filp));
- ret = rkvdec_init_ctrls(ctx); - if (ret) - goto err_free_ctx; - ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(rkvdec->m2m_dev, ctx, rkvdec_queue_init); if (IS_ERR(ctx->fh.m2m_ctx)) { ret = PTR_ERR(ctx->fh.m2m_ctx); - goto err_cleanup_ctrls; + goto err_free_ctx; }
+ ret = rkvdec_init_ctrls(ctx); + if (ret) + goto err_cleanup_m2m_ctx; + filp->private_data = &ctx->fh; v4l2_fh_add(&ctx->fh);
return 0;
-err_cleanup_ctrls: - v4l2_ctrl_handler_free(&ctx->ctrl_hdl); +err_cleanup_m2m_ctx: + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
err_free_ctx: kfree(ctx);