On Thu, Jan 02, 2025 at 01:01:49PM +0100, Luca Ceresoli wrote:
> Hi Dmitry,
>
> On Tue, 31 Dec 2024 16:57:38 +0200
> Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> > On Tue, Dec 31, 2024 at 11:40:02AM +0100, Luca Ceresoli wrote:
> > > Instead of using dsi->out_bridge during the bridge search process, use a
> > > temporary variable and assign dsi->out_bridge only on successful
> > > completion.
> > >
> > > The main goal is to be able to drm_bridge_get() the out_bridge before
> > > setting it in dsi->out_bridge, which is done in a later commit. Setting
> > > dsi->out_bridge as in current code would leave a use-after-free window in
> > > case the bridge is deallocated by some other thread between
> > > 'dsi->out_bridge = devm_drm_panel_bridge_add()' and drm_bridge_get().
> >
> > I don't think that's how refcounting should work. Any of the functions
> > that give you the bridge should also increase refcount, requiring manual
> > _put() call afterwards. We might need a separate API for that.
>
> You're perfectly right.
>
> > > This change additionally avoids leaving an ERR_PTR value in dsi->out_bridge
> > > on failure. This is not necessarily a problem but it is not clean.
> > >
> > > Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
> > >
> > > ---
> > >
> > > This patch was added in v5.
> > > ---
> > > drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++------
> > > 1 file changed, 9 insertions(+), 6 deletions(-)
> > >
[...]
> > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > >
> > > panel = of_drm_find_panel(remote);
> > > if (!IS_ERR(panel)) {
> > > - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > + out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > } else {
> > > - dsi->out_bridge = of_drm_find_bridge(remote);
> > > - if (!dsi->out_bridge)
> > > - dsi->out_bridge = ERR_PTR(-EINVAL);
> > > + out_bridge = of_drm_find_bridge(remote);
> > > + if (!out_bridge)
> > > + out_bridge = ERR_PTR(-EINVAL);
> > > }
> >
> > While looking at this patch, I think we should migrate the driver to
> > drm_of_find_panel_or_bridge().
>
> Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm
> going to convert it.
>
> > Then your patch might add a function
> > close to devm_drm_of_get_bridge() or drmm_of_get_bridge().
>
> ...which would return a bridge pointer, with refcount already
> incremented. Sure, except I think it should _not_ be a drmm, as
> the bridge might itself disappear while the card keeps existing.
Feel free to add new one.
>
> Luca
>
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
--
With best wishes
Dmitry
