Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm: double msm_gem_kernel_put() on error path in msm_gpu_init() --
commit 2d367cff8e8b826f570b2fdbbb2520e0cf351371 Author: Dmitry Baryshkov <[email protected]> drm/msm: share a page of kernel state with the GPU This patch allocates a global read-only GPU page (vdso) to share kernel state with GPU command streams and exposes its IOVA via MSM_PARAM_VDSO_IOVA. > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > index 0c2c356362518..747cd90e329a2 100644 > --- a/drivers/gpu/drm/msm/msm_gpu.c > +++ b/drivers/gpu/drm/msm/msm_gpu.c > @@ -989,6 +989,19 @@ int msm_gpu_init(struct drm_device *drm, struct > platform_device *pdev, > > msm_gem_object_set_name(gpu->memptrs_bo, "memptrs"); > > + gpu->vdso = msm_gem_kernel_new(drm, PAGE_SIZE, > + MSM_BO_WC | MSM_BO_GPU_READONLY, gpu->vm, &gpu->vdso_bo, > + &gpu->vdso_iova); > + > + if (IS_ERR(gpu->vdso)) { > + ret = PTR_ERR(gpu->vdso); > + gpu->vdso = NULL; > + DRM_DEV_ERROR(drm->dev, "could not allocate vdso: %d\n", ret); > + goto fail; > + } > + > + msm_gem_object_set_name(gpu->vdso_bo, "vdso"); > + > if (nr_rings > ARRAY_SIZE(gpu->rb)) { > DRM_DEV_INFO_ONCE(drm->dev, "Only creating %zu ringbuffers\n", > ARRAY_SIZE(gpu->rb)); > @@ -1033,6 +1046,7 @@ int msm_gpu_init(struct drm_device *drm, struct > platform_device *pdev, > gpu->rb[i] = NULL; > } > > + msm_gem_kernel_put(gpu->vdso_bo, gpu->vm); > msm_gem_kernel_put(gpu->memptrs_bo, gpu->vm); > > platform_set_drvdata(pdev, NULL); > @@ -1050,6 +1064,7 @@ void msm_gpu_cleanup(struct msm_gpu *gpu) > gpu->rb[i] = NULL; > } > > + msm_gem_kernel_put(gpu->vdso_bo, gpu->vm); > msm_gem_kernel_put(gpu->memptrs_bo, gpu->vm); [Severity: High] Can this cause a double put and use-after-free on gpu->vdso_bo and gpu->memptrs_bo if msm_gpu_init() fails after allocating them? In msm_gpu_init(), if allocating gpu->vdso fails (after gpu->memptrs_bo was allocated) or if a later step such as msm_ringbuffer_new() or msm_perfcntr_init() fails, the fail path calls msm_gem_kernel_put() on gpu->vdso_bo and gpu->memptrs_bo without clearing those pointers to NULL (unlike gpu->rb[i], which is cleared to NULL in the loop right above). When msm_gpu_init() returns an error to adreno_gpu_init(), the callers (a2xx_gpu_init() through a6xx_gpu_init()) clean up by invoking their destroy callback, for example: a6xx_gpu_init() adreno_gpu_init() -> msm_gpu_init() (fails, puts vdso_bo and memptrs_bo) a6xx_destroy() adreno_gpu_cleanup() msm_gpu_cleanup() msm_gem_kernel_put(gpu->vdso_bo, gpu->vm); msm_gem_kernel_put(gpu->memptrs_bo, gpu->vm); Because gpu->vdso_bo and gpu->memptrs_bo are still non-NULL, msm_gpu_cleanup() passes the already-freed GEM objects to msm_gem_kernel_put() a second time. Should the fail path in msm_gpu_init() clear gpu->vdso_bo and gpu->memptrs_bo to NULL after calling msm_gem_kernel_put(), or leave their release to msm_gpu_cleanup()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
