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] [day] [month] [year] [list]

Message-ID: <20251003183904.15c800ac@booty>
Date: Fri, 3 Oct 2025 18:39:04 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>
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

Hello Maxime,

On Fri, 3 Oct 2025 16:04:50 +0200
Maxime Ripard <mripard@...nel.org> wrote:

> 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).

Just to give some background, I have realized we need to lock the
encoder chain after drm_for_each_bridge_in_chain_scoped() was added.
Should I had realized it before, I would have sent it in the form you
can see in this patch, without the get/put because it is not necessary.
Not sure whether that would have changed the reception.

But I'm not aware of any performance issue, and the impact of
refcounting should not be small, soI'll try re-adding an explicit
get/put on top of the current version. It will likely make the macro
more complicated but should be reasonably doable. So, expect a v3 with
that change to we can all see how it looks.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com