| Message ID | 20240711091542.82083-4-tursulin@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v3d: Perfmon cleanup | expand |
On 7/11/24 06:15, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > If fetching of userspace memory fails during the main loop, all drm sync > objs looked up until that point will be leaked because of the missing > drm_syncobj_put. > > Fix it by exporting and using a common cleanup helper. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > Fixes: bae7cb5d6800 ("drm/v3d: Create a CPU job extension for the reset performance query job") > Cc: Maíra Canal <mcanal@igalia.com> > Cc: Iago Toral Quiroga <itoral@igalia.com> > Cc: <stable@vger.kernel.org> # v6.8+ > --- > drivers/gpu/drm/v3d/v3d_drv.h | 2 ++ > drivers/gpu/drm/v3d/v3d_sched.c | 22 ++++++++++---- > drivers/gpu/drm/v3d/v3d_submit.c | 50 ++++++++++++++++++++------------ > 3 files changed, 49 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h > index e208ffdfba32..dd3ead4cb8bd 100644 > --- a/drivers/gpu/drm/v3d/v3d_drv.h > +++ b/drivers/gpu/drm/v3d/v3d_drv.h > @@ -565,6 +565,8 @@ void v3d_mmu_remove_ptes(struct v3d_bo *bo); > /* v3d_sched.c */ > void v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *query_info, > unsigned int count); > +void v3d_performance_query_info_free(struct v3d_performance_query_info *query_info, > + unsigned int count); > void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue); > int v3d_sched_init(struct v3d_dev *v3d); > void v3d_sched_fini(struct v3d_dev *v3d); > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c > index 59dc0287dab9..5fbbee47c6b7 100644 > --- a/drivers/gpu/drm/v3d/v3d_sched.c > +++ b/drivers/gpu/drm/v3d/v3d_sched.c > @@ -87,20 +87,30 @@ v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *query_info, > } > } > > +void > +v3d_performance_query_info_free(struct v3d_performance_query_info *query_info, > + unsigned int count) > +{ > + if (query_info->queries) { > + unsigned int i; > + > + for (i = 0; i < count; i++) > + drm_syncobj_put(query_info->queries[i].syncobj); > + > + kvfree(query_info->queries); > + } > +} > + > static void > v3d_cpu_job_free(struct drm_sched_job *sched_job) > { > struct v3d_cpu_job *job = to_cpu_job(sched_job); > - struct v3d_performance_query_info *performance_query = &job->performance_query; > > v3d_timestamp_query_info_free(&job->timestamp_query, > job->timestamp_query.count); > > - if (performance_query->queries) { > - for (int i = 0; i < performance_query->count; i++) > - drm_syncobj_put(performance_query->queries[i].syncobj); > - kvfree(performance_query->queries); > - } > + v3d_performance_query_info_free(&job->performance_query, > + job->performance_query.count); > > v3d_job_cleanup(&job->base); > } > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c > index 121bf1314b80..d626c8539b04 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c > @@ -640,6 +640,8 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, > u32 __user *syncs; > u64 __user *kperfmon_ids; > struct drm_v3d_reset_performance_query reset; > + unsigned int i, j; > + int err; > > if (!job) { > DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); > @@ -668,39 +670,43 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, > syncs = u64_to_user_ptr(reset.syncs); > kperfmon_ids = u64_to_user_ptr(reset.kperfmon_ids); > > - for (int i = 0; i < reset.count; i++) { > + for (i = 0; i < reset.count; i++) { > u32 sync; > u64 ids; > u32 __user *ids_pointer; > u32 id; > > if (copy_from_user(&sync, syncs++, sizeof(sync))) { > - kvfree(job->performance_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > - job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); > - > if (copy_from_user(&ids, kperfmon_ids++, sizeof(ids))) { > - kvfree(job->performance_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > ids_pointer = u64_to_user_ptr(ids); > > - for (int j = 0; j < reset.nperfmons; j++) { > + for (j = 0; j < reset.nperfmons; j++) { > if (copy_from_user(&id, ids_pointer++, sizeof(id))) { > - kvfree(job->performance_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > job->performance_query.queries[i].kperfmon_ids[j] = id; > } > + > + job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); > } > job->performance_query.count = reset.count; > job->performance_query.nperfmons = reset.nperfmons; > > return 0; > + > +error: > + v3d_performance_query_info_free(&job->performance_query, i); > + return err; > } > > static int > @@ -711,6 +717,8 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, > u32 __user *syncs; > u64 __user *kperfmon_ids; > struct drm_v3d_copy_performance_query copy; > + unsigned int i, j; > + int err; > > if (!job) { > DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); > @@ -749,27 +757,27 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, > u32 id; > > if (copy_from_user(&sync, syncs++, sizeof(sync))) { > - kvfree(job->performance_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > - job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); > - > if (copy_from_user(&ids, kperfmon_ids++, sizeof(ids))) { > - kvfree(job->performance_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > ids_pointer = u64_to_user_ptr(ids); > > - for (int j = 0; j < copy.nperfmons; j++) { > + for (j = 0; j < copy.nperfmons; j++) { > if (copy_from_user(&id, ids_pointer++, sizeof(id))) { > - kvfree(job->performance_query.queries); > - return -EFAULT; > + err = -EFAULT; > + goto error; > } > > job->performance_query.queries[i].kperfmon_ids[j] = id; > } > + > + job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); > } > job->performance_query.count = copy.count; > job->performance_query.nperfmons = copy.nperfmons; > @@ -782,6 +790,10 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, > job->copy.stride = copy.stride; > > return 0; > + > +error: > + v3d_performance_query_info_free(&job->performance_query, i); I'm seeing this compiling warning from this patch: drivers/gpu/drm/v3d/v3d_submit.c:860:59: warning: variable 'i' is uninitialized when used here [-Wuninitialized] 860 | v3d_performance_query_info_free(&job->performance_query, i); | ^ drivers/gpu/drm/v3d/v3d_submit.c:781:16: note: initialize the variable 'i' to silence this warning 781 | unsigned int i, j; | ^ | = 0 1 warning generated. I believe that we are currently defining int i twice. Best Regards, - Maíra > + return err; > } > > /* Whenever userspace sets ioctl extensions, v3d_get_extensions parses data
On 11/07/2024 14:00, Maíra Canal wrote: > On 7/11/24 06:15, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> >> If fetching of userspace memory fails during the main loop, all drm sync >> objs looked up until that point will be leaked because of the missing >> drm_syncobj_put. >> >> Fix it by exporting and using a common cleanup helper. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> Fixes: bae7cb5d6800 ("drm/v3d: Create a CPU job extension for the >> reset performance query job") >> Cc: Maíra Canal <mcanal@igalia.com> >> Cc: Iago Toral Quiroga <itoral@igalia.com> >> Cc: <stable@vger.kernel.org> # v6.8+ >> --- >> drivers/gpu/drm/v3d/v3d_drv.h | 2 ++ >> drivers/gpu/drm/v3d/v3d_sched.c | 22 ++++++++++---- >> drivers/gpu/drm/v3d/v3d_submit.c | 50 ++++++++++++++++++++------------ >> 3 files changed, 49 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h >> b/drivers/gpu/drm/v3d/v3d_drv.h >> index e208ffdfba32..dd3ead4cb8bd 100644 >> --- a/drivers/gpu/drm/v3d/v3d_drv.h >> +++ b/drivers/gpu/drm/v3d/v3d_drv.h >> @@ -565,6 +565,8 @@ void v3d_mmu_remove_ptes(struct v3d_bo *bo); >> /* v3d_sched.c */ >> void v3d_timestamp_query_info_free(struct v3d_timestamp_query_info >> *query_info, >> unsigned int count); >> +void v3d_performance_query_info_free(struct >> v3d_performance_query_info *query_info, >> + unsigned int count); >> void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue); >> int v3d_sched_init(struct v3d_dev *v3d); >> void v3d_sched_fini(struct v3d_dev *v3d); >> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c >> b/drivers/gpu/drm/v3d/v3d_sched.c >> index 59dc0287dab9..5fbbee47c6b7 100644 >> --- a/drivers/gpu/drm/v3d/v3d_sched.c >> +++ b/drivers/gpu/drm/v3d/v3d_sched.c >> @@ -87,20 +87,30 @@ v3d_timestamp_query_info_free(struct >> v3d_timestamp_query_info *query_info, >> } >> } >> +void >> +v3d_performance_query_info_free(struct v3d_performance_query_info >> *query_info, >> + unsigned int count) >> +{ >> + if (query_info->queries) { >> + unsigned int i; >> + >> + for (i = 0; i < count; i++) >> + drm_syncobj_put(query_info->queries[i].syncobj); >> + >> + kvfree(query_info->queries); >> + } >> +} >> + >> static void >> v3d_cpu_job_free(struct drm_sched_job *sched_job) >> { >> struct v3d_cpu_job *job = to_cpu_job(sched_job); >> - struct v3d_performance_query_info *performance_query = >> &job->performance_query; >> v3d_timestamp_query_info_free(&job->timestamp_query, >> job->timestamp_query.count); >> - if (performance_query->queries) { >> - for (int i = 0; i < performance_query->count; i++) >> - drm_syncobj_put(performance_query->queries[i].syncobj); >> - kvfree(performance_query->queries); >> - } >> + v3d_performance_query_info_free(&job->performance_query, >> + job->performance_query.count); >> v3d_job_cleanup(&job->base); >> } >> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c >> b/drivers/gpu/drm/v3d/v3d_submit.c >> index 121bf1314b80..d626c8539b04 100644 >> --- a/drivers/gpu/drm/v3d/v3d_submit.c >> +++ b/drivers/gpu/drm/v3d/v3d_submit.c >> @@ -640,6 +640,8 @@ v3d_get_cpu_reset_performance_params(struct >> drm_file *file_priv, >> u32 __user *syncs; >> u64 __user *kperfmon_ids; >> struct drm_v3d_reset_performance_query reset; >> + unsigned int i, j; >> + int err; >> if (!job) { >> DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); >> @@ -668,39 +670,43 @@ v3d_get_cpu_reset_performance_params(struct >> drm_file *file_priv, >> syncs = u64_to_user_ptr(reset.syncs); >> kperfmon_ids = u64_to_user_ptr(reset.kperfmon_ids); >> - for (int i = 0; i < reset.count; i++) { >> + for (i = 0; i < reset.count; i++) { >> u32 sync; >> u64 ids; >> u32 __user *ids_pointer; >> u32 id; >> if (copy_from_user(&sync, syncs++, sizeof(sync))) { >> - kvfree(job->performance_query.queries); >> - return -EFAULT; >> + err = -EFAULT; >> + goto error; >> } >> - job->performance_query.queries[i].syncobj = >> drm_syncobj_find(file_priv, sync); >> - >> if (copy_from_user(&ids, kperfmon_ids++, sizeof(ids))) { >> - kvfree(job->performance_query.queries); >> - return -EFAULT; >> + err = -EFAULT; >> + goto error; >> } >> ids_pointer = u64_to_user_ptr(ids); >> - for (int j = 0; j < reset.nperfmons; j++) { >> + for (j = 0; j < reset.nperfmons; j++) { >> if (copy_from_user(&id, ids_pointer++, sizeof(id))) { >> - kvfree(job->performance_query.queries); >> - return -EFAULT; >> + err = -EFAULT; >> + goto error; >> } >> job->performance_query.queries[i].kperfmon_ids[j] = id; >> } >> + >> + job->performance_query.queries[i].syncobj = >> drm_syncobj_find(file_priv, sync); >> } >> job->performance_query.count = reset.count; >> job->performance_query.nperfmons = reset.nperfmons; >> return 0; >> + >> +error: >> + v3d_performance_query_info_free(&job->performance_query, i); >> + return err; >> } >> static int >> @@ -711,6 +717,8 @@ v3d_get_cpu_copy_performance_query_params(struct >> drm_file *file_priv, >> u32 __user *syncs; >> u64 __user *kperfmon_ids; >> struct drm_v3d_copy_performance_query copy; >> + unsigned int i, j; >> + int err; >> if (!job) { >> DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); >> @@ -749,27 +757,27 @@ v3d_get_cpu_copy_performance_query_params(struct >> drm_file *file_priv, >> u32 id; >> if (copy_from_user(&sync, syncs++, sizeof(sync))) { >> - kvfree(job->performance_query.queries); >> - return -EFAULT; >> + err = -EFAULT; >> + goto error; >> } >> - job->performance_query.queries[i].syncobj = >> drm_syncobj_find(file_priv, sync); >> - >> if (copy_from_user(&ids, kperfmon_ids++, sizeof(ids))) { >> - kvfree(job->performance_query.queries); >> - return -EFAULT; >> + err = -EFAULT; >> + goto error; >> } >> ids_pointer = u64_to_user_ptr(ids); >> - for (int j = 0; j < copy.nperfmons; j++) { >> + for (j = 0; j < copy.nperfmons; j++) { >> if (copy_from_user(&id, ids_pointer++, sizeof(id))) { >> - kvfree(job->performance_query.queries); >> - return -EFAULT; >> + err = -EFAULT; >> + goto error; >> } >> job->performance_query.queries[i].kperfmon_ids[j] = id; >> } >> + >> + job->performance_query.queries[i].syncobj = >> drm_syncobj_find(file_priv, sync); >> } >> job->performance_query.count = copy.count; >> job->performance_query.nperfmons = copy.nperfmons; >> @@ -782,6 +790,10 @@ v3d_get_cpu_copy_performance_query_params(struct >> drm_file *file_priv, >> job->copy.stride = copy.stride; >> return 0; >> + >> +error: >> + v3d_performance_query_info_free(&job->performance_query, i); > > I'm seeing this compiling warning from this patch: > > drivers/gpu/drm/v3d/v3d_submit.c:860:59: warning: variable 'i' is > uninitialized when used here [-Wuninitialized] > 860 | v3d_performance_query_info_free(&job->performance_query, i); > | ^ > drivers/gpu/drm/v3d/v3d_submit.c:781:16: note: initialize the variable > 'i' to silence this warning > 781 | unsigned int i, j; > | ^ > | = 0 > 1 warning generated. > > I believe that we are currently defining int i twice. Yep.. more re-ordering/rebasing fails on my end. Fixed locally. Regards, Tvrtko
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index e208ffdfba32..dd3ead4cb8bd 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -565,6 +565,8 @@ void v3d_mmu_remove_ptes(struct v3d_bo *bo); /* v3d_sched.c */ void v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *query_info, unsigned int count); +void v3d_performance_query_info_free(struct v3d_performance_query_info *query_info, + unsigned int count); void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue); int v3d_sched_init(struct v3d_dev *v3d); void v3d_sched_fini(struct v3d_dev *v3d); diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c index 59dc0287dab9..5fbbee47c6b7 100644 --- a/drivers/gpu/drm/v3d/v3d_sched.c +++ b/drivers/gpu/drm/v3d/v3d_sched.c @@ -87,20 +87,30 @@ v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *query_info, } } +void +v3d_performance_query_info_free(struct v3d_performance_query_info *query_info, + unsigned int count) +{ + if (query_info->queries) { + unsigned int i; + + for (i = 0; i < count; i++) + drm_syncobj_put(query_info->queries[i].syncobj); + + kvfree(query_info->queries); + } +} + static void v3d_cpu_job_free(struct drm_sched_job *sched_job) { struct v3d_cpu_job *job = to_cpu_job(sched_job); - struct v3d_performance_query_info *performance_query = &job->performance_query; v3d_timestamp_query_info_free(&job->timestamp_query, job->timestamp_query.count); - if (performance_query->queries) { - for (int i = 0; i < performance_query->count; i++) - drm_syncobj_put(performance_query->queries[i].syncobj); - kvfree(performance_query->queries); - } + v3d_performance_query_info_free(&job->performance_query, + job->performance_query.count); v3d_job_cleanup(&job->base); } diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c index 121bf1314b80..d626c8539b04 100644 --- a/drivers/gpu/drm/v3d/v3d_submit.c +++ b/drivers/gpu/drm/v3d/v3d_submit.c @@ -640,6 +640,8 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, u32 __user *syncs; u64 __user *kperfmon_ids; struct drm_v3d_reset_performance_query reset; + unsigned int i, j; + int err; if (!job) { DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); @@ -668,39 +670,43 @@ v3d_get_cpu_reset_performance_params(struct drm_file *file_priv, syncs = u64_to_user_ptr(reset.syncs); kperfmon_ids = u64_to_user_ptr(reset.kperfmon_ids); - for (int i = 0; i < reset.count; i++) { + for (i = 0; i < reset.count; i++) { u32 sync; u64 ids; u32 __user *ids_pointer; u32 id; if (copy_from_user(&sync, syncs++, sizeof(sync))) { - kvfree(job->performance_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } - job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); - if (copy_from_user(&ids, kperfmon_ids++, sizeof(ids))) { - kvfree(job->performance_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } ids_pointer = u64_to_user_ptr(ids); - for (int j = 0; j < reset.nperfmons; j++) { + for (j = 0; j < reset.nperfmons; j++) { if (copy_from_user(&id, ids_pointer++, sizeof(id))) { - kvfree(job->performance_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->performance_query.queries[i].kperfmon_ids[j] = id; } + + job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); } job->performance_query.count = reset.count; job->performance_query.nperfmons = reset.nperfmons; return 0; + +error: + v3d_performance_query_info_free(&job->performance_query, i); + return err; } static int @@ -711,6 +717,8 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, u32 __user *syncs; u64 __user *kperfmon_ids; struct drm_v3d_copy_performance_query copy; + unsigned int i, j; + int err; if (!job) { DRM_DEBUG("CPU job extension was attached to a GPU job.\n"); @@ -749,27 +757,27 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, u32 id; if (copy_from_user(&sync, syncs++, sizeof(sync))) { - kvfree(job->performance_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } - job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); - if (copy_from_user(&ids, kperfmon_ids++, sizeof(ids))) { - kvfree(job->performance_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } ids_pointer = u64_to_user_ptr(ids); - for (int j = 0; j < copy.nperfmons; j++) { + for (j = 0; j < copy.nperfmons; j++) { if (copy_from_user(&id, ids_pointer++, sizeof(id))) { - kvfree(job->performance_query.queries); - return -EFAULT; + err = -EFAULT; + goto error; } job->performance_query.queries[i].kperfmon_ids[j] = id; } + + job->performance_query.queries[i].syncobj = drm_syncobj_find(file_priv, sync); } job->performance_query.count = copy.count; job->performance_query.nperfmons = copy.nperfmons; @@ -782,6 +790,10 @@ v3d_get_cpu_copy_performance_query_params(struct drm_file *file_priv, job->copy.stride = copy.stride; return 0; + +error: + v3d_performance_query_info_free(&job->performance_query, i); + return err; } /* Whenever userspace sets ioctl extensions, v3d_get_extensions parses data