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 <[email protected]> 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/[email protected]?part=5
