Re: [PATCH v4 05/12] drm/panthor: Add support for entering and exiting protected mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] sched: data race and UAF on sched->protm.active_group during reset
- [Medium] mmu: delayed_update_mask is never cleared, causing permanent cache flushes
- [Medium] drm/panthor: UAPI loses -ERESTARTSYS on signal interruption during protm exit
- [Low] cleanup: mixing goto-based error handling with scoped_cond_guard()
--

commit d3563f471c349ec35f609214f5443133a617a0b3
Author: Florent Tomasin <florent.tomasin@arm.com>

drm/panthor: Add support for entering and exiting protected mode

This patch modifies the Panthor driver code to allow handling of the GPU HW
protected mode enter and exit. It introduces the mechanisms needed for
entering and exiting protected mode, handling of protected mode IRQs,
scheduler changes, and GPU fault handling.

> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -2626,6 +2690,7 @@ static int remap_evicted_vma(struct drm_gpuvm_bo *vm_bo,
>  		break;
>  	}
>  
> -	if (found) {
> +	if (!found)
> +		goto out_unlock;
> +
> +	scoped_cond_guard(protm_block_timeout, ret = -ETIMEDOUT, vm) {

[Severity: Low]
Does this mix goto-based error handling with scoped guards?

The function uses both goto labels like out_unlock and scoped_cond_guard. The
kernel guidelines advise against mixing goto-based cleanup and scope-based
cleanup helpers in the same function.

[Severity: Medium]
Does this lose the -ERESTARTSYS return value?

The wait function panthor_fw_protm_exit_wait() correctly returns -ERESTARTSYS
if interrupted by a signal, but panthor_sched_protm_block_timeout() converts
this error to a boolean. The guard macro here then hardcodes ret = -ETIMEDOUT.
If the userspace thread is interrupted, it incorrectly receives a timeout error
instead of standard signal interruption semantics.

>  		vm->op_ctx = op_ctx;
> +
>  		ret = panthor_vm_lock_region(vm, evicted_vma->base.va.addr,
[ ... ]
> @@ -2726,39 +2781,42 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
>  	mutex_lock(&vm->op_lock);
>  	vm->op_ctx = op;
>  
> -	ret = panthor_vm_lock_region(vm, op->va.addr, op->va.range);
> -	if (ret)
> -		goto out;
> -
> -	switch (op_type) {
[ ... ]
> +	scoped_cond_guard(protm_block_timeout, ret = -ETIMEDOUT, vm) {
> +		ret = panthor_vm_lock_region(vm, op->va.addr, op->va.range);
> +		if (ret)
> +			goto out;

[Severity: Low]
Is it safe to use a goto inside a scoped guard block when the function relies
on a mix of scope-based and goto-based cleanup?

[Severity: Medium]
Will signal interruptions incorrectly return -ETIMEDOUT here?

The boolean guard panthor_sched_protm_block_timeout discards the specific
-ERESTARTSYS error from panthor_fw_protm_exit_wait(), and this macro hardcodes
a timeout error on failure.

[ ... ]
> @@ -3517,3 +3575,54 @@ void panthor_mmu_pt_cache_fini(void)
[ ... ]
> +void panthor_mmu_protm_exit_event(struct panthor_device *ptdev)
> +{
[ ... ]
> +	guard(mutex)(&ptdev->mmu->as.slots_lock);
> +
> +	ptdev->mmu->as.delay_update_mask = 0;
> +
> +	if (!ptdev->mmu->as.delayed_update_mask)
> +		return;
> +
> +	ret = panthor_gpu_flush_caches(ptdev, CACHE_CLEAN | CACHE_INV,

[Severity: Medium]
Is it possible that delayed_update_mask is never cleared back to 0?

The code checks delayed_update_mask to decide whether to flush the cache,
but it only clears delay_update_mask. Leaving delayed_update_mask non-zero
could trigger a full GPU cache flush on every exit from protected mode.

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2484,6 +2484,79 @@ static void group_term_work(struct work_struct *work)
[ ... ]
> +bool panthor_sched_protm_block_timeout(struct panthor_device *ptdev,
> +				      struct panthor_vm *vm)
> +{
[ ... ]
> +	/* We do not want this function to fail, because that can lead to
> +	 * innocent processes failing. That is why we wait up to 2x the job
> +	 * timeout for GPU to exit protm here. This shall cover the worst case
> +	 * where protm work hangs and we must rely on job timeout to recover.
> +	 */
> +	ret = panthor_fw_protm_exit(ptdev, JOB_TIMEOUT_MS << 1);
> +	if (ret)
> +		up_read(&sched->protm.lock);
> +
> +	return ret ? false : true;
> +}

[Severity: Medium]
Does returning a boolean here discard the specific error code from
panthor_fw_protm_exit?

If panthor_fw_protm_exit_wait() returns -ERESTARTSYS, the boolean return type
prevents propagating it to userspace, which breaks standard syscall restart
semantics.

[ ... ]
> @@ -3094,6 +3507,11 @@ void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed)
>  
>  	mutex_lock(&sched->reset.lock);
>  
> +	if (sched->protm.active_group) {
> +		group_put(sched->protm.active_group);
> +		sched->protm.active_group = NULL;
> +	}
> +

[Severity: High]
Does this modification of active_group without holding the proper
rw_semaphore introduce a data race?

The reader side panthor_sched_protm_block_timeout() checks and dereferences
sched->protm.active_group->vm while holding down_read(&sched->protm.lock).

Because panthor_sched_post_reset() sets active_group to NULL and drops its
reference without acquiring down_write(&sched->protm.lock), the group could
be freed out from under the ioctl thread causing a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012125940.3291951-1-ketil.johnsen@arm.com?part=5