| Message ID | 20250318155424.78552-4-tvrtko.ursulin@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | A few drm_syncobj optimisations | expand |
Hi Tvrtko, On 18/03/25 12:54, Tvrtko Ursulin wrote: > Drm_syncobj_array_find() helper is used from many userspace ioctl entry > points with the task of looking up userspace handles to internal objects. > > We can easily avoid one temporary allocation by making it read the handles > as it is looking them up. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > --- > drivers/gpu/drm/drm_syncobj.c | 44 +++++++++++++++++------------------ > 1 file changed, 21 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index fd5ba6c89666..cdda2df06bec 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -1213,48 +1213,46 @@ signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec) > EXPORT_SYMBOL(drm_timeout_abs_to_jiffies); > > static int drm_syncobj_array_find(struct drm_file *file_private, > - void __user *user_handles, > - uint32_t count_handles, > + u32 __user *handles, > + uint32_t count, > struct drm_syncobj ***syncobjs_out) > { > - uint32_t i, *handles; > struct drm_syncobj **syncobjs; > + uint32_t i; > int ret; > > - handles = kmalloc_array(count_handles, sizeof(*handles), GFP_KERNEL); > - if (handles == NULL) > + if (!access_ok(handles, count * sizeof(*handles))) > + return -EFAULT; > + > + syncobjs = kmalloc_array(count, sizeof(*syncobjs), GFP_KERNEL); > + if (!syncobjs) > return -ENOMEM; > > - if (copy_from_user(handles, user_handles, > - sizeof(uint32_t) * count_handles)) { > - ret = -EFAULT; > - goto err_free_handles; > - } > + for (i = 0; i < count; i++) { > + u64 handle; I believe this is a u32. > > - syncobjs = kmalloc_array(count_handles, sizeof(*syncobjs), GFP_KERNEL); > - if (syncobjs == NULL) { > - ret = -ENOMEM; > - goto err_free_handles; > - } > - > - for (i = 0; i < count_handles; i++) { > - syncobjs[i] = drm_syncobj_find(file_private, handles[i]); > + if (__get_user(handle, handles++)) { > + ret = -EFAULT; > + syncobjs[i] = NULL; Do we need to assign syncobjs[i] to NULL? Can we just go to err_put_syncobjs? > + goto err_put_syncobjs; > + } > + syncobjs[i] = drm_syncobj_find(file_private, handle); > if (!syncobjs[i]) { > ret = -ENOENT; > goto err_put_syncobjs; > } > } > > - kfree(handles); > *syncobjs_out = syncobjs; > return 0; > > err_put_syncobjs: > - while (i-- > 0) > - drm_syncobj_put(syncobjs[i]); > + while (i > 0) { > + if (syncobjs[i]) I'm not sure if I understand which scenario this would be needed. For the first syncobj that we don't find, we go to err_free_handles and AFAIU all previous syncobjs exist, so there wouldn't be a need to check if the syncobj != NULL. Best Regards, - Maíra > + drm_syncobj_put(syncobjs[i]); > + i--; > + } > kfree(syncobjs); > -err_free_handles: > - kfree(handles); > > return ret; > }
On 24/03/2025 22:29, Maíra Canal wrote: > Hi Tvrtko, > > On 18/03/25 12:54, Tvrtko Ursulin wrote: >> Drm_syncobj_array_find() helper is used from many userspace ioctl entry >> points with the task of looking up userspace handles to internal objects. >> >> We can easily avoid one temporary allocation by making it read the >> handles >> as it is looking them up. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> --- >> drivers/gpu/drm/drm_syncobj.c | 44 +++++++++++++++++------------------ >> 1 file changed, 21 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/ >> drm_syncobj.c >> index fd5ba6c89666..cdda2df06bec 100644 >> --- a/drivers/gpu/drm/drm_syncobj.c >> +++ b/drivers/gpu/drm/drm_syncobj.c >> @@ -1213,48 +1213,46 @@ signed long drm_timeout_abs_to_jiffies(int64_t >> timeout_nsec) >> EXPORT_SYMBOL(drm_timeout_abs_to_jiffies); >> static int drm_syncobj_array_find(struct drm_file *file_private, >> - void __user *user_handles, >> - uint32_t count_handles, >> + u32 __user *handles, >> + uint32_t count, >> struct drm_syncobj ***syncobjs_out) >> { >> - uint32_t i, *handles; >> struct drm_syncobj **syncobjs; >> + uint32_t i; >> int ret; >> - handles = kmalloc_array(count_handles, sizeof(*handles), >> GFP_KERNEL); >> - if (handles == NULL) >> + if (!access_ok(handles, count * sizeof(*handles))) >> + return -EFAULT; >> + >> + syncobjs = kmalloc_array(count, sizeof(*syncobjs), GFP_KERNEL); >> + if (!syncobjs) >> return -ENOMEM; >> - if (copy_from_user(handles, user_handles, >> - sizeof(uint32_t) * count_handles)) { >> - ret = -EFAULT; >> - goto err_free_handles; >> - } >> + for (i = 0; i < count; i++) { >> + u64 handle; > > I believe this is a u32. Well spotted. >> - syncobjs = kmalloc_array(count_handles, sizeof(*syncobjs), >> GFP_KERNEL); >> - if (syncobjs == NULL) { >> - ret = -ENOMEM; >> - goto err_free_handles; >> - } >> - >> - for (i = 0; i < count_handles; i++) { >> - syncobjs[i] = drm_syncobj_find(file_private, handles[i]); >> + if (__get_user(handle, handles++)) { >> + ret = -EFAULT; >> + syncobjs[i] = NULL; > > Do we need to assign syncobjs[i] to NULL? Can we just go to > err_put_syncobjs? > >> + goto err_put_syncobjs; >> + } >> + syncobjs[i] = drm_syncobj_find(file_private, handle); >> if (!syncobjs[i]) { >> ret = -ENOENT; >> goto err_put_syncobjs; >> } >> } >> - kfree(handles); >> *syncobjs_out = syncobjs; >> return 0; >> err_put_syncobjs: >> - while (i-- > 0) >> - drm_syncobj_put(syncobjs[i]); >> + while (i > 0) { >> + if (syncobjs[i]) > > I'm not sure if I understand which scenario this would be needed. For > the first syncobj that we don't find, we go to err_free_handles and > AFAIU all previous syncobjs exist, so there wouldn't be a need to check > if the syncobj != NULL. Well spotted again. I don't remember why I thought I needed to change this since it looks the old unwind works fine with added __get_user failure path. Regards, Tvrtko >> + drm_syncobj_put(syncobjs[i]); >> + i--; >> + } >> kfree(syncobjs); >> -err_free_handles: >> - kfree(handles); >> return ret; >> } >
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index fd5ba6c89666..cdda2df06bec 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -1213,48 +1213,46 @@ signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec) EXPORT_SYMBOL(drm_timeout_abs_to_jiffies); static int drm_syncobj_array_find(struct drm_file *file_private, - void __user *user_handles, - uint32_t count_handles, + u32 __user *handles, + uint32_t count, struct drm_syncobj ***syncobjs_out) { - uint32_t i, *handles; struct drm_syncobj **syncobjs; + uint32_t i; int ret; - handles = kmalloc_array(count_handles, sizeof(*handles), GFP_KERNEL); - if (handles == NULL) + if (!access_ok(handles, count * sizeof(*handles))) + return -EFAULT; + + syncobjs = kmalloc_array(count, sizeof(*syncobjs), GFP_KERNEL); + if (!syncobjs) return -ENOMEM; - if (copy_from_user(handles, user_handles, - sizeof(uint32_t) * count_handles)) { - ret = -EFAULT; - goto err_free_handles; - } + for (i = 0; i < count; i++) { + u64 handle; - syncobjs = kmalloc_array(count_handles, sizeof(*syncobjs), GFP_KERNEL); - if (syncobjs == NULL) { - ret = -ENOMEM; - goto err_free_handles; - } - - for (i = 0; i < count_handles; i++) { - syncobjs[i] = drm_syncobj_find(file_private, handles[i]); + if (__get_user(handle, handles++)) { + ret = -EFAULT; + syncobjs[i] = NULL; + goto err_put_syncobjs; + } + syncobjs[i] = drm_syncobj_find(file_private, handle); if (!syncobjs[i]) { ret = -ENOENT; goto err_put_syncobjs; } } - kfree(handles); *syncobjs_out = syncobjs; return 0; err_put_syncobjs: - while (i-- > 0) - drm_syncobj_put(syncobjs[i]); + while (i > 0) { + if (syncobjs[i]) + drm_syncobj_put(syncobjs[i]); + i--; + } kfree(syncobjs); -err_free_handles: - kfree(handles); return ret; }
Drm_syncobj_array_find() helper is used from many userspace ioctl entry points with the task of looking up userspace handles to internal objects. We can easily avoid one temporary allocation by making it read the handles as it is looking them up. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> --- drivers/gpu/drm/drm_syncobj.c | 44 +++++++++++++++++------------------ 1 file changed, 21 insertions(+), 23 deletions(-)