Re: [PATCH v5 2/7] drm/encoder: drm_encoder_cleanup: lock the encoder chain mutex during removal

[Louis Chauvet <louis.chauvet@bootlin.com>]

On 3/24/26 9:58 AM, Luca Ceresoli wrote:
> drm_encoder_cleanup() modifies the encoder chain by removing bridges via
> drm_bridge_detach(). Protect this whole operation by taking the mutex, so
> that:
> 
>   * any users iterating over the chain will not access it during the change
>   * other code willing to modify the list (drm_bridge_attach()) will wait
>     until drm_encoder_cleanup() is done
> 
> Note that the _safe macro in use here is providing a different and
> orthogonal kind of protection than the mutex:
> 
>   1. list_for_each_entry_safe() allows removing the current entry from the
>      list it is iterating on, synchronously; the non-safe version would be
>      unable to find the next entry after the current entry has been removed
>   2. the mutex being added allows to ensure that the list is not used
>      asynchronously by other code while it is being modified; this prevents
>      such other concurrent code to derail because it is iterating over an
>      element while it is removed
> 
> The _safe macro, which works by taking the "next" pointer in addition to
> the "current" one, does not even try to provide the protection at item 2
> above. This is visible e.g. when the "next" element is removed by other
> concurrent code. This is what would happen without the added mutex:
> 
>   1. start loop: list_for_each_entry_safe(pos, n, ...) sets:
>      pos = list_first_entry()   = (bridge 1)
>      n   = list_next_entry(pos) = (bridge 2)
>   2. enter the loop 1st time, do something with *pos (bridge 1)
>   3. in the meanwhile bridge 2 is hot-unplugged
>      -> another thread removes bridge 2
>         -> drm_bridge_detach()
>            -> list_del() sets (bridge 2)->next = LIST_POISON1
>   4. loop iteration 1 finishes, list_for_each_entry_safe() sets:
>      pos = n (previously set to bridge 2)
>      n   = (bridge 2)->next = LIST_POISON1
>   5. enter the loop 2nd time, do something with *pos (bridge 2)
>   6. loop iteration 2 finishes, list_for_each_entry_safe() sets:
>      pos = n = LIST_POISON1 ==> bug!
> 
> However, simply adding mutex_[un]lock(&encoder->bridge_chain_mutex)
> before/after the list_for_each_entry_safe() seems a simple and good
> solution, but it is introducing a possible ABBA deadlock (found by
> PROVE_LOCKING). The two code paths involved are:
> 
>   * drm_encoder_cleanup():
>     - takes the bridge_chain_mutex (A)
>     - calls drm_bridge_detach -> drm_atomic_private_obj_fini ->
>       DRM_MODESET_LOCK_ALL_BEGIN() which takes all locks in the
>       acquisition context (B)
>   * drm_mode_getconnector() (and other code paths):
>     - calls drm_helper_probe_single_connector_modes() which:
>       - takes a drm_modeset_lock in the acquisition context (B)
>       - calls __drm_helper_update_and_validate ->
>         drm_bridge_chain_mode_valid -> drm_for_each_bridge_in_chain_from()
>         which takes the bridge_chain_mutex (A)
> 
> To avoid this potential ABBA deadlock, move all list items to a temporary
> list while holding the bridge_chain_mutex, then detach all elements from
> the temporary list without the mutex.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> 
> ---
> 
> Changes in v5:
> - Small commit message improvement
> 
> Changes in v3:
> - Prevent ABBA deadlock by using a temporary list
> - Improve commit message
> 
> Changes in v2:
> - Expanded commit messge with rationale, as discussed
> ---
>   drivers/gpu/drm/drm_encoder.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 3261f142baea..0d5dbed06db4 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -189,14 +189,26 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>   {
>   	struct drm_device *dev = encoder->dev;
>   	struct drm_bridge *bridge, *next;
> +	LIST_HEAD(tmplist);
>   
>   	/* Note that the encoder_list is considered to be static; should we
>   	 * remove the drm_encoder at runtime we would have to decrement all
>   	 * the indices on the drm_encoder after us in the encoder_list.
>   	 */
>   
> -	list_for_each_entry_safe(bridge, next, &encoder->bridge_chain,
> -				 chain_node)
> +	/*
> +	 * 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);
> +	list_cut_before(&tmplist, &encoder->bridge_chain, &encoder->bridge_chain);
> +	mutex_unlock(&encoder->bridge_chain_mutex);
> +
> +	list_for_each_entry_safe(bridge, next, &tmplist, chain_node)
>   		drm_bridge_detach(bridge);
>   
>   	drm_mode_object_unregister(dev, &encoder->base);
>