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 <[email protected]> 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: [email protected] Closes: https://sashiko.dev/#/patchset/[email protected]?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/[email protected]?part=4
