Re: [PATCH 1/7] media: verisilicon: Fix the cleanup when a codec ->run() fails
flat view
From: sashiko-bot@kernel.org
Date: 2026-08-19 11:24:00
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: Pre-existing issues: - [High] Missing calls to `hantro_end_prepare_run(ctx, error)` on early error paths in multiple codec `->run()` callbacks, leading to permanently incomplete V4L2 control requests. - [High] Missing NULL pointer check for the return value of `vb2_plane_vaddr()` before dereferencing it during JPEG header assembly. - [High] Missing synchronization to cancel `vpu->watchdog_work` during device removal, leading to a Use-After-Free. - [High] Race condition between late hardware IRQ delivery and `hantro_watchdog()` execution causes the watchdog of the *next* job to be cancelled and the next job to be prematurely finished. --
--- Patch [1]: [PATCH 1/7] media: verisilicon: Fix the cleanup when a codec ->run() fails ---commit 67d0bec622e69de9d51450bf1c4cea20e9bb249c
Author: Sascha Hauer [off-list ref]
media: verisilicon: Fix the cleanup when a codec ->run() fails
This commit pairs hantro_start_prepare_run() and hantro_end_prepare_run()
on error paths to ensure V4L2 requests are properly completed. It adds an
error argument to hantro_end_prepare_run() so the watchdog is only armed
when the hardware actually starts, preventing unrelated jobs from being
aborted. Resources are also properly released in device_run() on failure.
Fixes: 892bb6ecead9 ("media: hantro: do a PM resume earlier")
Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
Fixes: 727a400686a2 ("media: verisilicon: Add Rockchip AV1 decoder")
quoted hunk ↗ jump to hunk
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c index 2e81877f640fb..d9936f6979d2c 100644 --- a/drivers/media/platform/verisilicon/hantro_drv.c +++ b/drivers/media/platform/verisilicon/hantro_drv.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -162,6 +179,9 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx) v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->ctrl_handler); + if (error) + return; + /* Kick the watchdog. */ schedule_delayed_work(&ctx->dev->watchdog_work, msecs_to_jiffies(2000));
[Severity: High] This is a pre-existing issue, but does the device removal process in hantro_remove() lack synchronization to cancel vpu->watchdog_work? The driver schedules vpu->watchdog_work when a job starts. If a userspace application is streaming and a timeout occurs precisely when the device is unbound, the watchdog work might execute after the vpu object is freed by devres. This could lead to a use-after-free since cancel_delayed_work_sync() is not called during teardown. [Severity: High] This isn't a bug introduced by this patch, but can a late hardware IRQ delivery cause a race condition with hantro_watchdog()? If a job times out, hantro_watchdog() resets the hardware and completes the job. The M2M framework then schedules the next job and arms its watchdog. If the delayed hardware IRQ for the original job fires, hantro_irq_done() targets the current context (the new job) and cancels its watchdog. Could this cause the IRQ handler to erroneously call hantro_job_finish() on the new job while the hardware is still processing it, leading to state corruption and premature buffer returns?
quoted hunk ↗ jump to hunk
diff --git a/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c b/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c index ad5c1a6634f5c..30c2ac32cd0b0 100644 --- a/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c +++ b/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -264,7 +264,7 @@ int hantro_g1_h264_dec_run(struct hantro_ctx *ctx) set_ref(ctx); set_buffers(ctx, src_buf); - hantro_end_prepare_run(ctx); + hantro_end_prepare_run(ctx, 0);
[Severity: High] This is a pre-existing issue, but do the early error paths in codec run callbacks bypass hantro_end_prepare_run() after hantro_start_prepare_run() is invoked internally? If context preparation or reference setup fails in hantro_g1_h264_dec_run(), hantro_g2_hevc_dec_run(), or hantro_g1_vp8_dec_run(), early returns might miss the required call to hantro_end_prepare_run(). This can leave the V4L2 control requests incomplete indefinitely, which could lead to a resource leak and userspace application hang when malformed streams or invalid V4L2 parameters trigger these paths.
quoted hunk ↗ jump to hunk
diff --git a/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c b/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c index 86cc1a07026f0..78f51e92fb2e3 100644 --- a/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c +++ b/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -148,7 +148,7 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx) | H1_REG_ENC_PIC_INTRA | H1_REG_ENC_CTRL_EN_BIT; - hantro_end_prepare_run(ctx); + hantro_end_prepare_run(ctx, 0);
[Severity: High] This isn't a bug introduced by this patch, but is there a missing NULL pointer check for the return value of vb2_plane_vaddr() in hantro_h1_jpeg_enc_run() before it is dereferenced? Earlier in this function, vb2_plane_vaddr() assigns the virtual address of the destination buffer to jpeg_ctx.buffer. If the buffer mapping fails due to an invalid exporter or vmalloc exhaustion, it returns NULL. The pointer is then unconditionally passed to hantro_jpeg_header_assemble(), which calls memcpy() on it, potentially causing a kernel panic. Should there be a check here to safely handle mapping failures? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260819-rk3588-jpegdec-v1-0-33d74cdf369c@pengutronix.de?part=1