Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Calling DRM_IOCTL_MSM_GEM_NEW with the MSM_BO_NO_SHARE flag on a device without a GPU causes a NULL pointer dereference. - [High] Lockless fast-path reading of `ctx->vm` lacks acquire/release memory barriers, allowing concurrent threads to observe and access a partially initialized `drm_gpuvm` structure. -- commit e440c951a727aa924c22337cdf0290e20b62ff2e Author: Rob Clark <[email protected]> drm/msm: Validate lazy VM in GEM_NEW Otherwise creating a _NO_SHARE BO before any BOs are mapped could cause a NPE. Reported-by: Sashiko <[email protected]> Fixes: b58e12a66e47 ("drm/msm: Add _NO_SHARE flag") Signed-off-by: Rob Clark <[email protected]> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > index 2cb3ab04f1250..ef14ee66c6c30 100644 > --- a/drivers/gpu/drm/msm/msm_gem.c > +++ b/drivers/gpu/drm/msm/msm_gem.c > @@ -1148,7 +1148,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct > drm_file *file, > > 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); > + struct drm_gpuvm *vm = msm_context_vm(dev, ctx); [Severity: High] This isn't a bug introduced by this patch, but does the implementation of msm_context_vm() lack necessary memory barriers? Looking at msm_context_vm() in drivers/gpu/drm/msm/msm_drv.c: if (ctx->vm) return ctx->vm; mutex_lock(&init_lock); if (!ctx->vm) { ctx->vm = msm_gpu_create_private_vm(...); } mutex_unlock(&init_lock); Since the fast-path reads ctx->vm without smp_load_acquire() and the write happens without smp_store_release(), can a concurrent ioctl thread observe a non-NULL ctx->vm pointer before the underlying drm_gpuvm structure is fully initialized (such as setting vm->r_obj) on weakly-ordered architectures? > + struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(vm); [Severity: Critical] This is a pre-existing issue, but does msm_context_vm() return NULL on display-only configurations where the GPU fails to probe (priv->gpu == NULL)? If msm_context_vm() returns NULL, wouldn't passing it to drm_gpuvm_resv_obj(vm) cause an immediate NULL pointer dereference since the macro unconditionally expands to ((vm)->r_obj) here? > > drm_gem_object_get(r_obj); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
