On 2/5/26 14:56, Srinivasan Shanmugam wrote:
> drm_gem_objects_lookup() can allocate the output array and take
> references on GEM objects before it fails.
>
> If an error happens part-way through, callers previously had to clean up
> partially created results themselves. This relied on subtle and
> undocumented behavior and was easy to get wrong.
>
> Make drm_gem_objects_lookup() clean up on failure. The function now
> drops any references it already took, frees the array, and sets
> *objs_out to NULL before returning an error.
>
> On success, behavior is unchanged. Existing callers remain correct and
> their error cleanup paths simply do nothing when *objs_out is NULL.
>
> v3: Move partial-lookup cleanup into objects_lookup(), perform reference
> dropping outside the lock, and remove reliance on __GFP_ZERO or implicit
> NULL handling. (Christian)
>
> Suggested-by: Christian König <[email protected]>
> Suggested-by: Tvrtko Ursulin <[email protected]>
> Signed-off-by: Srinivasan Shanmugam <[email protected]>
> ---
> drivers/gpu/drm/drm_gem.c | 37 +++++++++++++++++++++++++------------
> 1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index a1a9c828938b..da18e49a8bde 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -759,6 +759,11 @@ static int objects_lookup(struct drm_file *filp, u32
> *handle, int count,
> }
> spin_unlock(&filp->table_lock);
>
> + if (ret) {
Might look better with a "goto error" instead of the break in the loop.
> + while (--i >= 0)
The usual pattern is to use while(i--), otherwise we rely on that i is unsigned.
> + drm_gem_object_put(objs[i]);
> + }
> +
> return ret;
> }
>
> @@ -775,9 +780,11 @@ static int objects_lookup(struct drm_file *filp, u32
> *handle, int count,
> * For a single handle lookup, use drm_gem_object_lookup().
> *
> * Returns:
> - * @objs filled in with GEM object pointers. Returned GEM objects need to be
> - * released with drm_gem_object_put(). -ENOENT is returned on a lookup
> - * failure. 0 is returned on success.
> + * On success, *@objs_out is set to an allocated array of @count pointers
> + * containing GEM objects. The caller must drop the references with
> + * drm_gem_object_put() and free the array with kvfree().
> + *
> + * On failure, *@objs_out is set to NULL and no further action is required.
> *
> */
> int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
> @@ -785,36 +792,42 @@ int drm_gem_objects_lookup(struct drm_file *filp, void
> __user *bo_handles,
> {
> struct drm_device *dev = filp->minor->dev;
> struct drm_gem_object **objs;
> - u32 *handles;
> + u32 *handles = NULL;
> int ret;
>
> + *objs_out = NULL;
> if (!count)
> return 0;
>
> - objs = kvmalloc_array(count, sizeof(struct drm_gem_object *),
> - GFP_KERNEL | __GFP_ZERO);
> + objs = kvmalloc_array(count, sizeof(*objs), GFP_KERNEL);
> if (!objs)
> return -ENOMEM;
>
> - *objs_out = objs;
> -
> handles = kvmalloc_array(count, sizeof(u32), GFP_KERNEL);
> if (!handles) {
> ret = -ENOMEM;
> - goto out;
> + goto err_free_objs;
> }
>
> if (copy_from_user(handles, bo_handles, count * sizeof(u32))) {
> ret = -EFAULT;
> drm_dbg_core(dev, "Failed to copy in GEM handles\n");
> - goto out;
> + goto err_free_handles;
> }
>
> ret = objects_lookup(filp, handles, count, objs);
> -out:
> kvfree(handles);
> - return ret;
> + if (ret)
> + goto err_free_objs;
Na, make that goto err_free_handles and move the kvfree later.
Both works but I think that's cleaner.
Christian.
>
> + *objs_out = objs;
> + return 0;
> +
> +err_free_handles:
> + kvfree(handles);
> +err_free_objs:
> + kvfree(objs);
> + return ret;
> }
> EXPORT_SYMBOL(drm_gem_objects_lookup);
>
