Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Concurrent ioctl calls to MSM_PARAM_SYSPROF can cause unbalanced pm_runtime and sysprof_active refcounts. - [Medium] TOCTOU race in MSM_PARAM_EN_VM_BIND handler leading to mismatched VM state. -- --- Patch [5]: [PATCH v4 5/6] drm/msm: Fix per-process-pgtables check --- commit 445f3f992eb83cba3cf22345c8afc2a87202a7ce Author: Rob Clark <[email protected]> Subject: drm/msm: Fix per-process-pgtables check This patch fixes the per-process-pgtable check in adreno_set_param() by verifying create_private_vm instead of ctx->vm, as ctx->vm is not yet initialized at this point. > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index c62c45bb0ddbb..bff2a773c0d4e 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -505,7 +505,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct > msm_context *ctx, > return msm_context_set_sysprof(ctx, gpu, value); [Severity: High] This isn't a bug introduced by this patch, but does calling msm_context_set_sysprof() here without locks allow a race condition? If two threads concurrently call MSM_PARAM_SYSPROF to transition from 2 to 0, couldn't they both read ctx->sysprof == 2 in msm_context_set_sysprof() and both call pm_runtime_put_autosuspend() and refcount_dec()? This might underflow the pm_runtime refcount, causing premature hardware suspension. > case MSM_PARAM_EN_VM_BIND: > /* We can only support VM_BIND with per-process pgtables: */ > - if (ctx->vm == gpu->vm) > + if (!gpu->funcs->create_private_vm) > return UERR(EINVAL, drm, "requires per-process > pgtables"); > > /* > * We can only swtich to VM_BIND mode if the VM has not yet > * been created: > */ > if (ctx->vm) > return UERR(EBUSY, drm, "VM already created"); > > ctx->userspace_managed_vm = value; [Severity: Medium] This is a pre-existing issue, but since the check if (ctx->vm) and the assignment ctx->userspace_managed_vm = value are performed locklessly in adreno_set_param(), is it possible for a concurrent lazy VM creation to race with this? If thread A issues MSM_PARAM_EN_VM_BIND, checks if (ctx->vm) and evaluates to false, thread B could simultaneously issue an ioctl that lazily creates the VM (e.g., DRM_IOCTL_MSM_GEM_SUBMIT). Thread B would create the VM and set ctx->vm, and then thread A proceeds to set ctx->userspace_managed_vm = value. This would leave the context state desynchronized from the actual VM state. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
