Re: [PATCH 23/37] drm/encoder: add drm_encoder_cleanup_from()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Infinite loop (kernel lockup) if `drm_bridge_detach()` returns early.
- [High] Unsafe list membership check leads to out-of-bounds traversal and memory corruption.
- [High] Moving bridges to a stack list breaks bridge chain iterators during detach callbacks.
--

commit 36261e8ecb547e8946c95a0aed4ce37428b2d56e
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>

drm/encoder: add drm_encoder_cleanup_from()

Introduce a variant of drm_encoder_cleanup() to detach all bridges from
a given point to the end of the pipeline.

> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
[ ... ]
> +void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge)
> +{
> +	struct drm_bridge *next;
> +	LIST_HEAD(tmplist);
> +
> +	/*
> +	 * We need the bridge_chain_mutex to modify the chain, but
[ ... ]
> +	 */
> +	mutex_lock(&encoder->bridge_chain_mutex);
> +	if (!list_empty(&bridge->chain_node))
> +		list_for_each_entry_safe_from(bridge, next, &encoder->bridge_chain, chain_node)
> +			list_move_tail(&bridge->chain_node, &tmplist);
> +	mutex_unlock(&encoder->bridge_chain_mutex);

Could this list_empty() check lead to an out-of-bounds traversal?

The check confirms the bridge is on a list, but not necessarily the
provided encoder's chain. If a caller mistakenly passes a bridge from a
different encoder, or if a concurrent caller processes a bridge already moved
to another thread's tmplist, list_for_each_entry_safe_from() will iterate
over that unrelated list.

Since the loop termination checks against &encoder->bridge_chain, it will
never find the list head, running past the end of the foreign list and
corrupting memory.

> +
> +	while (!list_empty(&tmplist))
> +		drm_bridge_detach(list_first_entry(&tmplist, struct drm_bridge, chain_node));

Can this loop become infinite if drm_bridge_detach() returns early?

Looking at drm_bridge_detach() in drm_bridge.c:

void drm_bridge_detach(struct drm_bridge *bridge)
{
        if (WARN_ON(!bridge))
                return;

        if (WARN_ON(!bridge->dev))
                return;
...
        list_del_init(&bridge->chain_node);
...

If an early return triggers before list_del_init(), the bridge remains in
tmplist. The list never becomes empty, and the loop will process the exact
same failing bridge indefinitely. Should the bridge be unlinked from the
temporary list before calling drm_bridge_detach()?

Also, does moving bridges to a stack-allocated tmplist break bridge chain
iterators during the detach callbacks?

drm_bridge_detach() executes bridge->funcs->detach(bridge). If a driver
calls a chain traversal API like drm_bridge_chain_get_next_bridge() during
this detach callback, the macro checks:

list_is_last(&bridge->chain_node, &bridge->encoder->bridge_chain)

Because the bridge is currently on tmplist (a stack variable), this boundary
check fails. This causes the iterator to read past the list end, returning
the stack tmplist head cast as a struct drm_bridge *, leading to memory
corruption.

> +}
> +EXPORT_SYMBOL(drm_encoder_cleanup_from);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-drm-bridge-hotplug-v1-0-45e2bdb3dfb4@bootlin.com?part=23