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....@nxp.com> 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
signature.asc
Description: PGP signature
