Re: [PATCH v5 02/16] drm/panthor: Don't overrule pending immediate ticks in sched_resume_tick()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Dereference of ERR_PTR in panthor_heap_pool_put() during tiler OOM handling.
- [High] Asynchronous cleanup of scheduling groups executes without holding a DRM device reference, leading to a Use-After-Free of panthor_device.
- [High] Missing bounds check on synchronization object offset allows out-of-bounds read.
--

commit eccb99f3f3029758d8c3f80d0f349c280e91b3c2
Author: Boris Brezillon <boris.brezillon@collabora.com>

drm/panthor: Don't overrule pending immediate ticks in sched_resume_tick()

We schedule immediate ticks when we need to process events on CSGs,
but those immediate ticks don't change the resched_target because we
want the other groups to stay scheduled for the remaining of the GPU
timeslot they were given. Make sure these immediate ticks don't get
overruled by a sched_queue_delayed_work() that would delay the tick
execution.

Fixes: 99820b4b7e50 ("drm/panthor: Make sure we resume the tick when new jobs are submitted")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260625-panthor-signal-from-irq-v4-0-3d2908912afa@collabora.com?part=9

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 5b34032deff81..1913bc8a62977 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2668,7 +2668,14 @@ static void sched_resume_tick(struct panthor_device *ptdev)
>  	else
>  		delay_jiffies = 0;
>  
> -	sched_queue_delayed_work(sched, tick, delay_jiffies);
> +	/* We schedule immediate ticks when we need to process events on CSGs,
> +	 * but those don't change the resched_target because we want the other
> +	 * groups to stay scheduled for the remaining of the GPU timeslot they
> +	 * were given. Make sure those immediate ticks don't get overruled by
> +	 * a sched_queue_delayed_work() that would delay the tick execution.
> +	 */
> +	if (!delayed_work_pending(&sched->tick_work))
> +		sched_queue_delayed_work(sched, tick, delay_jiffies);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but we noticed a potential
out-of-bounds read nearby. In panthor_queue_get_syncwait_obj(), the
return value of panthor_gem_sync() is ignored:

drivers/gpu/drm/panthor/panthor_sched.c:panthor_queue_get_syncwait_obj() {
    ...
	panthor_gem_sync(&bo->base,
			 DRM_PANTHOR_BO_SYNC_CPU_CACHE_FLUSH_AND_INVALIDATE,
			 queue->syncwait.offset,
			 queue->syncwait.sync64 ?
			 sizeof(struct panthor_syncobj_64b) :
			 sizeof(struct panthor_syncobj_32b));

	return queue->syncwait.kmap + queue->syncwait.offset;
}

If panthor_gem_sync() detects the offset is out of bounds and returns
-EINVAL, the function ignores the error and still returns the
pointer, which points outside the buffer boundary.

Could this allow panthor_queue_eval_syncwait() to dereference an
out-of-bounds pointer when it reads syncobj->sync64.seqno?

[Severity: High]
This is a pre-existing issue, but is it safe for group_release() to queue
asynchronous cleanup work without pinning the DRM device?

drivers/gpu/drm/panthor/panthor_sched.c:group_release() {
	...
	queue_work(panthor_cleanup_wq, &group->release_work);
}

When the DRM device file is closed and panthor_postclose() executes group
destruction, it doesn't flush the panthor_cleanup_wq workqueue. If the
device is hot-unplugged, the DRM core might free the panthor_device.

When group_release_work() eventually executes later, does this lead to a
use-after-free when it accesses the freed panthor_device during cleanup
(e.g., in panthor_kernel_bo_destroy())?

[Severity: High]
This is a pre-existing issue, but we noticed a potential pointer dereference
issue in the tiler OOM handling path. In group_process_tiler_oom():

drivers/gpu/drm/panthor/panthor_sched.c:group_process_tiler_oom() {
    ...
	heaps = panthor_vm_get_heap_pool(group->vm, false);
    ...
	if (IS_ERR(heaps) || frag_end > vt_end || vt_end >= vt_start) {
		ret = -EINVAL;
    ...
out_put_heap_pool:
	panthor_heap_pool_put(heaps);
	return ret;
}

If heaps is an ERR_PTR (like ERR_PTR(-ENOENT)), the code correctly sets
ret = -EINVAL but later jumps to out_put_heap_pool and calls
panthor_heap_pool_put().

Since panthor_heap_pool_put() only checks if (pool) and doesn't check
IS_ERR(pool), wouldn't this try to dereference an invalid error pointer
in kref_put()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-panthor-signal-from-irq-v5-0-8836a74e0ef9@collabora.com?part=2