On 06/02/2026 08:09, Christian König wrote:
On 2/5/26 16:42, 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)
v5: Rebase on drm-misc-next, drop the ret local variable. (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]>
One little nit pick below, apart from that:
Reviewed-by: Christian König <[email protected]>
Same opinion on the nit pick as Christian and with that the rest LGTM:
Reviewed-by: Tvrtko Ursulin <[email protected]>
Regards,
Tvrtko
---
drivers/gpu/drm/drm_gem.c | 45 +++++++++++++++++++++++++++------------
1 file changed, 31 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 7ff6b7bbeb73..5895ae09c27d 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -783,7 +783,7 @@ EXPORT_SYMBOL(drm_gem_put_pages);
static int objects_lookup(struct drm_file *filp, u32 *handle, int count,
struct drm_gem_object **objs)
{
- int i, ret = 0;
+ int i;
struct drm_gem_object *obj;
spin_lock(&filp->table_lock);
@@ -791,16 +791,23 @@ 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;
- return ret;
+err:
+ spin_unlock(&filp->table_lock);
+
+ while (i--)
+ drm_gem_object_put(objs[i]);
+
+ return -ENOENT;
}
/**
@@ -825,27 +832,37 @@ int drm_gem_objects_lookup(struct drm_file *filp, void
__user *bo_handles,
int count, struct drm_gem_object ***objs_out)
{
struct drm_gem_object **objs;
- u32 *handles;
+ u32 *handles = NULL;
I might be missing something but setting handles to NULL looks superfluous to
me and we now have static checkers warning about superfluous initialization.
Regards,
Christian.
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 = vmemdup_array_user(bo_handles, count, sizeof(u32));
- if (IS_ERR(handles))
- return PTR_ERR(handles);
+ if (IS_ERR(handles)) {
+ ret = PTR_ERR(handles);
+ goto err_free_objs;
+ }
ret = objects_lookup(filp, handles, count, objs);
+ 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);
