Re: [PATCH 06/26] drm/bridge: add devm_drm_of_find_bridge

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 9554 bytes --]

On Mon, Nov 24, 2025 at 05:25:39PM +0100, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Mon Nov 24, 2025 at 11:39 AM CET, Maxime Ripard wrote:
> > On Wed, Nov 19, 2025 at 02:05:37PM +0100, Luca Ceresoli wrote:
> >> Several drivers (about 20) follow the same pattern:
> >>
> >>  1. get a pointer to a bridge (typically the next bridge in the chain) by
> >>     calling of_drm_find_bridge()
> >>  2. store the returned pointer in the private driver data, keep it until
> >>     driver .remove
> >>  3. dereference the pointer at attach time and possibly at other times
> >>
> >> of_drm_find_bridge() is now deprecated because it does not increment the
> >> refcount and should be replaced with drm_of_find_bridge() +
> >> drm_bridge_put().
> >>
> >> However some of those drivers have a complex code flow and adding a
> >> drm_bridge_put() call in all the appropriate locations is error-prone,
> >> leads to ugly and more complex code, and can lead to errors over time with
> >> code flow changes.
> >>
> >> To handle all those drivers in a straightforward way, add a devm variant of
> >> drm_of_find_bridge() that adds a devm action to invoke drm_bridge_put()
> >> when the said driver is removed. This allows all those drivers to put the
> >> reference automatically and safely with a one line change:
> >>
> >>   - priv->next_bridge = of_drm_find_bridge(remote_np);
> >>   + priv->next_bridge = devm_drm_of_find_bridge(dev, remote_np);
> >>
> >> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> >>
> >> ---
> >>  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
> >>  include/drm/drm_bridge.h     |  5 +++++
> >>  2 files changed, 35 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 09ad825f9cb8..c7baafbe5695 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -1446,6 +1446,36 @@ struct drm_bridge *drm_of_find_bridge(struct device_node *np)
> >>  }
> >>  EXPORT_SYMBOL(drm_of_find_bridge);
> >>
> >> +/**
> >> + * devm_drm_of_find_bridge - find the bridge corresponding to the device
> >> + *			     node in the global bridge list and add a devm
> >> + *			     action to put it
> >> + *
> >> + * @dev: device requesting the bridge
> >> + * @np: device node
> >> + *
> >> + * On success the returned bridge refcount is incremented, and a devm
> >> + * action is added to call drm_bridge_put() when @dev is removed. So the
> >> + * caller does not have to put the returned bridge explicitly.
> >> + *
> >> + * RETURNS:
> >> + * drm_bridge control struct on success, NULL on failure
> >> + */
> >> +struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np)
> >> +{
> >> +	struct drm_bridge *bridge = drm_of_find_bridge(np);
> >> +
> >> +	if (bridge) {
> >> +		int err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
> >> +
> >> +		if (err)
> >> +			return ERR_PTR(err);
> >> +	}
> >> +
> >> +	return bridge;
> >> +}
> >> +EXPORT_SYMBOL(devm_drm_of_find_bridge);
> >
> > That's inherently unsafe though, because even if the bridge is removed
> > other parts of DRM might still have a reference to it and could call
> > into it.
> >
> > We'd then have dropped our reference to the next bridge, which could
> > have been freed, and it's a use-after-free.
> 
> I think you refer to this scenario:
> 
>   1. pipeline: encoder --> bridge A --> bridge B --> bridge C
>   2. encoder takes a reference to bridge B
>      using devm_drm_of_find_bridge() or other means
>   3. bridge B takes a next_bridge reference to bridge C
>      using devm_drm_of_find_bridge()
>   4. encoder calls (bridge B)->foo(), which in turns references
>      next_bridge, e.g.:
> 
>        b_foo() {
>            bar(b->next_bridge);
>        }
> 
> If bridges B and C are removed, bridge C can be freed but B is still
> allocated because the encoder holds a ref. So when step 4 happens, 'b->c'
> would be a use-after-free (or NULL deref if b.remove cleared it, which is
> just as bad).

Yep.

> If I got you correctly, then I'm a bit surprised by your comment. This
> series is part of the first chapter of the hotplug work, which does not aim
> at fixing everything but rather at fixing one part: handle dynamic
> _allocation_ lifetime of drm_bridges by adding a refcount and
> drm_bridge_get/put().
> 
> Chapter 2 of the work is adding drm_bridge_enter/exit/unplug() [1] and
> other changes in order to avoid code of drivers of removed bridges to
> access fields they shouldn't. So the above example at point 4 would become:
> 
>        b_foo() {
>            if (!drm_bridge_enter())
>                return;
>            bar(b->c);
>            drm_bridge_exit();
>        }
> 
> And that avoids 'b->c' after bridge B is removed.
> 
> Does that answer your remark?

Not really. I wasn't really questionning your current focus, or the way
you laid out the current agenda or whatever.

What I am questionning though is whether or not we want to introduce
something we will have to untangle soon, and even more so when we're not
mentioning it anywhere.

> > It's more complicated than it sounds, because we only have access to the
> > drm_device when the bridge is attached, so later than probe.
> >
> > I wonder if we shouldn't tie the lifetime of that reference to the
> > lifetime of the bridge itself, and we would give up the next_bridge
> > reference only when we're destroyed ourselves.
> 
> I'm afraid I'm not following you, sorry. Do you refer to the time between
> the bridge removal (driver .remove) and the last bridge put (when
> deallocation happens)?
> 
> In that time frame the struct drm_bridge is still allocated along with any
> next_bridge pointer it may contain, but the following bridge could have
> been deallocated.
> 
> What do you mean by "give up the next_bridge"?

What I was trying to say was that if we want to fix the problem you
illustrated about, we need to give up the reference at __drm_bridge_free
time. So each bridge having a reference to a bridge would need to do so
in its destroy hook.

Since it's quite a common pattern, it would make sense to add a
next_bridge field to drm_bridge itself, so the core can do it
automatically in __drm_bridge_free if that pointer is !NULL.

But...

> > Storing a list of all the references we need to drop is going to be
> > intrusive though, so maybe the easiest way to do it would be to create a
> > next_bridge field in drm_bridge, and only drop the reference stored
> > there?
> >
> > And possibly tie the whole thing together using a helper?
> >
> > Anyway, I'm not sure it should be a prerequisite to this series. I we do
> > want to go the devm_drm_of_find_bridge route however, we should at least
> > document that it's unsafe, and add a TODO entry to clean up the mess
> > later on.

... I *really* don't consider it something you need to work on right now.

> Do you mean the drm variant is unsafe while the original
> (drm_of_find_bridge() in this series, might be renamed) is not? I
> don't see how that can happen. If the driver for bridge B were to use
> drm_of_find_bridge(), that driver would be responsible to
> drm_bridge_put(b->next_bridge) in its .remove() function or earlier.
> So the next_bridge pointing to bridge C would equally become subject
> to use-after-free.

No, I was saying that both are equally unsafe. But we're adding a new,
broken, helper, and we don't mention anywhere that it is. So what I was
saying is mostly do we really want to introduce some more broken code
when we know it is. And if we do, we should be really clear about it.

> devm does not make it worse, on the opposite it postpones the
> drm_bridge_put(next_bridge) as late as possible: just after
> b.remove().

Which doesn't really change anything, does it? I'd expect the window
between the remove and final drm_bridge_put to be much wider than the
execution time of remove itself.

> One final, high-level thought about the various 'next_bridge' pointers that
> many bridge drivers have. Most of them do:
> 
>  0. have a 'struct drm_bridge next_bridge *' in their private struct
>  1. take the next_bridge reference during probe or another startup phase
>  2. store it in their private driver struct
>  3. use it to call drm_bridge_attach
>  4. (pending) put the reference to it in their .remove or earlier
> 
> I'm wondering whether we could let the DRM bridge core do it all, by
> removing items 0, 1, 2 and 4, and change 3 as:
> 
> -     drm_bridge_attach(encoder, me->next_bridge, &me->bridge, flags);
> +  drm_of_bridge_attach(encoder, &me->bridge, dev->of_node, 1, -1, flags);
> 
> where dev->of_node and the following integers are the same flags passed to
> devm_drm_of_get_bridge() and the like, i.e. the endpoint info needed to
> walk the DT graph and reach the next bridge.
> 
> This would allow the core to take care of all locking and lifetime of the
> next bridge, and most (all?) bridges would never access any pointers to the
> next bridge. The idea is to let the core do the right thing in a single
> place instead of trying to make all drivers do the right thing (and
> touching dozen files when needing to touch the logic).
> 
> That is more a long-term ideal than something I'd do right now, but having
> opinions would be very interesting.

That was pretty much my point, yeah.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]