Re: [PATCH v7 01/17] drm/msm: Fix barriers accessing ctx vm

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Directly accessing `ctx->vm` in ioctl entry points before ensuring the VM is initialized via `msm_context_vm()` causes a NULL pointer dereference on the first operation.
- [Critical] `msm_context_vm()` unconditionally caches an `ERR_PTR` inside `ctx->vm` upon allocation failure, permanently breaking the context and causing crashes on subsequent accesses.
- [Critical] Callers of `msm_context_vm()` fail to check if the returned value is an error pointer, leading to a crash on VM creation failure.
- [High] The patch misses concurrent readers like `msm_gem_close()` that still perform plain reads of `ctx->vm`, leaving them vulnerable to data races and partially initialized VM states.
--

--- Patch [1]: [PATCH v7 01/17] drm/msm: Fix barriers accessing ctx vm ---
Commit SHA: 8c0550637c7f990d073da2dbc73f127555883df0
Author: Rob Clark <robin.clark@oss.qualcomm.com>
Subject: drm/msm: Fix barriers accessing ctx vm

This commit updates msm_context_vm() to use proper memory barriers
(smp_load_acquire and smp_store_release) to prevent callers from observing
a partially initialized VM.

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index ac86b427c0e53..29075aafc5cc5 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -224,18 +224,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);
>  
>  	/* 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);

[Severity: Critical]
This is a pre-existing issue, but I noticed that msm_gpu_create_private_vm()
can return an ERR_PTR on failure (e.g., if page table allocation fails). 

If we store that ERR_PTR into ctx->vm without an IS_ERR() check, will 
future fast-path readers see this truthy pointer and attempt to dereference 
the error value as a valid VM?

>  	}
> -	mutex_unlock(&init_lock);
>  
>  	return ctx->vm;
>  }

[Severity: Critical]
This is also a pre-existing issue, but does submit_create() need to check 
the return value of msm_context_vm() before assigning it to submit->vm?

drivers/gpu/drm/msm/msm_gem_submit.c:submit_create() {
	...
	submit->dev = dev;
	submit->vm = msm_context_vm(dev, queue->ctx);
	submit->gpu = gpu;
	...
}

If it returns an ERR_PTR, it will eventually cause a panic when submit->vm 
is dereferenced.

Similarly, in msm_submitqueue_create():

drivers/gpu/drm/msm/msm_submitqueue.c:msm_submitqueue_create() {
	...
	if (flags & MSM_SUBMITQUEUE_VM_BIND) {
		struct drm_gpu_scheduler *sched =
			&to_msm_vm(msm_context_vm(drm, ctx))->sched;

		queue->entity = &queue->_vm_bind_entity[0];
	...
}

Does this code risk a crash by passing an unchecked pointer to to_msm_vm(), 
which computes an offset from the error pointer and then accesses sched?


[Severity: Critical]
This isn't a bug introduced by this patch, but I noticed some paths directly 
access ctx->vm before it's guaranteed to be initialized. For example:

drivers/gpu/drm/msm/msm_gem_submit.c:msm_ioctl_gem_submit() {
	...
	if (args->pad)
		return -EINVAL;

	if (to_msm_vm(ctx->vm)->unusable)
		return UERR(EPIPE, dev, "context is unusable");
	...
}

If an unprivileged user calls this ioctl as the very first operation on a
new context, won't ctx->vm still be NULL here, leading to an invalid
pointer dereference via the to_msm_vm() offset macro?

I noticed a similar pattern in msm_gem_new_handle():

drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle() {
	...
	if (flags & MSM_BO_NO_SHARE) {
		struct msm_context *ctx = file->driver_priv;
		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);

		drm_gem_object_get(r_obj);
	...
}


[Severity: High]
This isn't a bug introduced by this patch, but since the patch addresses 
concurrent access to ctx->vm, does msm_gem_close() also need to use 
smp_load_acquire() instead of a plain read?

drivers/gpu/drm/msm/msm_gem.c:msm_gem_close() {
	...
	msm_gem_vma_put(obj);

	/*
	 * If VM isn't created yet, nothing to cleanup.  And in fact calling
	 * put_iova_spaces() with vm=NULL would be bad, in that it will tear-
	 * down the mappings of shared buffers in other contexts.
	 */
	if (!ctx->vm)
		return;
	...
}

Without a read barrier, could a concurrent ioctl that initializes the VM
cause this function to observe a non-NULL pointer but read uninitialized
memory from the VM structure?

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