Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Missing `v3d_job_deallocate()` on pre-init validation error paths causes a kernel panic in `drm_sched_job_cleanup()`. - [High] Incomplete cleanup on `v3d_get_extensions()` failure causes resource leaks and deadlocks for chained extensions. - [High] Early validation failures for INDIRECT_CSD CPU jobs bypass WW mutex unreservation and leak nested jobs. -- --- Patch [1]: [PATCH v3] drm/v3d: release CPU job resources on pre-init failures --- commit 0d9b615d1fa0aedf4a08be6bbd88de15b262217c Author: Guangshuo Li <[email protected]> drm/v3d: release CPU job resources on pre-init failures This commit extracts CPU job resource cleanup into a new helper function and calls it on pre-init failure paths in the drm/v3d driver. This helps prevent leaking extension resources if validation fails before the job destructor is installed. > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index 7682b24f13ec5..77e94c25b2afd 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c [ ... ] > @@ -1320,12 +1326,14 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void > *data, [Severity: High] This is a pre-existing issue, but since the v3 patch notes mention removing v3d_cpu_job_free_resources() from the v3d_get_extensions() error path, can an error during extension parsing leak resources? If v3d_get_extensions() loops through a chain of user extensions and a later extension fails to parse, it returns an error immediately. Are the resources and reservations acquired by previously parsed extensions in the chain unrolled and freed? For example, if an INDIRECT_CSD extension parses successfully but a subsequent extension fails, it appears the BO reservations locked in v3d_setup_csd_jobs_and_bos() are never unlocked, which could lead to a WW mutex deadlock. Does v3d_get_extensions() handle chained extension failures fully? > /* Every CPU job must have a CPU job user extension */ > if (!cpu_job->job_type) { > drm_dbg(dev, "CPU job must have a CPU job user extension.\n"); > + v3d_cpu_job_free_resources(cpu_job); > ret = -EINVAL; > goto fail; > } > > if (args->bo_handle_count != > cpu_job_bo_handle_count[cpu_job->job_type]) { > drm_dbg(dev, "This CPU job was not submitted with the proper > number of BOs.\n"); > + v3d_cpu_job_free_resources(cpu_job); > ret = -EINVAL; > goto fail; > } [Severity: High] This is a pre-existing issue, but does jumping to the fail label here leave cpu_job allocated and crash during cleanup? When jumping to the fail block, cpu_job is not set to NULL and v3d_job_deallocate() is not called. The fail block executes: fail: v3d_job_cleanup((void *)cpu_job); This eventually calls drm_sched_job_cleanup(&job->base). Because v3d_job_init() has not been called yet, job->s_fence is NULL. Can this result in a NULL pointer dereference and kernel panic in drm_sched_job_cleanup()? [Severity: High] This is also a pre-existing issue, but can these early validation paths leak nested jobs and cause deadlocks for INDIRECT_CSD jobs? When an INDIRECT_CSD extension is parsed earlier, v3d_setup_csd_jobs_and_bos() allocates nested csd_job and clean_job structures and locks their GEM BO reservations. If validation fails here, the code jumps to fail, bypassing the fail_unreserve label. While v3d_cpu_job_free_resources() drops the indirect GEM reference, it does not free the nested jobs or unlock their context. At the fail block, the local csd_job and clean_job variables are still NULL (they are assigned later in the function), so v3d_job_cleanup() does nothing for them. Can this permanently leak the jobs and leave the BOs locked? > @@ -1335,6 +1343,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data, > ret = v3d_job_init(v3d, file_priv, &cpu_job->base, > v3d_cpu_job_free, 0, &se, V3D_CPU); > if (ret) { > + v3d_cpu_job_free_resources(cpu_job); > v3d_job_deallocate((void *)&cpu_job); > goto fail; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
