Re: [PATCH v6 03/28] drm/bridge: Switch print_state to use bridge name

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm_bridge: ignored return from drm_atomic_private_obj_init causes crash
- [Low] drm_bridge: degraded debug output for statically allocated bridges
--

commit e5b998c537e0f9eb83be0fe1e66e8f70427ccfcc
Author: Maxime Ripard <mripard@kernel.org>
Subject: drm/bridge: Switch print_state to use bridge name

This commit updates drm_bridge_atomic_print_priv_state() to print the
human-readable bridge->name instead of the function pointer bridge->funcs.
This is intended to make the state readout output more consistent and
easier to interpret.

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index d474d60204110..447fe6d333b3b 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -563,9 +563,9 @@ drm_bridge_atomic_print_priv_state(struct drm_printer *p,
>  	struct drm_bridge *bridge = drm_priv_to_bridge(s->obj);
>  
>  	if (bridge->of_node)
> -		drm_printf(p, "bridge: %ps (%pOFfc)\n", bridge->funcs, bridge->of_node);
> +		drm_printf(p, "bridge: %s (%pOFfc)\n", bridge->name, bridge->of_node);
>  	else
> -		drm_printf(p, "bridge: %ps\n", bridge->funcs);
> +		drm_printf(p, "bridge: %s\n", bridge->name);

[Severity: Low]
In drivers/gpu/drm/drm_bridge.c:drm_bridge_atomic_print_priv_state(),
for bridges that do not use devm_drm_bridge_alloc() (e.g., legacy or
statically allocated bridges), bridge->name is never initialized and
remains NULL.

When bridge->name is NULL, vsprintf safely intercepts the NULL pointer
and prints '(null)'.

Does this degrade the debug output compared to the previous behavior
(which printed the function pointer symbol for identification), defeating
the goal for these bridges?

[Severity: High]
While looking into the bridge->name changes, I noticed a related issue in
drivers/gpu/drm/drm_bridge.c:drm_bridge_attach(), where the return value
of drm_atomic_private_obj_init() is ignored.

A preceding patch in this series added a dynamically allocated name
parameter to drm_atomic_private_obj_init() which fails with -ENOMEM if
the allocation fails:

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 bridge->name is NULL for statically allocated bridges,
kstrdup(NULL) returns NULL, causing drm_atomic_private_obj_init() to
unconditionally fail. Since drm_bridge_attach() ignores the return value:

drivers/gpu/drm/drm_bridge.c:drm_bridge_attach() {
    ...
	drm_atomic_private_obj_init(bridge->dev, &bridge->base, bridge->name,
				    &drm_bridge_priv_state_funcs);

	return 0;
}

The private object is left zeroed and uninitialized.

Will this cause a deterministic NULL pointer dereference during teardown
when drm_bridge_detach() calls drm_atomic_private_obj_fini(), which
executes list_del(&obj->head) on a zeroed list_head? Does it also cause a
crash during atomic commits when drm_atomic_helper_swap_state() attempts
to dereference old_obj_state->state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=3