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

[Maíra Canal]

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.

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