On 05/02/2026 12:13, 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.
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 | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index a1a9c828938b..862c9b2b9c0c 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -775,19 +775,21 @@ 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,
int count, struct drm_gem_object ***objs_out)
{
struct drm_device *dev = filp->minor->dev;
struct drm_gem_object **objs;
- u32 *handles;
- int ret;
+ u32 *handles = NULL;
+ int ret, i;
+ *objs_out = NULL;
if (!count)
return 0;
@@ -796,25 +798,34 @@ int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles,
if (!objs)
return -ENOMEM;
- *objs_out = objs;
-
handles = kvmalloc_array(count, sizeof(u32), GFP_KERNEL);
if (!handles) {
ret = -ENOMEM;
- goto out;
+ goto err_put_free;
}
if (copy_from_user(handles, bo_handles, count * sizeof(u32))) {
ret = -EFAULT;
drm_dbg_core(dev, "Failed to copy in GEM handles\n");
I think this patch is possibly against the AMD staging branch? You will
either need to rebase on top of drm-tip, or AMD staging needs to catch
up with 6.19-rc1. Because in there I have replaced the above with
vmemdup_array_user().
Otherwise I agree with what Christian said, that it would be nice to get
rid of the __GFP_ZERO requirement and handle it in objects_lookup().
In the meantime I have again went through all the callers and it seems
they will all handle not having to do the cleanup just fine.
Hm, another interesting question if we maybe want to add a cleanup
helper so the callers do not have to know that they have to use kvfree?
All of them have their own loops so that would also nicely consolidate.
void drm_gem_objects_cleanup(struct drm_gem_object **objs_out, int
count) ? drm_gem_objects_lookup_free? drm_gem_objects_lookup_cleanup (!)?
Regards,
Tvrtko
- goto out;
+ goto err_put_free;
}
ret = objects_lookup(filp, handles, count, objs);
-out:
+ if (ret)
+ goto err_put_free;
+
kvfree(handles);
- return ret;
+ *objs_out = objs;
+ return 0;
+
+err_put_free:
+ kvfree(handles);
+
+ for (i = 0; i < count; i++)
+ drm_gem_object_put(objs[i]);
+ kvfree(objs);
+ return ret;
}
EXPORT_SYMBOL(drm_gem_objects_lookup);
