On 2/5/26 13: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"); > - 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]);
This here works only because the objs array is allocated with __GFP_ZERO and drm_gem_object_put() can deal with NULL pointers. That not obvious at all and it would be nice if we could avoid using __GFP_ZERO as well. So I think we should just add proper error handling to objects_lookup(), so that the function drops the references from already looked up objects again if something went wrong. Just keep in mind that this needs to be outside of the lock protected area. Regards, Christian. > > + kvfree(objs); > + return ret; > } > EXPORT_SYMBOL(drm_gem_objects_lookup); >
