On 05/02/2026 09: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.
Nitpick - maybe the fact that it is a wrapper is not relevant for API
docs but just say along the lines of "Similiar to
drm_gem_objects_lookup() but 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.
Maybe just say "On failure no further action is required."?
+ *
+ * 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;
This seems redundant since all paths below are either success, *objs_out
already NULL, or *obj_out set to NULL at the very end?
+
+ 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]);
It appears drm_gem_object_put handles the NULL pointer on its own so you
could remove it if you want.
+ }
+
+ kvfree(objs);
+ *objs_out = NULL;
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(drm_gem_objects_lookup_safe);
drm_gem_objects_lookup is EXPORT_SYMBOL and EXPORT_SYMBOL_GPL in general
seem a minority in DRM. But TBH I am not sure what is the criteria.
Now one higher level question. Could we convert at least one driver to
the new helper? Both because it is best not to add unused helpers, and
also because it would be even better to later retire the unsafe version
altogether.
Since the drivers already have to cope with checking the pointer and
handles themselves, in order to properly cleanup on failure, perhaps
making them use the new helper would just work.
Or even better, maybe all callers would cope just fine and the
_existing_ drm_gem_objects_lookup could be simply made do the cleanup,
without the need to add a new helper.
Regards,
Tvrtko
+
/**
* 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);
