Re: [PATCH v7 6/8] drm/panthor: Implement sample handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] drm: Integer overflow in user_sample_size bypasses BO size validation
- [High] drm: 64-bit division by variable size_t breaks 32-bit builds
- [High] workqueue: CPU hog due to immediate requeueing in panthor_perf_sample_worker during reset
- [High] drm: Unbounded loop in panthor_perf_handle_fw_samples allows firmware to hang kernel
- [High] drm: Incompatible pointer type in bitmap_from_arr64 corrupts memory on 32-bit big-endian
- [High] drm: panthor_perf_pool_destroy leaks session pool and PM references on unplug
- [High] drm: panthor_perf_session_destroy fails to re-enable global FW sampling
- [High] drm: Missing memory barriers in FW ringbuffer communication
- [High] drm: sleeping lock in wait_event_timeout condition triggers might_sleep() warning
- [High] drm/panthor: UAF and NULL-deref when device unplug races with file close
- [Medium] drm: Timestamp accumulation zeroed out by skipped blocks
- [Medium] drm: Undefined behavior in BLK_MASK macro when argument is zero
- [Medium] drm/panthor: memory and pm runtime leak in panthor_perf_pool_destroy()
--

commit 3f117df9269c8346ebf5d90d19e892acc18e5210
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panthor: Implement sample handling

Adds performance counter sample handling for the Panthor driver. It
introduces a ring buffer for firmware-to-kernel sample communication,
handles interrupts for new samples or overflows, and accumulates partial
hardware samples into a user-facing buffer.

> diff --git a/drivers/gpu/drm/panthor/panthor_perf.c b/drivers/gpu/drm/panthor/panthor_perf.c
> index 76c3469811508..c66ad658a95db 100644
> --- a/drivers/gpu/drm/panthor/panthor_perf.c
> +++ b/drivers/gpu/drm/panthor/panthor_perf.c
[ ... ]
>  static void panthor_perf_handle_fw_samples(struct panthor_device *ptdev,
>  					   u32 extract_idx, u32 insert_idx)
>  {
>  	struct panthor_perf *perf = ptdev->perf;
>  	const size_t ann_block_size =
>  		get_annotated_block_size(ptdev->perf_info.counters_per_block);
>  	u32 i;
>  
>  	for (i = extract_idx; i != insert_idx; i++) {

[Severity: High]
Are there any bounds checks on extract_idx and insert_idx? Since these
come directly from untrusted firmware shared memory, could a malicious or
buggy firmware return a bogus insert_idx (e.g., extract_idx + U32_MAX)
that forces this loop to execute billions of times, permanently stalling the
kernel worker thread?

[ ... ]
> +#define BLK_MASK(x) GENMASK_ULL((x) - 1, 0)
> +
> +static u64 get_sc_mask(struct panthor_device *ptdev)
> +{
[ ... ]
> +static int panthor_perf_setup_fw_buffer(struct panthor_device *ptdev, struct panthor_perf *perf)
> +{
[ ... ]
> +	const struct panthor_perf_fw_block blocks[DRM_PANTHOR_PERF_BLOCK_MAX] = {
> +		[DRM_PANTHOR_PERF_BLOCK_METADATA] = {
> +			.block_count = has_metadata ? 1 : 0,
> +			.phys_mask = has_metadata ? BIT_ULL(0) : 0,
> +		},
> +		[DRM_PANTHOR_PERF_BLOCK_FW] = {
> +			.block_count = info->fw_blocks,
> +			.phys_mask = info->fw_blocks ? BLK_MASK(info->fw_blocks) : 0,
> +		},
> +		[DRM_PANTHOR_PERF_BLOCK_CSHW] = {
> +			.block_count = info->cshw_blocks,
> +			.phys_mask = BLK_MASK(info->cshw_blocks),
> +		},

[Severity: Medium]
If any hardware capabilities (like info->cshw_blocks, tiler_blocks, or
memsys_blocks) have a count of 0, does BLK_MASK(0) expand to
GENMASK_ULL(0 - 1, 0)? Passing -1 (or ULLONG_MAX) as the high bit
argument to GENMASK_ULL triggers an invalid shift and undefined behavior.
Should these usages have a ternary guard for zero similar to how
info->fw_blocks is protected?

[ ... ]
> +static void session_accumulate_sample(struct panthor_device *ptdev,
> +				      struct panthor_perf_session *session,
> +				      u8 *session_sample, u8 *perf_sample)
> +{
> +	const struct drm_panthor_perf_info *const perf_info = &ptdev->perf_info;
> +	struct panthor_perf_enable_masks *em = session->enabled_counters;
> +	struct panthor_perf_sample_block_iter iter;
> +	const u32 counters = perf_info->counters_per_block;
> +
> +	sample_block_iter_init(perf_info, perf_sample, session_sample, &iter);
> +
> +	while (sample_block_iter_next(&iter)) {
> +		DECLARE_BITMAP(enabled_ctrs, PANTHOR_PERF_EM_BITS);
> +		const struct panthor_perf_counter_block *src_blk = iter.src_blk;
> +		struct panthor_perf_counter_block *dst_blk = iter.dst_blk;
> +
> +		bitmap_from_arr64(enabled_ctrs, em->mask[dst_blk->header.block_type],
> +				  PANTHOR_PERF_EM_BITS);

[Severity: High]
bitmap_from_arr64() expects a const u64 * source, but em->mask[type]
is declared as an unsigned long array. On 32-bit big-endian architectures,
reading a 64-bit integer from a pair of 32-bit unsigned long values
effectively swaps the two words. Will this silently scramble the entire
enable mask?

> +		bitmap_clear(enabled_ctrs, 0, PANTHOR_HEADER_COUNTERS);
> +
> +		dst_blk->counters[PANTHOR_CTR_TIMESTAMP_HI] =
> +			src_blk->counters[PANTHOR_CTR_TIMESTAMP_HI];
> +		dst_blk->counters[PANTHOR_CTR_TIMESTAMP_LO] =
> +			src_blk->counters[PANTHOR_CTR_TIMESTAMP_LO];

[Severity: Medium]
Because the intermediate buffer perf->sample is completely zeroed out at
the end of each worker run, if the firmware did not produce a sample for a
given block in the current batch (prfcnt_en == 0), its block remains all
zeros.

Since this unconditionally copies timestamps from the intermediate block,
will this overwrite the session's previously accumulated valid timestamp
with zero?

[ ... ]
> +static void session_copy_sample(struct panthor_device *ptdev,
> +				struct panthor_perf_session *session, u32 flags)
> +{
[ ... ]
> +	new_sample = session->samples + (insert_idx % session->ringbuf_slots) * sample_size;

[Severity: High]
Because insert_idx is a u64 and session->ringbuf_slots is a size_t,
does this perform a 64-bit by 32-bit modulo operation on 32-bit
architectures? This can result in a link-time error (__umoddi3 undefined).
Since ringbuf_slots is guaranteed to be a power of two during creation,
could this be optimized to a bitwise AND
(insert_idx & (session->ringbuf_slots - 1)) to avoid the division entirely?

[ ... ]
> +static void panthor_perf_sample_worker(struct work_struct *work)
> +{
> +	struct panthor_perf *perf = container_of(work, struct panthor_perf, sample_work);
> +	struct panthor_device *const ptdev = perf->ptdev;
> +	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> +	const u32 extract_idx = READ_ONCE(glb_iface->input->perfcnt_extract);
> +	const u32 insert_idx = READ_ONCE(glb_iface->output->perfcnt_insert);
> +	const u32 ack = READ_ONCE(glb_iface->output->ack);
> +	const u32 req = READ_ONCE(glb_iface->input->req);
> +	const u32 events = (req ^ ack) & (GLB_PERFCNT_THRESHOLD | GLB_PERFCNT_OVERFLOW);
> +	const u32 flags = events & GLB_PERFCNT_OVERFLOW ? DRM_PANTHOR_PERF_SAMPLE_OVERFLOW : 0;
> +
> +	/* Defer sample processing until the pending reset completes. */
> +	if (panthor_device_reset_is_pending(ptdev)) {
> +		queue_work(perf->sample_workq, &perf->sample_work);
> +		return;
> +	}

[Severity: High]
Since sample_workq is created with WQ_HIGHPRI | WQ_UNBOUND, does
immediately re-queueing the work create a tight busy-loop? Could this pin a
CPU at 100% utilization until the reset completes, potentially starving the
reset handler itself? Would it be safer to use queue_delayed_work() or
rely on the reset handler to kick the queue?

> +
> +	/* A stale notification must not complete a userspace sample request. */
> +	if (extract_idx == insert_idx)
> +		goto ack_events;
> +
> +	panthor_perf_handle_fw_samples(ptdev, extract_idx, insert_idx);
> +	WRITE_ONCE(glb_iface->input->perfcnt_extract, insert_idx);

[Severity: High]
Is an smp_rmb() needed before parsing the payload in
panthor_perf_handle_fw_samples()? The architecture's weakly-ordered memory
model might permit the CPU to speculatively load sample data before the
insert_idx read guarantees the data is fully written.

Similarly, is an smp_wmb() required before the WRITE_ONCE updates
perfcnt_extract, to ensure all payload reads complete before the firmware
reclaims the slots?

[ ... ]
> +static int panthor_perf_request_sync(struct panthor_perf *perf,
> +				     struct panthor_perf_session *session, session_state_cb cb)
> +{
> +	int ret;
> +
> +	perf_fw_request_sample(perf);
> +
> +	ret = wait_event_timeout(session->state_wq, cb(session),
> +				 msecs_to_jiffies(1000));

[Severity: High]
wait_event_timeout() explicitly sets the task state to
TASK_UNINTERRUPTIBLE before evaluating its condition. The condition
callbacks (like session_is_stopped_locked and
session_is_active_locked) unconditionally acquire a sleeping lock
(mutex) via guard(mutex).

Does calling a sleeping lock while the task state is not TASK_RUNNING
violate kernel scheduling rules and trigger might_sleep() kernel warnings
every time the condition is checked?

Furthermore, if the mutex is contended, the slowpath overwrites the task
state back to TASK_RUNNING. When the condition callback returns false,
schedule_timeout() is called with TASK_RUNNING, causing it to return
immediately without sleeping and create a busy-wait loop.

[ ... ]
> +int panthor_perf_session_create(struct drm_file *file,
> +				struct drm_panthor_perf_session_create *create_args,
> +				unsigned long *sid)
> +{
[ ... ]
> +	user_sample_size = ptdev->perf_info.sample_size * slots;
> +
> +	ret = panthor_perf_bo_get(file, create_args->ringbuf_handle,
> +				  user_sample_size, &ringbuffer);

[Severity: Critical]
Userspace provides create_args->sample_slots, which is validated only by
is_power_of_2(). Could slots be large enough that multiplying it by
ptdev->perf_info.sample_size overflows the 32-bit size_t
user_sample_size on 32-bit platforms, wrapping it to 0?

If it wraps to 0, panthor_perf_bo_get() bypasses size validation entirely
because of the "if (expected_size && ...)" check. Does this bind a massive
slots count to a maliciously small GEM BO, allowing out-of-bounds memory
corruption when the worker emits data?

[ ... ]
> +int panthor_perf_session_destroy(struct panthor_file *pfile, u32 handle)
> +{
[ ... ]
> +		xa_erase(&pfile->sessions->xa, handle);
> +		ret = perf_disable(perf);
> +
> +		scoped_guard(mutex, &perf->session_lock)
> +			perf_detach_session(perf, session);
> +	}
>  
>  	session_put(session);

[Severity: High]
After stopping firmware sampling via perf_disable(perf) and detaching the
session, this function neglects to call perf_enable(perf). If a single
session is destroyed by userspace, does this permanently stop firmware
sampling globally and starve all remaining active sessions of data?

[ ... ]
>  void panthor_perf_unplug(struct panthor_device *ptdev)
>  {
> +	int ret;
>  	struct panthor_perf *perf = ptdev->perf;
>  
>  	if (!perf)
>  		return;
>  
> +	scoped_guard(mutex, &perf->op_lock)
> +		ret = perf_disable(perf);
> +	if (ret)
> +		drm_err(&perf->ptdev->base, "Failed to disable performance counter sampling!");
> +
> +	disable_work_sync(&perf->sample_work);
> +
> +	panthor_kernel_bo_vunmap(perf->rb);
> +	panthor_kernel_bo_destroy(perf->rb);
> +
>  	ptdev->perf = NULL;
>  }

[Severity: High]
If panthor_device_unplug() runs concurrently with the closing of a DRM
file descriptor containing multiple sessions, could panthor_postclose()
safely bypass ioctl synchronization?

If panthor_perf_unplug() frees perf->rb and clears ptdev->perf to
NULL while panthor_perf_pool_destroy() is iterating, could xa_for_each()
either pass a NULL pointer to panthor_perf_session_remove() (causing a
panic) or call perf_enable() on the freed perf->rb, triggering a
use-after-free when writing its virtual address to the device register?

[ ... ]
>  void panthor_perf_pool_destroy(struct panthor_file *pfile)
>  {
>  	struct panthor_perf_pool *pool = pfile->sessions;
>  	unsigned long handle;
>  	struct panthor_perf_session *session;
>  
>  	if (IS_ERR_OR_NULL(pool))
>  		return;
>  
> +	if (!pfile->ptdev->perf)
> +		return;
> +
>  	xa_for_each(&pool->xa, handle, session) {
>  		xa_erase(&pool->xa, handle);
> +		panthor_perf_session_remove(pfile->ptdev->perf, session);
>  		session_put(session);
> +		pm_runtime_put_sync(pfile->ptdev->base.dev);
>  	}
>  
>  	xa_destroy(&pool->xa);

[Severity: Medium]
If panthor_device_unplug() has already cleared ptdev->perf to NULL,
this early return completely bypasses the xa_for_each() cleanup loop.

Does this result in a persistent memory leak of the xarray, all active
panthor_perf_session objects, the pool structure itself, and the
associated Power Management (PM) runtime references taken during session
creation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-panthor-performance-counters-v7-0-12c32495062f@arm.com?part=6