[PATCH 3/7] drm/syncobj: Avoid one temporary allocation in drm_syncobj_array_find

[Tvrtko Ursulin]

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 at 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;
>>   }
>