Re: [PATCH RFC v2 10/24] drm/encoder: add drm_encoder_cleanup_from()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm: list corruption in privobj_list during dynamic bridge detachment
--

commit 77c9ca5601fd825a1740e548bb2319aadc1fb84a
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>

drm/encoder: add drm_encoder_cleanup_from()

Supporting hardware whose final part of the DRM pipeline can be physically
removed requires the ability to detach all bridges from a given point to
the end of the pipeline. Introduce a variant of drm_encoder_cleanup()
for this.

> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 0d5dbed06db44..40ece477b3025 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
[ ... ]
> @@ -179,6 +179,44 @@ int drm_encoder_init(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_encoder_init);
>  
> +/**
> + * drm_encoder_cleanup_from - remove a given bridge and all the following
> + * @encoder: encoder whole list of bridges shall be pruned
> + * @bridge: first bridge to remove
> + *
> + * Removes from an encoder all the bridges starting with a given bridge
> + * and until the end of the chain.
> + *
> + * Does nothing if the bridge is not attached to an encoder chain.
> + *
> + * This should not be used in "normal" DRM pipelines. It is only useful for
> + * devices whose final part of the DRM chain can be physically removed and
> + * later reconnected (possibly with different hardware).
> + */
> +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
> +	 * drm_bridge_detach() will call DRM_MODESET_LOCK_ALL_BEGIN() (in
> +	 * drm_modeset_lock_fini()), resulting in a possible ABBA circular
> +	 * deadlock. Avoid it by first moving all the bridges to a
> +	 * temporary list holding the lock, and then calling
> +	 * drm_bridge_detach() without the lock.
> +	 */
> +	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);
> +
> +	while (!list_empty(&tmplist))
> +		drm_bridge_detach(list_first_entry(&tmplist, struct drm_bridge, chain_node));

[Severity: High]
Does this dynamically detach the bridge without acquiring the
connection_mutex?

If a hardware bridge unplug triggers drm_bridge_remove(), this calls:
drm_encoder_cleanup_from() -> drm_bridge_detach() ->
drm_atomic_private_obj_fini()

In drm_atomic_private_obj_fini(), it executes:
    list_del(&obj->head);
on dev->mode_config.privobj_list without holding connection_mutex or any
other list-protecting lock.

Can this create a fatal race condition with concurrent lockless readers?

Concurrent userspace DRM IOCTLs (like modeset or atomic commits) can invoke
drm_modeset_lock_all_ctx(), which iterates over privobj_list locklessly
using list_for_each_entry(). The lockless iteration would race with the
un-synchronized list_del() triggered here, potentially corrupting the list
traversal for the reader and causing a crash.

> +}
> +EXPORT_SYMBOL(drm_encoder_cleanup_from);
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-drm-bridge-hotplug-v2-0-8e34986dcb68@bootlin.com?part=10