Hi Maxime, On Fri, 20 Jun 2025 13:41:48 +0200 Maxime Ripard <[email protected]> wrote:
> Hi Luca, > > On Fri, Jun 20, 2025 at 11:32:08AM +0200, Luca Ceresoli wrote: > > To the best of my knowledge, all drivers in the mainline kernel adding a > > DRM bridge are now converted to using devm_drm_bridge_alloc() for > > allocation and initialization. Among others this ensures initialization of > > the bridge refcount, allowing dynamic allocation lifetime. > > > > devm_drm_bridge_alloc() is now mandatory for all new bridges. Code using > > the old pattern ([devm_]kzalloc + filling the struct fields + > > drm_bridge_add) is not allowed anymore. > > > > Any drivers that might have been missed during the conversion, patches in > > flight towards mainline and out-of-tre drivers still using the old pattern > > will already be caught by a warning looking like: > > > > ------------[ cut here ]------------ > > refcount_t: addition on 0; use-after-free. > > WARNING: CPU: 2 PID: 83 at lib/refcount.c:25 > > refcount_warn_saturate+0x120/0x148 > > [...] > > Call trace: > > refcount_warn_saturate+0x120/0x148 (P) > > drm_bridge_get.part.0+0x70/0x98 [drm] > > drm_bridge_add+0x34/0x108 [drm] > > sn65dsi83_probe+0x200/0x480 [ti_sn65dsi83] > > [...] > > > > This warning comes from the refcount code and happens because > > drm_bridge_add() is increasing the refcount, which is uninitialized and > > thus initially zero. > > > > Having a warning and the corresponding stack trace is surely useful, but > > the warning text does not clarify the root problem nor how to fix it. > > > > Add a DRM_WARN() just before increasing the refcount, so the log will be > > much more readable: > > > > [drm] DRM bridge corrupted or not allocated by devm_drm_bridge_alloc() > > ------------[ cut here ]------------ > > refcount_t: addition on 0; use-after-free. > > [...etc...] > > > > A DRM_WARN is used because drm_warn and drm_WARN require a struct > > drm_device pointer which is not yet available when adding a bridge. > > > > Do not print the dev_name() in the warning because struct drm_bridge has no > > pointer to the struct device. The affected driver should be easy to catch > > based on the following stack trace however. > > > > Signed-off-by: Luca Ceresoli <[email protected]> > > > > --- > > > > This patch was added in v8 > > --- > > drivers/gpu/drm/drm_bridge.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index > > f001bbe95559aabf0aac9f25f89250ad4e1ad9c8..7d511c28608f1d8ea8fbb81d00efa9e227b02a13 > > 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -295,6 +295,9 @@ EXPORT_SYMBOL(__devm_drm_bridge_alloc); > > */ > > void drm_bridge_add(struct drm_bridge *bridge) > > { > > + if (kref_read(&bridge->refcount) == 0) > > + DRM_WARN("DRM bridge corrupted or not allocated by > > devm_drm_bridge_alloc()\n"); > > + > > I'm fine with it on principle, but I wonder if using bridge->container > is set wouldn't be a more obvious way to check it? Good point. Indeed the refcount check is potentially fragile because by its own nature the refcount gets changed at various moments by various parts of the kernel, unlike the container which is supposed to be never modified after allocation. I'll change this for v9. Ouch, I just realized this does not show as v8 as it should (see cover letter). Sorry about that, I messed up with my b4 branches, fixing both things in, well, v9. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
