[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. Thanks, Srini > > 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
