Re: [PATCH v7 6/9] media: chips-media: wave6: Improve debugging capabilities

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Lockless call to `vb2_get_num_buffers()` in debugfs read leads to use-after-free on `q->bufs_bitmap`.
- [Medium] Multiplication can overflow 32-bit bounds before assignment to a 64-bit variable in the debugfs FPS calculation.
- [Low] Direct use of 64-bit division and modulo operators (`/` and `%`) will cause build failures on 32-bit architectures.

Pre-existing issues:
- [High] Unprotected iteration over V4L2 mem2mem ready queues without holding `rdy_spinlock` leads to list corruption.
--

commit a789f4b587c983369a5ba74d2b58395c60628047
Author: Nas Chung <nas.chung@chipsnmedia.com>

media: chips-media: wave6: Improve debugging capabilities

This commit adds debugfs entries and trace events to provide detailed
debugging information for the Wave6 core driver.

Tested-by: Ming Qian <ming.qian@oss.nxp.com>
Tested-by: Marek Vasut <marek.vasut@mailbox.org>

> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpu-dbg.c b/drivers/media/platform/chips-media/wave6/wave6-vpu-dbg.c
> new file mode 100644
> index 0000000000000..ae71edeae0888
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpu-dbg.c
[ ... ]
> +	vq = v4l2_m2m_get_src_vq(inst->v4l2_fh.m2m_ctx);
> +	seq_printf(s, "output (%2d, %2d): fmt = %c%c%c%c %d x %d, %d;\n",
> +		   vb2_is_streaming(vq),
> +		   vb2_get_num_buffers(vq),

[Severity: High]
Does this locklessly access `q->bufs_bitmap`? Concurrently, an application
can execute `VIDIOC_REQBUFS` with count=0, which acquires the queue lock,
frees `q->bufs_bitmap`, and sets the pointer to NULL. Because this read
doesn't hold `m2m_ctx->q_lock` or equivalent synchronization, it could read
a non-NULL pointer right before it is freed by `REQBUFS`, executing
`bitmap_weight()` on a freed pointer and causing a use-after-free.

> +		   inst->src_fmt.pixelformat,
[ ... ]
> +	vq = v4l2_m2m_get_dst_vq(inst->v4l2_fh.m2m_ctx);
> +	seq_printf(s, "capture(%2d, %2d): fmt = %c%c%c%c %d x %d, %d;\n",
> +		   vb2_is_streaming(vq),
> +		   vb2_get_num_buffers(vq),

[Severity: High]
This appears to have the same lockless access concern as the source queue
above. Does this need queue lock protection to prevent a use-after-free
if `VIDIOC_REQBUFS` is called concurrently?

> +		   inst->dst_fmt.pixelformat,
[ ... ]
> +	seq_puts(s, "fps");
> +	tmp = MSEC_PER_SEC * inst->processed_buf_num;

[Severity: Medium]
Will this operation overflow on 32-bit architectures? `MSEC_PER_SEC` is
typically defined as `1000L` (a 32-bit signed integer on 32-bit systems) and
`inst->processed_buf_num` is a `u32`. If the buffer count exceeds ~4.29
million, the 32-bit multiplication might overflow before it is assigned and
sign-extended into the 64-bit `tmp` variable.

> +	if (perf->ts_last > perf->ts_first + NSEC_PER_MSEC) {
> +		fps = DIV_ROUND_CLOSEST(tmp, (perf->ts_last - perf->ts_first) / NSEC_PER_MSEC);

[Severity: Low]
Does this cause a build failure on 32-bit architectures? `DIV_ROUND_CLOSEST`
internally expands to a division operation (`/`), and the denominator here
also uses direct division. Using direct 64-bit division on `s64` variables
emits calls to undefined functions like `__divdi3` on 32-bit systems. Should
explicit 64-bit math helpers like `div64_s64()` be used here instead?

> +		seq_printf(s, " actual: %lld;", fps);
> +	}
> +	duration = perf->total_sw_time / NSEC_PER_MSEC;

[Severity: Low]
Similar to the above, this performs direct 64-bit division on a `u64`
variable, which can break 32-bit builds due to missing math symbols.

> +	if (duration > 0) {
> +		fps = DIV_ROUND_CLOSEST(tmp, duration);
> +		seq_printf(s, " sw: %lld;", fps);
> +	}
> +	duration = perf->total_hw_time / NSEC_PER_MSEC;

[Severity: Low]
Another instance of direct 64-bit division that might break 32-bit builds.

> +	if (duration > 0) {
> +		fps = DIV_ROUND_CLOSEST(tmp, duration);
> +		seq_printf(s, " hw: %lld", fps);
> +	}
> +	seq_putc(s, '\n');
> +
> +	seq_printf(s, "latency(ms) first: %llu.%06llu, max %llu.%06llu, setup %llu.%06llu\n",
> +		   perf->latency_first / NSEC_PER_MSEC,
> +		   perf->latency_first % NSEC_PER_MSEC,

[Severity: Low]
This performs direct 64-bit division and modulo on an `s64` variable, which
is known to cause linking errors (like `__moddi3` and `__moddi3`) on 32-bit
architectures. Should this use explicit math helpers like `div64_s64_rem()`?


[Severity: High]
This is a pre-existing issue, but the series iterates over the V4L2 m2m ready
queues locklessly using `v4l2_m2m_for_each_src_buf` and
`v4l2_m2m_for_each_dst_buf` without holding `rdy_spinlock`.

For example, in drivers/media/platform/chips-media/wave6/wave6-vpu-enc.c:

wave6_get_valid_src_buf() {
    ...
    v4l2_m2m_for_each_src_buf(inst->v4l2_fh.m2m_ctx, v4l2_m2m_buf) {
    ...
}

And in drivers/media/platform/chips-media/wave6/wave6-vpu-v4l2.c:

wave6_get_dst_buf_by_addr() {
    ...
    v4l2_m2m_for_each_dst_buf(inst->v4l2_fh.m2m_ctx, v4l2_m2m_buf) {
    ...
}

Are these list traversals vulnerable to data races? These macros wrap
`list_for_each_entry`, and they appear to be called from the m2m framework's
worker thread without holding `rdy_spinlock`. Concurrently, userspace can
call `VIDIOC_QBUF`, which dynamically adds elements to the `rdy_queue` while
holding the lock. Modifying a linked list while a separate thread traverses
it unprotected can result in structural data races and list pointer
corruption, potentially leading to a use-after-free or kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788496816.git.nas.chung@chipsnmedia.com?part=6