Re: [PATCH v7 4/8] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential
issues to consider:
- [High] media: qcom: jpeg: missing hardware reinit in
  qcom_jpeg_pm_system_resume()
- [High] media: qcom: jpeg: use-after-free of m2m_ctx in
  jpeg_finish_work()
- [High] media: qcom: jpeg: stop_streaming() drains both queues and
  releases HW early
- [High] media: qcom: jpeg: unverified contiguous DMA length in
  jpeg_store_fe/we_next()
- [Medium] media: qcom: jpeg: unadjusted WE_BSIZE_0 limit in
  jpeg_setup_we_size()
- [Medium] media: qcom: jpeg: 1-bit MAL_BOUNDARY masks truncate 3-bit
  boundary values
- [Medium] media: qcom: jpeg: disjoint vdev->lock and vq->lock leave
  ioctls unserialized
- [Medium] media: qcom: jpeg: fop_jpeg_file_open() skips
  v4l2_ctrl_handler_free()
- [Medium] media: qcom: jpeg: unhandled IRQs unmasked in jpeg_init()
  risk lost edge IRQ
- [Medium] media: qcom: jpeg: non-exclusive IRQ status handling in
  qcom_jenc_v420_irq_bot()
- [Low] media: qcom: jpeg: 32-bit multiplication overflow in
  jpeg_calc_optimal_clk()
--

commit 0f8ce3b457607190f1f36e252188757adec6db68
Author: Atanas Filipov <atanas.filipov@oss.qualcomm.com>

media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

This commit adds a V4L2 mem2mem driver for the standalone Qualcomm JPEG
encoder hardware block on SM8250 and SC7280 platforms.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
> new file mode 100644
> index 0000000000000..da69c0bb3bbe7
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
[ ... ]
> +static int qcom_jpeg_pm_resume(struct device *dev)
> +{
> +	struct qcom_jenc_dev *jenc = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = qcom_jpeg_clk_on(jenc);
> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static int qcom_jpeg_pm_system_suspend(struct device *dev)
> +{
> +	struct qcom_jenc_dev *jenc = dev_get_drvdata(dev);
> +	int rc;
> +
> +	v4l2_m2m_suspend(jenc->m2m_dev);
> +
> +	rc = pm_runtime_force_suspend(dev);
> +	if (rc)
> +		v4l2_m2m_resume(jenc->m2m_dev);
> +
> +	return rc;
> +}
> +
> +static int qcom_jpeg_pm_system_resume(struct device *dev)
> +{
> +	struct qcom_jenc_dev *jenc = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = pm_runtime_force_resume(dev);
> +	if (rc)
> +		return rc;
> +
> +	v4l2_m2m_resume(jenc->m2m_dev);
> +
> +	return 0;
> +}

[Severity: High]
Does system suspend and resume while streaming is active leave the JPEG
hardware uninitialized and JPEG_V420_REG_INT_MASK cleared?

jpeg_init() is only called when qcom_jenc_v420_acquire() transitions
jenc->ref_count from 0 to 1.  When qcom_jpeg_pm_system_suspend() runs
pm_runtime_force_suspend(), the titan_top power domain can collapse and
reset volatile registers, including JPEG_V420_REG_INT_MASK.  On resume,
pm_runtime_force_resume() only enables clocks in qcom_jpeg_pm_resume()
before v4l2_m2m_resume() restarts queued jobs, so subsequent jobs run with
interrupts masked in hardware and hang VIDIOC_STREAMOFF or close() in
v4l2_m2m_cancel_job().

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v420.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_v420.c
> new file mode 100644
> index 0000000000000..cd1501bca71a7
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v420.c
[ ... ]
> +static int jpeg_init(struct qcom_jenc_dev *jenc)
> +{
[ ... ]
> +	jpeg_clear_planes_queues(jenc);
> +
> +	jpeg_io_write(jenc, JPEG_V420_REG_INT_CLR, JPEG_V420_IRQ_STATUS_ALL_BITS);
> +	jpeg_io_write(jenc, JPEG_V420_REG_INT_MASK, JPEG_V420_IRQ_STATUS_ALL_BITS);
> +
> +	return 0;
> +}

[Severity: Medium]
Can unmasking JPEG_V420_IRQ_STATUS_ALL_BITS here cause unnecessary
threaded IRQ wakeups or a lost edge interrupt on SESSION_DONE?

JPEG_V420_IRQ_STATUS_ALL_BITS enables RD_BUF_PLNS_DONE_MSK,
WR_BUF_PLN0_DONE, RD_BUF_PLNS_REQ_ATTN_MSK, and WR_BUF_PLNS_REQ_ATTN_MSK,
which neither qcom_jenc_v420_irq_top() nor qcom_jenc_v420_irq_bot()
handles.  When one of those plane bits fires at the end of a frame,
qcom_jenc_v420_irq_top() wakes the IRQ thread for a no-op, and if
SESSION_DONE asserts in JPEG_V420_REG_INT_STATUS between jpeg_io_read()
and jpeg_io_write(..., JPEG_V420_REG_INT_CLR, ...) while the plane bit is
still holding the rising-edge interrupt line high, clearing only the
earlier status bits leaves SESSION_DONE asserted with no new rising edge
to complete the job.

[ ... ]
> +static int jpeg_store_fe_next(struct jenc_context *ectx, struct vb2_buffer *vb2)
> +{
> +	struct qcom_jenc_queue *q = &ectx->bufq[TYPE2QID(vb2->type)];
> +	struct qcom_jpeg_buff *buff = &q->buff[vb2->index];
> +	u8 pln;
> +
> +	for (pln = 0; pln < q->vf.num_planes; pln++) {
> +		buff->plns[pln].sgt = vb2_dma_sg_plane_desc(vb2, pln);
> +		if (!buff->plns[pln].sgt || !buff->plns[pln].sgt->sgl)
> +			return -EINVAL;
> +
> +		buff->plns[pln].dma = sg_dma_address(buff->plns[pln].sgt->sgl);
> +		if (!buff->plns[pln].dma)
> +			return -EINVAL;
> +
> +		buff->plns[pln].size = vb2_plane_size(vb2, pln);
> +		if (!buff->plns[pln].size)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Can jpeg_store_fe_next() and jpeg_store_we_next() cause out-of-bounds DMA
when a buffer is mapped as multiple scatterlist segments?

Both queues use vb2_dma_sg_memops with VB2_USERPTR and VB2_DMABUF enabled,
which does not guarantee a single contiguous DMA segment (for example, an
unaligned userptr on arm64 where iommu_dma_map_sg() falls back to
iommu_dma_map_sg_swiotlb(), or a multi-segment dmabuf).  Because the
driver programs the hardware with sg_dma_address(sgt->sgl) for the whole
plane without checking that sg_dma_len(sgt->sgl) covers
vb2_plane_size(vb2, pln), the hardware will DMA linearly past the end of
the first segment.  Should the queues use vb2_dma_contig_memops, or verify
that sg_dma_len(sgt->sgl) >= vb2_plane_size(vb2, pln)?

[ ... ]
> +static int jpeg_apply_we_addr(struct jenc_context *ectx, struct qcom_jenc_queue *q,
> +			      struct vb2_buffer *vb)
> +{
[ ... ]
> +	header_size = ectx->hdr_cache.layout.size;
> +	if (vb->planes[0].length < header_size)
> +		return -EINVAL;
> +
> +	memcpy(mptr, ectx->hdr_cache.data, header_size);
> +	v4l2_jpeg_hdr_set_size(mptr, &ectx->hdr_cache.layout, q->vf.width, q->vf.height);
> +
> +	dma += header_size;
> +
> +	/*
> +	 * Flush CPU writes to the header before handing the buffer to the
> +	 * hardware DMA engine.
> +	 */
> +	jpeg_sync_sg(jenc->dev, frame, DMA_BIDIRECTIONAL, true);
> +
> +	jpeg_io_write(jenc, jpeg_v420_we_pntr[pln], dma);
[ ... ]
> +}
[ ... ]
> +static void jpeg_setup_we_size(struct jenc_context *ectx, struct qcom_jenc_queue *q)
> +{
> +	struct v4l2_pix_format_mplane *dfmt = &q->vf;
> +	struct qcom_jenc_dev *jenc = ectx->jenc;
> +	u8 pln;
> +
> +	for (pln = 0; pln < QCOM_JPEG_MAX_PLANES; pln++)
> +		jpeg_rw_bits(jenc, jpeg_v420_we_stride[pln], JPEG_V420_PLN_WR_STRIDE_MSK, 0);
> +
> +	jpeg_io_write(jenc, JPEG_V420_REG_WE_BSIZE_0, dfmt->plane_fmt[0].sizeimage);
> +
> +	dev_dbg(ectx->dev, "WE size=%u\n", dfmt->plane_fmt[0].sizeimage);
> +}

[Severity: Medium]
Should JPEG_V420_REG_WE_BSIZE_0 be reduced by header_size?

jpeg_apply_we_addr() advances JPEG_V420_REG_WE_PNTR_0 by header_size into
the destination buffer, but jpeg_setup_we_size() programs
JPEG_V420_REG_WE_BSIZE_0 with the full dfmt->plane_fmt[0].sizeimage.  If a
compressed frame ever reaches sizeimage - header_size, the write engine is
allowed to DMA up to header_size bytes past the end of a sizeimage-sized
buffer, and out_size + header_size in qcom_jenc_v420_irq_bot() will exceed
vb->planes[0].length.

[ ... ]
> +irqreturn_t qcom_jenc_v420_irq_bot(int irq, void *data)
> +{
> +	struct qcom_jenc_dev *jenc = data;
> +	unsigned long flags;
> +	u32 irq_status;
> +
> +	irq_status = READ_ONCE(jenc->pending_irq_status);
> +
> +	if (FIELD_GET(JPEG_V420_IRQ_STATUS_SESSION_DONE, irq_status)) {
> +		struct jenc_context *ctx;
> +
> +		spin_lock_irqsave(&jenc->hw_lock, flags);
> +		ctx = jenc->actx;
> +		jenc->actx = NULL;
> +		spin_unlock_irqrestore(&jenc->hw_lock, flags);
> +
> +		if (ctx && ctx->bufq[JENC_DST_QUEUE].buff_id >= 0) {
[ ... ]
> +			jpeg_stop(jenc);
> +			jenc->enc_hw_irq_cb(ctx, VB2_BUF_STATE_DONE,
> +					    out_size + ctx->hdr_cache.layout.size);
> +		} else {
> +			jpeg_stop(jenc);
> +		}
> +	}
> +
> +	if (FIELD_GET(JPEG_V420_IRQ_STATUS_SESION_ERROR_MSK, irq_status)) {
> +		struct jenc_context *ctx;
> +
> +		spin_lock_irqsave(&jenc->hw_lock, flags);
> +		ctx = jenc->actx;
> +		jenc->actx = NULL;
> +		spin_unlock_irqrestore(&jenc->hw_lock, flags);
> +
> +		dev_err(jenc->dev, "encoder hardware failure=0x%lx\n",
> +			FIELD_GET(JPEG_V420_IRQ_STATUS_SESION_ERROR_MSK, irq_status));
> +
> +		jpeg_stop(jenc);
> +		if (ctx)
> +			jenc->enc_hw_irq_cb(ctx, VB2_BUF_STATE_ERROR, 0);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
What happens if both JPEG_V420_IRQ_STATUS_SESSION_DONE and
JPEG_V420_IRQ_STATUS_SESION_ERROR_MSK are set in irq_status?

The first block pops jenc->actx, completes the failed frame as
VB2_BUF_STATE_DONE, and schedules finish_work, which can start the next
job and set a new jenc->actx on another CPU; the second block then runs,
calls jpeg_stop(jenc) without dev_mutex, and aborts that newly started
job.  Checking JPEG_V420_IRQ_STATUS_SESION_ERROR_MSK first and using else
if for JPEG_V420_IRQ_STATUS_SESSION_DONE would avoid both completing a
failed frame and clobbering the next job.

[ ... ]
> +static unsigned long jpeg_calc_optimal_clk(struct qcom_jenc_dev *jenc,
> +					   u32 width, u32 height, u32 fps)
> +{
> +	unsigned long ref_pixel_rate;
> +	unsigned long optimal_clk;
> +	unsigned long pixel_rate;
> +
> +	if (!width || !height || !fps)
> +		return jenc->max_freq;
> +
> +	ref_pixel_rate = JPEG_REF_THROUGHPUT_MPPS * 1000000UL;
> +	pixel_rate = (unsigned long)width * height * fps;
> +
> +	optimal_clk = DIV_ROUND_UP_ULL(pixel_rate * JPEG_REF_CLK_HZ, ref_pixel_rate);
> +
> +	return min_t(unsigned long, jenc->max_freq, optimal_clk);
> +}

[Severity: Low]
Can pixel_rate * JPEG_REF_CLK_HZ overflow on 32-bit builds (such as
COMPILE_TEST) before DIV_ROUND_UP_ULL() widens the product to 64 bits?
Both pixel_rate and JPEG_REF_CLK_HZ have type unsigned long, so casting
one operand to u64 (or using mul_u32_u32()) is needed to avoid 32-bit
truncation.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v420_hw_info.h b/drivers/media/platform/qcom/jpeg/qcom_jenc_v420_hw_info.h
> new file mode 100644
> index 0000000000000..4fc5eb980c5f7
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v420_hw_info.h
[ ... ]
> +#define JPEG_V420_FE_CFG_SIXTEEN_MCU_EN		BIT(21)
> +#define JPEG_V420_FE_CFG_MCUS_PER_BLOCK		BIT(22)
> +#define JPEG_V420_FE_CFG_MAL_BOUNDARY		BIT(24)
> +#define JPEG_V420_FE_CFG_MAL_EN			BIT(27)
> +#define	JPEG_V420_FE_CFG_PLNS_EN_MSK		GENMASK(18, 16)
[ ... ]
> +/* WE_CFG fields */
> +#define JPEG_V420_WE_CFG_MEMORY_FORMAT_MSK	GENMASK(9, 8)
> +#define JPEG_V420_WE_CFG_CBCR_ORDER		BIT(12)
> +#define JPEG_V420_WE_CFG_PLN0_EN		BIT(16)
> +#define JPEG_V420_WE_CFG_MAL_BOUNDARY		BIT(24)
> +#define JPEG_V420_WE_CFG_MAL_EN			BIT(27)
> +#define JPEG_V420_WE_CFG_POP_BUFF_ON_EOS	BIT(28)

[Severity: Medium]
Should JPEG_V420_FE_CFG_MAL_BOUNDARY and JPEG_V420_WE_CFG_MAL_BOUNDARY be
GENMASK(26, 24) instead of BIT(24)?

enum jpeg_mal_bounds defines 3-bit values 0x0..0x7, and for valid widths
(256..8192) jpeg_get_mal_boundary() returns 0x3..0x7.  Passing BIT(24) to
jpeg_rw_bits() masks out bits [2:1] of the boundary value, programming the
MAL boundary as 32B or 64B instead of 256B..4KB.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> new file mode 100644
> index 0000000000000..b068a9d2322ed
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
[ ... ]
> +static void jpeg_v4l2_return_bufs(struct jenc_context *ctx, enum vb2_buffer_state buff_state)
> +{
> +	bool draining = ctx->fh.m2m_ctx->is_draining;
> +	struct vb2_v4l2_buffer *vb;
> +
> +	/* Drain CAPTURE queue; signal EOS on last buffer if V4L2_ENC_CMD_STOP. */\n
> +	while ((vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx))) {
> +		if (draining && !v4l2_m2m_last_dst_buf(ctx->fh.m2m_ctx)) {
> +			vb2_set_plane_payload(&vb->vb2_buf, 0, 0);
> +			v4l2_m2m_last_buffer_done(ctx->fh.m2m_ctx, vb);
> +		} else {
> +			v4l2_m2m_buf_done(vb, buff_state);
> +		}
> +	}
> +
> +	while ((vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx)))
> +		v4l2_m2m_buf_done(vb, buff_state);
> +}
[ ... ]
> +static void bop_jpeg_vb2_stop_streaming(struct vb2_queue *q)
> +{
> +	struct jenc_context *ectx = vb2_get_drv_priv(q);
> +	struct qcom_jenc_dev *jenc = ectx->jenc;
> +	bool do_release;
> +
> +	mutex_lock(&jenc->dev_mutex);
> +
> +	do_release = ectx->hw_acquired;
> +	ectx->hw_acquired = false;
> +
> +	mutex_unlock(&jenc->dev_mutex);
> +
> +	/*
> +	 * qcom_jenc_v420_release() calls jpeg_deinit() which issues HW_STOP and
> +	 * waits for the STOP_ACK interrupt completion. It runs before any buffer
> +	 * is returned below and outside dev_mutex: the threaded IRQ handler takes
> +	 * dev_mutex, and with IRQF_ONESHOT the STOP_ACK interrupt stays masked
> +	 * until that handler returns.
> +	 */
> +	if (do_release)
> +		qcom_jenc_v420_release(ectx);
> +
> +	mutex_lock(&jenc->dev_mutex);
> +	jpeg_v4l2_return_bufs(ectx, VB2_BUF_STATE_ERROR);
> +	v4l2_m2m_update_stop_streaming_state(ectx->fh.m2m_ctx, q);
> +	mutex_unlock(&jenc->dev_mutex);
> +}

[Severity: High]
Doesn't calling VIDIOC_STREAMOFF on one queue in
bop_jpeg_vb2_stop_streaming() fail all buffers on the other queue and
power down the hardware while the other queue is still streaming?

vb2_ops.stop_streaming() is called for a single vb2_queue q, but
jpeg_v4l2_return_bufs() removes and completes buffers from both CAPTURE
and OUTPUT (and if OUTPUT stops while draining with multiple CAPTURE
buffers queued, jpeg_v4l2_return_bufs() fails all but the last CAPTURE
buffer with VB2_BUF_STATE_ERROR and consumes is_draining before
v4l2_m2m_update_stop_streaming_state() runs).  Likewise,
jpeg_v4l2_work_stop() calls jpeg_v4l2_return_bufs() on a single job error,
draining every queued buffer in both queues instead of just the active
job's buffers.

In addition, ectx->hw_acquired is a single boolean cleared on the first
queue's stop_streaming(), so stopping one queue releases the hardware
reference and disables the IRQ while the sibling queue remains streaming.

[ ... ]
> +static int jpeg_v4l2_init_queue(void *priv, struct vb2_queue *sq, struct vb2_queue *dq)
> +{
> +	struct jenc_context *ectx = priv;
> +	int rc;
> +
> +	sq->drv_priv		= ectx;
> +	sq->dev			= ectx->dev;
> +	sq->type		= V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +	sq->io_modes		= VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> +	sq->buf_struct_size	= sizeof(struct v4l2_m2m_buffer);
> +	sq->ops			= &qcom_jpeg_v4l2_vb2_ops;
> +	sq->mem_ops		= &vb2_dma_sg_memops;
> +	sq->timestamp_flags	= V4L2_BUF_FLAG_TIMESTAMP_COPY;
> +	sq->lock		= &ectx->ctx_lock;
> +	sq->min_queued_buffers	= 1;

[Severity: Medium]
Does setting sq->lock and dq->lock to &ectx->ctx_lock while
jenc->vdev->lock is &jenc->dev_mutex leave format and encoder command
ioctls unserialized against queue ioctls?

In v4l2_ioctl_get_lock(), queue ioctls (INFO_FL_QUEUE: REQBUFS,
CREATE_BUFS, PREPARE_BUF, QBUF, STREAMON, STREAMOFF) lock
m2m_ctx->q_lock (&ectx->ctx_lock), whereas VIDIOC_S_FMT, VIDIOC_G_FMT, and
VIDIOC_ENCODER_CMD lock vdev->lock (&jenc->dev_mutex) and never take
ectx->ctx_lock:

CPU0                                  CPU1
v4l2_ioctl (VIDIOC_S_FMT)             v4l2_ioctl (VIDIOC_REQBUFS)
  mutex_lock(&jenc->dev_mutex)          mutex_lock(&ectx->ctx_lock)
  jpeg_v4l2_set_format()                bop_jpeg_vb2_queue_setup()
    jpeg_v4l2_queues_busy(ectx)           reads q->vf
    updates sq->vf, dq->vf                qcom_jenc_v420_queue_setup()
    qcom_jenc_v420_src_fmt_update()         jpeg_header_init(&ectx->hdr_cache)
      jpeg_header_init(&ectx->hdr_cache)      <-- data race on vf / hdr_cache

Similarly, iop_jpeg_encoder_command() calls v4l2_m2m_ioctl_encoder_cmd()
under jenc->dev_mutex while VIDIOC_QBUF and VIDIOC_STREAMOFF mutate the
same m2m_ctx draining state and rdy_queue under ectx->ctx_lock.

[ ... ]
> +static int fop_jpeg_file_open(struct file *file)
> +{
[ ... ]
> +	v4l2_ctrl_handler_init(&ectx->ctrl_hdl, 1);
> +	ectx->quality_ctl = v4l2_ctrl_new_std(&ectx->ctrl_hdl,
> +					      &qcom_jpeg_v4l2_ctrl_ops,
> +					      V4L2_CID_JPEG_COMPRESSION_QUALITY,
> +					      QCOM_JPEG_QUALITY_MIN,
> +					      QCOM_JPEG_QUALITY_MAX,
> +					      QCOM_JPEG_QUALITY_UNT,
> +					      QCOM_JPEG_QUALITY_DEF);
> +	if (ectx->ctrl_hdl.error) {
> +		rc = ectx->ctrl_hdl.error;
> +		goto err_fh_exit;
> +	}
> +
> +	ectx->fh.ctrl_handler = &ectx->ctrl_hdl;
> +
> +	rc = v4l2_ctrl_handler_setup(&ectx->ctrl_hdl);
> +	if (rc)
> +		goto err_ctrl_handler_free;
[ ... ]
> +	return 0;
> +
> +err_ctrl_handler_free:
> +	v4l2_ctrl_handler_free(&ectx->ctrl_hdl);
> +err_fh_exit:
> +	v4l2_fh_del(&ectx->fh, file);
> +	v4l2_fh_exit(&ectx->fh);

[Severity: Medium]
Does the ectx->ctrl_hdl.error path leak ectx->ctrl_hdl.buckets by jumping
to err_fh_exit instead of err_ctrl_handler_free after
v4l2_ctrl_handler_init()?

> +err_unlock_free:
> +	mutex_destroy(&ectx->quality_mutex);
> +	mutex_destroy(&ectx->ctx_lock);
> +	kfree(ectx);
> +
> +	return rc;
> +}
> +
> +static int fop_jpeg_file_release(struct file *file)
> +{
> +	struct jenc_context *ectx = jpeg_file2ctx(file);
> +	struct v4l2_m2m_dev *m2m_dev = ectx->fh.m2m_ctx->m2m_dev;
> +	struct qcom_jenc_dev *jenc = ectx->jenc;
> +	unsigned long flags;
> +
> +	/*
> +	 * Waits until a running job of this context completes: its
> +	 * SESSION_DONE interrupt needs jenc->actx, and finish_work must not
> +	 * be cancelled before, as it calls v4l2_m2m_job_finish().
> +	 */
> +	v4l2_m2m_ctx_release(ectx->fh.m2m_ctx);
> +
> +	/*
> +	 * Ensure the threaded IRQ handler cannot dereference this context
> +	 * after it is freed.  Clear jenc->actx under hw_lock before
> +	 * cancelling work.
> +	 */
> +	spin_lock_irqsave(&jenc->hw_lock, flags);
> +	if (jenc->actx == ectx)
> +		jenc->actx = NULL;
> +	spin_unlock_irqrestore(&jenc->hw_lock, flags);
> +
> +	cancel_work_sync(&ectx->finish_work);

[Severity: High]
Can jpeg_finish_work() dereference ectx->fh.m2m_ctx after
v4l2_m2m_ctx_release() frees it?

v4l2_m2m_ctx_release() only waits for TRANS_RUNNING to clear before
freeing m2m_ctx.  Once _v4l2_m2m_job_finish() clears TRANS_RUNNING and
wakes m2m_ctx->finished (or if a prior STREAMOFF already cleared
TRANS_RUNNING while finish_work was still pending), v4l2_m2m_ctx_release()
frees m2m_ctx before cancel_work_sync(&ectx->finish_work) is reached:

CPU0 (close)                          CPU1 (kworker)
fop_jpeg_file_release()               jpeg_finish_work()
  v4l2_m2m_ctx_release(m2m_ctx)         v4l2_m2m_job_finish(..., m2m_ctx)
    v4l2_m2m_cancel_job()                 _v4l2_m2m_job_finish()
      wait_event(m2m_ctx->finished)         clears TRANS_RUNNING
                                            wake_up(&m2m_ctx->finished)
    kfree(m2m_ctx)                        v4l2_m2m_schedule_next_job(..., m2m_ctx)
  ...                                       <-- use-after-free of m2m_ctx
  cancel_work_sync(&ectx->finish_work)

Since v4l2_m2m_job_finish() already defers the next job via
schedule_work(&m2m_dev->job_work), calling v4l2_m2m_job_finish() directly
in jpeg_v4l2_work_done() and jpeg_v4l2_work_stop() would avoid needing
finish_work altogether.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106082522.1448390-1-atanas.filipov@oss.qualcomm.com?part=4