On 2/5/26 15:32, 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)
>
> v4: Use goto-style error handling in objects_lookup(), drop partial
> references outside the lock, and simplify drm_gem_objects_lookup()
> cleanup by routing failures through err_free_handles as suggested.
> (Christian)
>
> Cc: Alex Deucher <[email protected]>
> 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 | 48 ++++++++++++++++++++++++++-------------
> 1 file changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index a1a9c828938b..e1218728352d 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -750,14 +750,22 @@ static int objects_lookup(struct drm_file *filp, u32
> *handle, int count,
> for (i = 0; i < count; i++) {
> /* Check if we currently have a reference on the object */
> obj = idr_find(&filp->object_idr, handle[i]);
> - if (!obj) {
> - ret = -ENOENT;
> - break;
> - }
> + if (!obj)
> + goto err;
> +
> drm_gem_object_get(obj);
> objs[i] = obj;
> }
> +
> spin_unlock(&filp->table_lock);
> + return 0;
> +
> +err:
> + ret = -ENOENT;
I think you can now also drop the ret local variable.
> + spin_unlock(&filp->table_lock);
> +
> + while (i--)
> + drm_gem_object_put(objs[i]);
>
> return ret;
> }
> @@ -775,9 +783,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 +795,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);
As pointed out by Tvrtko as well please rebase that on top of drm-misc-next.
This code here has changed there.
Regards,
Christian.
> 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:
> + if (ret)
> + goto err_free_handles;
> +
> kvfree(handles);
> - return ret;
> + *objs_out = objs;
> + return 0;
>
> +err_free_handles:
> + kvfree(handles);
> +err_free_objs:
> + kvfree(objs);
> + return ret;
> }
> EXPORT_SYMBOL(drm_gem_objects_lookup);
>
