linux-kernel - Re: [PATCH v2 4/7] drm/bridge: lock the encoder chain in scoped for

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251003-dexterous-loose-guppy-45e1b3@houat>
Date: Fri, 3 Oct 2025 16:04:50 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Andrzej Hajda <andrzej.hajda@...el.com>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, 
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Hui Pu <Hui.Pu@...ealthcare.com>, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/7] drm/bridge: lock the encoder chain in scoped
 for_each loops

On Fri, Oct 03, 2025 at 12:39:26PM +0200, Luca Ceresoli wrote:
> drm_for_each_bridge_in_chain_scoped() and
> drm_for_each_bridge_in_chain_from() currently get/put the bridge at each
> iteration. But they don't protect the encoder chain, so it could change
> (bridges added/removed) while some code is iterating over the list
> itself. To make iterations safe, change the logic of these for_each macros
> to lock the encoder chain mutex at the beginning and unlock it at the end
> of the loop (be it at the end of the list, or earlier due to a 'break' or
> 'return' statement).
> 
> Also remove the get/put on the current bridge because it is not needed
> anymore. In fact all bridges in the encoder chain are refcounted already
> thanks to the drm_bridge_get() in drm_bridge_attach() and the
> drm_bridge_put() in drm_bridge_detach(). So while iterating with the mutex
> held the list cannot change _and_ the refcount of all bridges in the list
> cannot drop to zero.

This second paragraph *really* needs to be its own patch. And I'm not
really sure playing games when it comes to refcounting is a good idea.

A strict, simple, rule is way easier to follow than trying to figure out
two years from now why this loop skips the refcounting.

Unless you have a performance issue that is, in which case you should
add a comment (and we will need a meaningful benchmark to back that
claim).

> Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
> 
> ---
> 
> Changed in v2:
> - Fixed infinite loop in drm_for_each_bridge_in_chain_scoped() when
>   encoder->bridge_chain is empty, reported here:
>   https://lore.kernel.org/lkml/202509301358.38036b85-lkp@intel.com/
> - Slightly improved commit message
> ---
>  include/drm/drm_bridge.h | 62 ++++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 29 deletions(-)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 0ff7ab4aa8689a022458f935a7ffb23a2b715802..5a72817f04a78f5379f69e72fe519c5eb3ea043e 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1440,26 +1440,29 @@ drm_bridge_chain_get_last_bridge(struct drm_encoder *encoder)
>  						      struct drm_bridge, chain_node));
>  }
>  
> -/**
> - * drm_bridge_get_next_bridge_and_put - Get the next bridge in the chain
> - *                                      and put the previous
> - * @bridge: bridge object
> - *
> - * Same as drm_bridge_get_next_bridge() but additionally puts the @bridge.
> - *
> - * RETURNS:
> - * the next bridge in the chain after @bridge, or NULL if @bridge is the last.
> - */
> -static inline struct drm_bridge *
> -drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
> +static inline struct drm_bridge *drm_bridge_encoder_chain_lock(struct drm_bridge *bridge)
>  {
> -	struct drm_bridge *next = drm_bridge_get_next_bridge(bridge);
> +	drm_encoder_chain_lock(bridge->encoder);
> +
> +	return bridge;
> +}

You create a public function, this needs to be documented.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)