Re: [PATCH v7 05/34] drm/atomic: Add drm_private_obj name

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/bridge: NULL bridge->name causes drm_atomic_private_obj_init() failure
- [Medium] drm/dp_tunnel: empty group->name duplicated in init_group()
--

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

drm/atomic: Add drm_private_obj name

This commit adds a human-readable name field to drm_private_obj,
allocated in drm_atomic_private_obj_init() and freed in
drm_atomic_private_obj_fini(), and updates existing callers to pass a
name.

> diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c b/drivers/gpu/drm/display/drm_dp_tunnel.c
> index d29ad4116e641..391d643793eb3 100644
> --- a/drivers/gpu/drm/display/drm_dp_tunnel.c
> +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c
> @@ -1732,6 +1732,7 @@ static bool init_group(struct drm_dp_tunnel_mgr *mgr, struct drm_dp_tunnel_group
>  	INIT_LIST_HEAD(&group->tunnels);
>  
>  	drm_atomic_private_obj_init(mgr->dev, &group->base,
> +				    group->name,
>  				    &tunnel_group_funcs);
>  
>  	return true;

[Severity: Medium]
Is group->name populated when init_group() runs?

In drm_dp_tunnel_mgr_create(), mgr->groups is zero-allocated via
kzalloc_objs() immediately before init_group() is called on each element:

drivers/gpu/drm/display/drm_dp_tunnel.c:drm_dp_tunnel_mgr_create() {
	mgr->groups = kzalloc_objs(*mgr->groups, max_group_count);
	...
	for (i = 0; i < max_group_count; i++) {
		if (!init_group(mgr, &mgr->groups[i])) {
}

Because drm_atomic_private_obj_init() duplicates the passed string with
kstrdup(), group->base.name is initialized to "" while group->name is only
formatted later when a tunnel is added in lookup_or_alloc_group():

drivers/gpu/drm/display/drm_dp_tunnel.c:lookup_or_alloc_group() {
	...
	snprintf(group->name, sizeof(group->name), "%d:%d:*",
		 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));
}

Won't this leave group->base.name permanently set to an empty string for
every DP tunnel group?

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index e7f9e6d589272..ac65ac9416919 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -317,6 +317,7 @@ static void __drm_bridge_free(struct kref *kref)
>  
>  	drm_bridge_put(bridge->next_bridge);
>  
> +	kfree(bridge->name);
>  	kfree(bridge->container);
>  }
>  
> @@ -418,6 +419,13 @@ void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset,
>  		return ERR_PTR(-ENOMEM);
>  
>  	bridge = container + offset;
> +
> +	bridge->name = kasprintf(GFP_KERNEL, "bridge-%s", dev_name(dev));
> +	if (!bridge->name) {
> +		kfree(container);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
>  	INIT_LIST_HEAD(&bridge->list);
>  	bridge->container = container;
>  	bridge->funcs = funcs;
> @@ -649,7 +657,7 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  			goto err_reset_bridge;
>  	}
>  
> -	drm_atomic_private_obj_init(bridge->dev, &bridge->base,
> +	drm_atomic_private_obj_init(bridge->dev, &bridge->base, bridge->name,
>  				    &drm_bridge_priv_state_funcs);
>  
>  	return 0;

[Severity: High]
What happens in drm_bridge_attach() if a bridge was not allocated via
devm_drm_bridge_alloc() and therefore has bridge->name == NULL?

For example, ingenic_drm_bind() allocates struct ingenic_drm_bridge via
drmm_encoder_alloc() and attaches &ib->bridge:

drivers/gpu/drm/ingenic/ingenic-drm-drv.c:ingenic_drm_bind() {
		ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
					NULL, DRM_MODE_ENCODER_DPI, NULL);
		...
		ib->bridge.funcs = &ingenic_drm_bridge_funcs;
		ib->next_bridge = bridge;

		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
}

When bridge->name is NULL, kstrdup(name, GFP_KERNEL) in
drm_atomic_private_obj_init() returns NULL and aborts with -ENOMEM before
adding bridge->base to dev->mode_config.privobj_list or calling
atomic_create_state() to initialize bridge->base.state:

drivers/gpu/drm/drm_atomic.c:drm_atomic_private_obj_init() {
	obj->name = kstrdup(name, GFP_KERNEL);
	if (!obj->name) {
		ret = -ENOMEM;
		goto err_lock_fini;
	}
}

Because drm_bridge_attach() ignores the return value of
drm_atomic_private_obj_init(), attach succeeds with bridge->base.state ==
NULL, causing subsequent atomic commits via
drm_atomic_add_encoder_bridges() -> drm_atomic_get_bridge_state() ->
drm_atomic_helper_bridge_duplicate_state() to trigger
WARN_ON(!bridge->base.state) and fail with -ENOMEM. Can
drm_atomic_private_obj_init() or drm_bridge_attach() handle a NULL bridge
name gracefully?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-drm-state-readout-v7-0-25e5163d75c3@kernel.org?part=5