Hi Guangshuo, You could incorporate your patch "[PATCH] drm/v3d: release indirect CSD GEM on pre-init failures" into this one, which would be a much cleaner solution.
The patch looks okay to me, just a few things. First, please rebase your changes into drm-misc-fixes. On 08/07/26 06:01, Guangshuo Li wrote:
The changes referenced by the Fixes tags moved CPU job resource cleanup to v3d_cpu_job_free(), the CPU job kref destructor. However, CPU job extensions are parsed before v3d_job_init() has successfully installed that destructor. Timestamp and performance query arrays, their syncobj references, and the indirect CSD GEM reference can therefore be acquired before the CPU job is managed by the kref cleanup path. If a later pre-init validation fails, or if v3d_job_init() itself fails, the CPU job can be cleaned up or deallocated without running v3d_cpu_job_free(). Those extension resources are then leaked. Move the CPU-job owned resource release into a helper and call it both from the normal CPU job destructor and from the pre-init error paths. Fixes: b0fe80c0b925 ("drm/v3d: Fix use-after-free of CPU job query arrays on error path") Fixes: 6eb6e5acafa4 ("drm/v3d: Release indirect CSD GEM reference on CPU job free") Signed-off-by: Guangshuo Li <[email protected]> --- drivers/gpu/drm/v3d/v3d_submit.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 8ed95a203810..2cca9c49f30c 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -127,20 +127,28 @@ v3d_render_job_free(struct kref *ref) }static void-v3d_cpu_job_free(struct kref *ref) +v3d_cpu_job_free_resources(struct v3d_cpu_job *job) { - struct v3d_cpu_job *job = container_of(ref, struct v3d_cpu_job, - base.refcount); - v3d_timestamp_query_info_free(&job->timestamp_query, job->timestamp_query.count); + job->timestamp_query.queries = NULL; + job->timestamp_query.count = 0;
Why do you need to do that? It looks unnecessary to me. Best regards, - Maíra
v3d_performance_query_info_free(&job->performance_query,job->performance_query.count); + job->performance_query.queries = NULL; + job->performance_query.count = 0;- if (job->indirect_csd.indirect)+ if (job->indirect_csd.indirect) { drm_gem_object_put(job->indirect_csd.indirect); + job->indirect_csd.indirect = NULL; + } +}+static void+v3d_cpu_job_free(struct kref *ref) +{ + v3d_cpu_job_free_resources(job); v3d_job_free(ref); }@@ -1318,10 +1326,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,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; - } + v3d_cpu_job_free_resources(cpu_job); goto fail; } } @@ -1329,12 +1334,14 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data, /* 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; } @@ -1344,10 +1351,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) { - if (cpu_job->indirect_csd.indirect) { - drm_gem_object_put(cpu_job->indirect_csd.indirect); - cpu_job->indirect_csd.indirect = NULL; - } + v3d_cpu_job_free_resources(cpu_job); v3d_job_deallocate((void *)&cpu_job); goto fail; }
