Re: [PATCH v5 01/10] drm/msm: Fix barriers accessing ctx vm

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] MSM VM Lazy Initialization Violation / NULL Pointer Dereference in `msm_ioctl_gem_submit` and `msm_gem_new_handle`
- [High] Incomplete barrier fix: `msm_gem_close()` performs a lockless plain read of `ctx->vm` leading to a data race
--

--- Patch [1]: [PATCH v5 01/10] drm/msm: Fix barriers accessing ctx vm ---
commit fdc4f5db0f02ec8c94395cb949b516d777a3923b
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Fix barriers accessing ctx vm

This patch fixes a race condition by using smp_load_acquire and
smp_store_release when accessing ctx->vm. This prevents callers from
seeing a partially initialized vm.

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 79f78e7e16e9d..3c8b5083a1fbe 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -225,18 +225,19 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context *ctx
>  {
>  	static DEFINE_MUTEX(init_lock);
>  	struct msm_drm_private *priv = dev->dev_private;
> +	struct drm_gpuvm *vm = smp_load_acquire(&ctx->vm);

[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing the
barriers for ctx->vm here, do we also need to update msm_gem_close()?

It appears to perform a lockless plain read of ctx->vm:

drivers/gpu/drm/msm/msm_gem.c:msm_gem_close() {
	...
	if (!ctx->vm)
		return;

	/* ... */
	msm_gem_lock_vm_and_obj(&exec, obj, ctx->vm);
	...
}

Could this race with the smp_store_release() during VM creation and
observe a partially initialized VM object?

[Severity: High]
This is a pre-existing issue, but I noticed there are still direct accesses
to ctx->vm instead of using msm_context_vm() for lazy initialization.

For example, in msm_ioctl_gem_submit():

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
	...
	if (to_msm_vm(ctx->vm)->unusable)
	...
}

And also in msm_gem_new_handle():

drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle() {
	...
	struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);
	...
}

If ctx->vm is NULL before initialization, could these direct accesses
result in a NULL pointer dereference?

>  
>  	/* Once ctx->vm is created it is valid for the lifetime of the context: */
> -	if (ctx->vm)
> -		return ctx->vm;
> +	if (vm)
> +		return vm;
> +
> +	guard(mutex)(&init_lock);
>  
> -	mutex_lock(&init_lock);
>  	if (!ctx->vm) {
> -		ctx->vm = msm_gpu_create_private_vm(
> +		vm = msm_gpu_create_private_vm(
>  			priv->gpu, current, !ctx->userspace_managed_vm);
> -
> +		smp_store_release(&ctx->vm, vm);
>  	}
> -	mutex_unlock(&init_lock);
>  
>  	return ctx->vm;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716201538.365824-1-robin.clark@oss.qualcomm.com?part=1