Re: [PATCH v8 07/39] drm/atomic: Add drm_private_obj name

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/atomic: leak and uninitialized state if
  drm_atomic_private_obj_init() fails
--

commit f4a0998ec2ff1c620aa6e4e7d195fce0e092e47a
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic: Add drm_private_obj name

This commit adds a human-readable name field to struct drm_private_obj,
initialized via drm_atomic_private_obj_init() and updated via
drm_atomic_private_obj_set_name(), and updates callers to provide a name.

[ ... ]

> diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c b/drivers/gpu/drm/display/drm_dp_tunnel.c
> index d29ad4116e641..5f30f82b2ebb2 100644
> --- a/drivers/gpu/drm/display/drm_dp_tunnel.c
> +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c
> @@ -389,6 +389,14 @@ lookup_or_alloc_group(struct drm_dp_tunnel_mgr *mgr, u8 drv_group_id)
>  		 tunnel_group_drv_id(drv_group_id) & ((1 << DP_GROUP_ID_BITS) - 1),
>  		 tunnel_group_id(drv_group_id) & ((1 << DP_USB4_DRIVER_ID_BITS) - 1));
>  
> +	/*
> +	 * The private object was named after the group index when it
> +	 * was initialized, since the group identifier wasn't known
> +	 * yet. Refresh it now that we have one. The name is only used
> +	 * for debug messages, so ignore any allocation failure.
> +	 */
> +	drm_atomic_private_obj_set_name(&group->base, group->name);
> +
>  	return group;
>  }
>  
> @@ -1727,11 +1735,22 @@ EXPORT_SYMBOL(drm_dp_tunnel_atomic_get_new_state);
>  
>  static bool init_group(struct drm_dp_tunnel_mgr *mgr, struct drm_dp_tunnel_group *group)
>  {
> +	char name[32];
> +
>  	group->mgr = mgr;
>  	group->available_bw = -1;
>  	INIT_LIST_HEAD(&group->tunnels);
>  
> -	drm_atomic_private_obj_init(mgr->dev, &group->base,
> +	/*
> +	 * The group identifier, and thus the group name, is only known
> +	 * once a tunnel has been added to the group. Name the private
> +	 * object after the group index until then,
> +	 * lookup_or_alloc_group() will refresh it.
> +	 */
> +	snprintf(name, sizeof(name), "dp-tunnel-group-%d",
> +		 (int)(group - mgr->groups));
> +
> +	drm_atomic_private_obj_init(mgr->dev, &group->base, name,
>  				    &tunnel_group_funcs);
>  
>  	return true;
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a1f18fc96b4ca..63647dd3204f6 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1048,6 +1048,7 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>   * drm_atomic_private_obj_init - initialize private object
>   * @dev: DRM device this object will be attached to
>   * @obj: private object
> + * @name: human-readable name for debug messages
>   * @funcs: pointer to the struct of function pointers that identify the object
>   * type
>   *
> @@ -1059,6 +1060,7 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>   */
>  int drm_atomic_private_obj_init(struct drm_device *dev,
>  				struct drm_private_obj *obj,
> +				const char *name,
>  				const struct drm_private_state_funcs *funcs)
>  {
>  	struct drm_private_state *state;
> @@ -1077,6 +1079,12 @@ int drm_atomic_private_obj_init(struct drm_device *dev,
>  
>  	obj->dev = dev;
>  	obj->funcs = funcs;
> +	obj->name = kstrdup(name, GFP_KERNEL);
> +	if (!obj->name) {
> +		ret = -ENOMEM;
> +		goto err_lock_fini;
> +	}
> +
>  	list_add_tail(&obj->head, &dev->mode_config.privobj_list);
>  
>  	state = obj->funcs->atomic_create_state(obj);
> @@ -1091,6 +1099,9 @@ int drm_atomic_private_obj_init(struct drm_device *dev,
>  
>  err_list_del:
>  	list_del_init(&obj->head);
> +	kfree(obj->name);
> +	obj->name = NULL;
> +err_lock_fini:
>  	drm_modeset_lock_fini(&obj->lock);
>  	return ret;
>  }
> @@ -1118,6 +1129,7 @@ drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>  
>  	list_del_init(&obj->head);
>  	obj->funcs->atomic_destroy_state(obj, obj->state);
> +	kfree(obj->name);
>  	drm_modeset_lock_fini(&obj->lock);
>  }
>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
> @@ -1151,6 +1163,37 @@ static int drm_atomic_commit_set_private_obj_state(struct drm_atomic_commit *com
>  	return 0;
>  }
>  
> +/**
> + * drm_atomic_private_obj_set_name - update the name of a private object
> + * @obj: private object
> + * @name: new human-readable name for debug messages
> + *
> + * Replaces the name of @obj previously set at
> + * drm_atomic_private_obj_init() time. This is meant for callers that
> + * only get a meaningful name well after the object has been
> + * initialized.
> + *
> + * RETURNS:
> + * Zero on success, a negative error code on allocation failure. The
> + * name is only used for debug messages, so callers can safely ignore
> + * the return value.
> + */
> +int drm_atomic_private_obj_set_name(struct drm_private_obj *obj,
> +				    const char *name)
> +{
> +	char *new_name;
> +
> +	new_name = kstrdup(name, GFP_KERNEL);
> +	if (!new_name)
> +		return -ENOMEM;
> +
> +	kfree(obj->name);
> +	obj->name = new_name;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_private_obj_set_name);

[Severity: Medium]
Can this leak obj->name or dereference a NULL obj->state when
drm_atomic_private_obj_init() fails in init_group()?

init_group() ignores the return value of drm_atomic_private_obj_init() and
returns true.  If kstrdup() or atomic_create_state() fails, group->base is
left with an empty obj->head and obj->state set to NULL.  When a tunnel is
later added to the group, lookup_or_alloc_group() calls
drm_atomic_private_obj_set_name(), which allocates a new obj->name even
though obj->head is empty, and cleanup_group() ->
drm_atomic_private_obj_fini() returns early on list_empty(&obj->head)
without freeing obj->name.  Duplicating the group's atomic state in
tunnel_group_duplicate_state() will also dereference the NULL obj->state.

Also, drm_atomic_private_obj_fini() does not reset obj->name to NULL after
kfree(obj->name), leaving a dangling pointer if the object is inspected or
renamed after finalization.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-drm-state-readout-v8-0-cbe78624e1bc@kernel.org?part=7