Re: [PATCH v8 2/5] drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()

[Maxime Ripard]

On Tue, Jun 24, 2025 at 10:44:03AM +0800, Liu Ying wrote:
> On 06/23/2025, Luca Ceresoli wrote:
> > On Mon, 23 Jun 2025 10:56:13 +0800
> > Liu Ying <victor.liu@xxxxxxx> wrote:
> > 
> >> On 06/21/2025, Luca Ceresoli wrote:
> >>> drm_bridge_chain_get_first_bridge() returns a bridge pointer that the
> >>> caller could hold for a long time. Increment the refcount of the returned
> >>> bridge and document it must be put by the caller.  
> >>
> >> To make sure the incremented refcount is decremented once this patch is
> >> applied, does it make sense to squash patch 3, 4 and 5 into this one?
> > 
> > I see there is a trade off here between bisectability and patch
> > readability.
> > 
> > However about bisectability the problem is limited for this series. To
> > get an actual get/put imbalance you'd have to be able to remove the
> > bridge, but removing (part of) the bridge chain is not at all supported
> > right now, and it won't be until after chapter 4 of this work (see
> > cover letter).
> > 
> > However I realize there is an issue if:
> > * patch 2 is applied but patches 3/4/5 are not
> >   (it does not make sense to apply this series partially, but this
> >   might happen when cherry-picking?)
> 
> Yes for cherry-picking and bisecting.
> 
> > * an entire DRM card is removed where
> >   drm_bridge_chain_get_first_bridge() is used by some components
> > 
> > If both happen we'd have a get without put, thus a missing free and a
> > memory leak for the container struct.
> 
> Yes, that's a memory leak.
> 
> > Note that, besides drm_bridge_chain_get_first_bridge() that this
> > series covers, there are various other accessors: see items 1.E.{2..8}
> 
> IIUC, without those items addressed, the issue we have is use-after-free,
> but not the memory leak this patch introduces(without squash).

Given that this structure is going to be allocated a couple of times in
the system life at best, and that the situation prior to the work Luca
has been doing was a use-after-free, I'm not really concerned about a
transient memory leak in a situation that cannot happen.

If people want to come and backport random patches without looking at
the whole thing, that's their problem.

Maxime

Attachment: signature.asc
Description: PGP signature