On 05/02/2026 14:01, SHANMUGAM, SRINIVASAN wrote:
[AMD Official Use Only - AMD Internal Distribution Only]
-----Original Message-----
From: Tvrtko Ursulin <[email protected]>
Sent: Thursday, February 5, 2026 6:33 PM
To: SHANMUGAM, SRINIVASAN <[email protected]>;
Koenig, Christian <[email protected]>; Deucher, Alexander
<[email protected]>
Cc: [email protected]
Subject: Re: [PATCH v2] drm/gem: Make drm_gem_objects_lookup() self-cleaning
on failure
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?
Yes.
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().
I have proposed v3, based on this , will check with Alex or Christian regarding
the rebasing part
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.
To keep this change focused, I suggest we first land the self-cleaning behavior
and then take up the helper as a separate follow-up patch. I’m happy to align
this with Christian’s preference as well.
Follow up is absolutely fine.
Btw, add dri-devel to the cc for next respin.
Regards,
Tvrtko
