Re: [PATCH v5 08/10] drm/msm: Validate lazy VM in GEM_NEW

[sashiko-bot@kernel.org]

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

New issues:
- [High] The fallback logic for shared VMs in `msm_gpu_create_private_vm()` is inverted, causing VM_BIND contexts to incorrectly fall back to the shared VM while legacy contexts fail to do so.

Pre-existing issues:
- [High] Concurrent modification of `obj->resv` after publishing the GEM object to `priv->objects` causes a mismatched lock/unlock sequence if accessed via debugfs.
--

--- Patch [8]: [PATCH v5 08/10] drm/msm: Validate lazy VM in GEM_NEW ---
commit b2a817e93a22417a9189cd5ad2d48b886f18f14d
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Validate lazy VM in GEM_NEW

Otherwise creating a _NO_SHARE BO before any BOs are mapped could cause
a NPE.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 2cb3ab04f1250..9bdbcf1279b00 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1147,8 +1147,16 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  		msm_gem_object_set_name(obj, "%s", name);
>  
>  	if (flags & MSM_BO_NO_SHARE) {
> +		struct msm_drm_private *priv = dev->dev_private;
>  		struct msm_context *ctx = file->driver_priv;
> -		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm);
> +		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);

[Severity: High]
Does this call to msm_context_vm() trigger inverted fallback logic for shared
VMs in drivers/gpu/drm/msm/msm_gpu.c?

When msm_context_vm() evaluates !ctx->userspace_managed_vm, it passes this as
the kernel_managed parameter to msm_gpu_create_private_vm().

In msm_gpu_create_private_vm(), the fallback condition is:

    if (IS_ERR_OR_NULL(vm) && !kernel_managed)
        vm = drm_gpuvm_get(gpu->vm);

This causes VM_BIND contexts (where kernel_managed is false) to silently fall
back to the shared VM, directly violating the intent described in the commit
'drm/msm: Don't fallback to shared VM for VM_BIND'.

Conversely, for legacy contexts (where kernel_managed is true), the fallback
is skipped. This leaves vm as NULL on legacy targets that lack private VM
support, which would break context initialization completely.

> +
> +		if (!priv->gpu || !vm) {
> +			ret = UERR(EINVAL, dev, "not supported with shared VM");
> +			goto out_put;
> +		}
> +
> +		struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(vm);
>  
>  		drm_gem_object_get(r_obj);
>  
>  		obj->resv = r_obj->resv;

[Severity: High]
This is a pre-existing issue, but can this concurrent modification of
obj->resv cause a mismatched lock/unlock sequence if accessed via debugfs?

In drivers/gpu/drm/msm/msm_gem.c:msm_gem_new(), the BO is allocated,
obj->resv is initialized to &obj->_resv, and it is added to the global
priv->objects list under priv->obj_lock. This makes the object globally
visible.

If debugfs iterates over priv->objects concurrently via msm_gem_show(), it
calls msm_gem_describe(obj) which evaluates msm_gem_trylock(obj) and locks
the original &obj->_resv.

If the ioctl thread executing msm_gem_new_handle() then overwrites obj->resv
here without holding any locks, the subsequent msm_gem_unlock(obj) in debugfs
will unlock the new r_obj->resv (which it never locked).

This mismatched sequence would cause a kernel BUG in ww_mutex_unlock or a
lockdep splat.

[ ... ]

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