Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [Critical] Calling `v3d_job_cleanup()` on an uninitialized CPU job crashes the kernel in early error paths. - [High] Nested CPU job allocations and query arrays are leaked on error paths. - [High] Missing NULL check for the `info->indirect` GEM object allows a delayed NULL pointer dereference during job execution. - [High] Memory allocated for `se.out_syncs` is leaked in multiple ioctls if `v3d_get_extensions()` fails midway. - [Medium] An uninitialized `ww_acquire_ctx` is passed to `ww_acquire_fini()`, leading to undefined behavior and lockdep crashes. -- --- Patch [1]: [PATCH] drm/v3d: release indirect CSD GEM on pre-init failures --- commit f91c6cff2fb0a592f891c8261f38ae45087334d1 Author: Guangshuo Li <[email protected]> drm/v3d: release indirect CSD GEM on pre-init failures The commit drops the indirect CSD GEM reference on pre-init error paths in the CPU job submit ioctl to avoid leaking the object when v3d_job_init() or extensions fail. > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index 1db43c6a078d5..8ed95a2038102 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c > @@ -476,6 +476,7 @@ v3d_get_cpu_indirect_csd_params(struct drm_file > *file_priv, > struct v3d_dev *v3d = v3d_priv->v3d; > struct drm_v3d_indirect_csd indirect_csd; > struct v3d_indirect_csd_info *info = &job->indirect_csd; > + int ret; > > if (!v3d_validate_cpu_job(file_priv, job)) > return -EINVAL; > @@ -496,9 +497,15 @@ v3d_get_cpu_indirect_csd_params(struct drm_file > *file_priv, > > info->indirect = drm_gem_object_lookup(file_priv, > indirect_csd.indirect); [Severity: High] This is a pre-existing issue, but what happens if the user passes an invalid GEM handle? If drm_gem_object_lookup() returns NULL in v3d_get_cpu_indirect_csd_params(), the code proceeds to submit the job. During execution, v3d_rewrite_csd_job_wg_counts_from_indirect() will attempt to use it: v3d_rewrite_csd_job_wg_counts_from_indirect() v3d_get_bo_vaddr(to_v3d_bo(indirect_csd->indirect)) Can this lead to a NULL pointer dereference later in the pipeline? > - return v3d_setup_csd_jobs_and_bos(file_priv, v3d, &indirect_csd.submit, > - &info->job, &info->clean_job, > - NULL, &info->acquire_ctx); > + ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, &indirect_csd.submit, > + &info->job, &info->clean_job, > + NULL, &info->acquire_ctx); > + if (ret && info->indirect) { > + drm_gem_object_put(info->indirect); > + info->indirect = NULL; > + } > + > + return ret; > } [ ... ] > @@ -1311,6 +1318,10 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void > *data, > struct v3d_submit_ext *out_se = NULL; > struct v3d_cpu_job *cpu_job = NULL; > struct v3d_csd_job *csd_job = NULL; > struct v3d_job *clean_job = NULL; > struct ww_acquire_ctx acquire_ctx; [Severity: Medium] This isn't a bug introduced by this patch, but is acquire_ctx guaranteed to be initialized when jobs don't require BOs? If the ioctl args have bo_handle_count == 0, v3d_lock_bo_reservations() is skipped, leaving acquire_ctx uninitialized on the stack. The function later calls v3d_attach_fences_and_unlock_reservation(), which unconditionally passes it to drm_gem_unlock_reservations() and ww_acquire_fini(). Could this pass an uninitialized lock context to the ww_mutex subsystem? > int ret; [ ... ] > if (args->flags & DRM_V3D_SUBMIT_EXTENSION) { > ret = v3d_get_extensions(file_priv, args->extensions, &se, > cpu_job); > if (ret) { > drm_dbg(dev, "Failed to get extensions.\n"); > + if (cpu_job->indirect_csd.indirect) { > + > drm_gem_object_put(cpu_job->indirect_csd.indirect); > + cpu_job->indirect_csd.indirect = NULL; > + } > goto fail; [Severity: High] This is a pre-existing issue, but does v3d_get_extensions() handle cleanup consistently across ioctls? When v3d_get_extensions() parses a DRM_V3D_EXT_ID_MULTI_SYNC extension, it allocates se.out_syncs. If a subsequent extension fails, the function returns an error. While this function correctly jumps to the fail block to clean up, other callers like v3d_submit_cl_ioctl() and v3d_submit_csd_ioctl() appear to return directly: v3d_submit_cl_ioctl() ret = v3d_get_extensions(...) if (ret) return ret; Does this leave the out_syncs memory leaked by bypassing the cleanup at v3d_put_multisync_post_deps(&se)? > } > } [ ... ] > ret = v3d_job_init(v3d, file_priv, &cpu_job->base, > v3d_cpu_job_free, 0, &se, V3D_CPU); > if (ret) { > + if (cpu_job->indirect_csd.indirect) { > + drm_gem_object_put(cpu_job->indirect_csd.indirect); > + cpu_job->indirect_csd.indirect = NULL; > + } > v3d_job_deallocate((void *)&cpu_job); [Severity: High] This isn't a bug introduced by this patch, but does shallowly deallocating the job here leak the nested structures? Earlier, v3d_setup_csd_jobs_and_bos() may allocate inner jobs: v3d_setup_csd_jobs_and_bos() v3d_job_allocate(v3d, (void *)job, ...) v3d_job_allocate(v3d, (void *)clean_job, ...) When v3d_job_deallocate() frees cpu_job, it zeroes the pointer. Since the pointer is now NULL, the cleanup at the fail label will skip it, leaving those nested job structs without a way to be freed. > goto fail; [Severity: Critical] This is a pre-existing issue, but is it safe to pass the uninitialized job to the cleanup path? If execution jumps to the fail label before v3d_job_init() succeeds, cpu_job will still be zero-initialized. The cleanup path does: fail: v3d_job_cleanup((void *)cpu_job); drm_sched_job_cleanup(&job->base); Since base.s_fence is NULL, would drm_sched_job_cleanup() trigger a NULL pointer dereference when it attempts to read the refcount: if (kref_read(&job->s_fence->finished.refcount)) > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
