On 2/5/26 10:59, Srinivasan Shanmugam wrote: > drm_gem_objects_lookup() may allocate the output array and acquire > references on GEM objects before returning an error. This requires > callers to handle partial cleanup correctly, which is not obvious and > easy to get wrong. > > Introduce drm_gem_objects_lookup_safe(), a wrapper helper that > guarantees cleanup on failure. If lookup fails, the helper drops any > acquired object references, frees the array, and sets the output pointer > to NULL. > > This provides a safer alternative for new and fragile call sites without > changing the behavior of drm_gem_objects_lookup() or affecting existing > callers. > > 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]> > Change-Id: I553551dd1e7aadf1b2a477eaf19ce9c80b2ce2ea > --- > drivers/gpu/drm/drm_gem.c | 48 +++++++++++++++++++++++++++++++++++++++ > include/drm/drm_gem.h | 2 ++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 0f8013b9377e..f1d13700a62c 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -830,6 +830,54 @@ int drm_gem_objects_lookup(struct drm_file *filp, void > __user *bo_handles, > } > EXPORT_SYMBOL(drm_gem_objects_lookup); > > +/** > + * drm_gem_objects_lookup_safe - look up GEM objects from an array of > handles with failure cleanup > + * @filp: DRM file private data > + * @bo_handles: user pointer to array of userspace handles > + * @count: size of handle array > + * @objs_out: returned pointer to array of drm_gem_object pointers > + * > + * Wrapper around drm_gem_objects_lookup() that guarantees cleanup on > failure. > + * > + * On success, *@objs_out is set to an allocated array of @count pointers > + * containing GEM objects. The caller must drop references with > + * drm_gem_object_put() and free the array with kvfree(). > + * > + * On failure, this helper will drm_gem_object_put() any successfully looked > up > + * objects, free the array, and set *@objs_out to NULL. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_objects_lookup_safe(struct drm_file *filp, void __user > *bo_handles, > + int count, struct drm_gem_object ***objs_out) > +{ > + struct drm_gem_object **objs; > + int ret, i; > + > + /* Ensure callers never see stale pointers on failure. */ > + *objs_out = NULL; > + > + ret = drm_gem_objects_lookup(filp, bo_handles, count, objs_out); > + if (!ret) > + return 0; > + > + objs = *objs_out; > + if (!objs) > + return ret; > + > + for (i = 0; i < count; i++) { > + if (objs[i]) > + drm_gem_object_put(objs[i]); > + } > + > + kvfree(objs); > + *objs_out = NULL;
I see the point with the existing error handling now, but that is clearly really fragile. I would clearly prefer that when something unforeseen happens that we drop the GEM reference to the already looked up objects inside the function and also cleanup the objs_out pointer by freeing it and setting it to NULL. Regards, Christian. > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_objects_lookup_safe); > + > /** > * drm_gem_object_lookup - look up a GEM object from its handle > * @filp: DRM file private date > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 8d48d2af2649..7decce2fdef7 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -561,6 +561,8 @@ void drm_gem_vunmap(struct drm_gem_object *obj, struct > iosys_map *map); > > int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles, > int count, struct drm_gem_object ***objs_out); > +int drm_gem_objects_lookup_safe(struct drm_file *filp, void __user > *bo_handles, > + int count, struct drm_gem_object ***objs_out); > struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 > handle); > long drm_gem_dma_resv_wait(struct drm_file *filep, u32 handle, > bool wait_all, unsigned long timeout);
