Re: [PATCH 04/11] drm/panthor: Fix potential invalid pointer deref in group_process_tiler_oom()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `group->fatal_queues` is modified without any locking in `group_process_tiler_oom()`, leading to a data race with other contexts that update it, and `group->fatal_lock` is completely unused.
- [High] Firmware render pass counter validation check is vulnerable to 32-bit integer wrap-around, which can erroneously reject valid memory allocations.
--

commit 849043d96182c6a00588e13d1652d042b8e7a787
Author: Boris Brezillon <boris.brezillon@collabora.com>
drm/panthor: Fix potential invalid pointer deref in group_process_tiler_oom()

This commit prevents a potential invalid pointer dereference when
panthor_heap_pool_put() is called with an error pointer by ensuring heaps is
set to NULL on failure.

Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260625-panthor-signal-from-irq-v5-0-8836a74e0ef9@collabora.com?part=2

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index e97f29469d28a..8fd4d97b062e0 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1600,7 +1600,10 @@ static int group_process_tiler_oom(struct panthor_group *group, u32 cs_id)
>  	if (unlikely(csg_id < 0))
>  		return 0;
>  
> -	if (IS_ERR(heaps) || frag_end > vt_end || vt_end >= vt_start) {
> +	if (IS_ERR(heaps)) {
> +		ret = -EINVAL;
> +		heaps = NULL;
> +	} else if (frag_end > vt_end || vt_end >= vt_start) {
                                        ^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does this condition incorrectly
handle 32-bit unsigned wrap-arounds for the FW pass counters?

If a long-running graphical application issues enough render passes, the 32-bit
counters can wrap around. For example, if vt_start = 0x00000001 and
vt_end = 0xFFFFFFFF, the direct comparison vt_end >= vt_start evaluates to
true even though vt_start is logically ahead.

This causes valid tiler OOM requests to fail with -EINVAL, marking the
affected queue as fatal.

Should sequence arithmetic like (s32)(vt_end - vt_start) >= 0 be used here
instead to safely compare wrapping sequence numbers?

>  		ret = -EINVAL;
>  	} else {
>  		/* We do the allocation without holding the scheduler lock to avoid

[Severity: High]
This is a pre-existing issue, but is there a data race when updating
group->fatal_queues later in this same function?

In group_process_tiler_oom(), which executes in the heap_alloc_wq workqueue,
the fatal_queues bitmask is updated without holding any locks:

drivers/gpu/drm/panthor/panthor_sched.c:group_process_tiler_oom() {
    ...
    if (ret && ret != -ENOMEM) {
        drm_warn(&ptdev->base, "Failed to extend the tiler heap\n");
        group->fatal_queues |= BIT(cs_id);
    ...
}

Concurrently, cs_slot_process_fatal_event_locked() updates this same field
from process_fw_events_work in the sched->wq workqueue. Because |= is a
non-atomic read-modify-write operation, concurrent execution could result
in lost updates, potentially leaving a queue that experienced a fatal error
incorrectly marked as healthy.

Additionally, panthor_group defines spinlock_t fatal_lock to protect access
to fatal fields, but this lock appears to be completely unused.

Could fatal_lock be used here to prevent the race?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-panthor-misc-fixes-v1-0-b67ed973fea6@collabora.com?part=4