Re: [PATCH v4 5/6] drm/msm: Fix per-process-pgtables check

[sashiko-bot@kernel.org]

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 <robin.clark@oss.qualcomm.com>
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/20260708173356.578713-1-robin.clark@oss.qualcomm.com?part=5