Re: [PATCH AUTOSEL 6.18] drm/msm: Fix barriers accessing ctx vm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: missing smp_load_acquire() when reading ctx->vm in msm_gem_close()
--

commit 8eb807a6c2ff148154f2d8bde5de68d598d4c695
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Fix barriers accessing ctx vm

This commit adds smp_load_acquire() and smp_store_release() barriers to
msm_context_vm() to prevent concurrent callers from observing a partially
initialized struct drm_gpuvm when ctx->vm is lazily created.

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c2a8aaa33f84a..8094e5e7e9a54 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -226,18 +226,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);
>  	}
> -	mutex_unlock(&init_lock);
>  
>  	return ctx->vm;
>  }

[Severity: High]
Does msm_gem_close() also need smp_load_acquire() to pair with this
smp_store_release(&ctx->vm, vm)?

When a process creates a GEM handle via msm_ioctl_gem_new() ->
msm_gem_new_handle() without MSM_BO_NO_SHARE, ctx->vm can still be NULL. If
one thread then triggers lazy VM creation via msm_context_vm() (for example
through msm_ioctl_gem_info_iova()) while another thread closes the GEM handle
via drm_gem_handle_delete() -> drm_gem_object_release_handle() ->
msm_gem_close(), msm_gem_close() intentionally avoids calling msm_context_vm()
and reads ctx->vm directly without holding init_lock:

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

	if (msm_context_is_vmbind(ctx))
		return;

	dma_resv_wait_timeout(obj->resv, DMA_RESV_USAGE_BOOKKEEP, false,
			      MAX_SCHEDULE_TIMEOUT);

	msm_gem_lock_vm_and_obj(&exec, obj, ctx->vm);
	put_iova_spaces(obj, ctx->vm, true, "close");
	drm_exec_fini(&exec);     /* drop locks */
}

Without smp_load_acquire(&ctx->vm) in msm_gem_close(), could a weakly-ordered
architecture observe a non-NULL ctx->vm pointer before the initialization
stores to the VM (such as vm->r_obj and its dma_resv lock initialized in
msm_gem_vm_create()) become visible when msm_gem_lock_vm_and_obj() and
put_iova_spaces() dereference it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-460-sashal@kernel.org?part=1