Le dimanche 01 juin 2025 à 19:23 -0400, Sasha Levin a écrit :
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.
"While validating the controls the ops **may** need"
It wasn't the case yet, so its not as critical as this analyses made it sound like. The changes that came next, removes superfluous branches that was only there for the purpose of avoiding the crash.
It is harmless otherwise, the change is good practice and at best may help people backportin more patches. But not worth "Fixes:" and "CC: Stable" in my opinion. You are free to proceed.
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.
Suggestion for improving the report, perhaps adding line breaks for each items ?
best regards, Nicolas
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 f9bef5173bf25..4702df5b095b7 100644 --- a/drivers/staging/media/rkvdec/rkvdec.c +++ b/drivers/staging/media/rkvdec/rkvdec.c @@ -819,24 +819,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);