linux-kernel - Re: [PATCH v4 2/4] drm/panel: Add refcount support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250429-benign-sidewinder-of-defense-6dd4d8@houat>
Date: Tue, 29 Apr 2025 11:00:53 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Jani Nikula <jani.nikula@...ux.intel.com>
Cc: Anusha Srivatsa <asrivats@...hat.com>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, Luca Ceresoli <luca.ceresoli@...tlin.com>
Subject: Re: [PATCH v4 2/4] drm/panel: Add refcount support

Hi Jani,

On Mon, Apr 28, 2025 at 07:31:50PM +0300, Jani Nikula wrote:
> On Mon, 31 Mar 2025, Anusha Srivatsa <asrivats@...hat.com> wrote:
> > Allocate panel via reference counting. Add _get() and _put() helper
> > functions to ensure panel allocations are refcounted. Avoid use after
> > free by ensuring panel pointer is valid and can be usable till the last
> > reference is put.
> >
> > Reviewed-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
> > Reviewed-by: Maxime Ripard <mripard@...nel.org>
> > Signed-off-by: Anusha Srivatsa <asrivats@...hat.com>
> >
> > ---
> > v4: Add refcounting documentation in this patch (Maxime)
> >
> > v3: Add include in this patch (Luca)
> >
> > v2: Export drm_panel_put/get() (Maxime)
> > - Change commit log with better workding (Luca, Maxime)
> > - Change drm_panel_put() to return void (Luca)
> > - Code Cleanups - add return in documentation, replace bridge to
> > panel (Luca)
> > ---
> >  drivers/gpu/drm/drm_panel.c | 64 ++++++++++++++++++++++++++++++++++++++++++++-
> >  include/drm/drm_panel.h     | 19 ++++++++++++++
> >  2 files changed, 82 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index bdeab5710ee324dc1742fbc77582250960556308..7b17531d85a4dc3031709919564d2e4d8332f748 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -355,24 +355,86 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
> >  }
> >  EXPORT_SYMBOL(of_drm_find_panel);
> >  
> > +static void __drm_panel_free(struct kref *kref)
> > +{
> > +	struct drm_panel *panel = container_of(kref, struct drm_panel, refcount);
> > +
> > +	kfree(panel->container);
> > +}
> > +
> > +/**
> > + * drm_panel_get - Acquire a panel reference
> > + * @panel: DRM panel
> > + *
> > + * This function increments the panel's refcount.
> > + * Returns:
> > + * Pointer to @panel
> > + */
> > +struct drm_panel *drm_panel_get(struct drm_panel *panel)
> > +{
> > +	if (!panel)
> > +		return panel;
> > +
> > +	kref_get(&panel->refcount);
> > +
> > +	return panel;
> > +}
> > +EXPORT_SYMBOL(drm_panel_get);
> > +
> > +/**
> > + * drm_panel_put - Release a panel reference
> > + * @panel: DRM panel
> > + *
> > + * This function decrements the panel's reference count and frees the
> > + * object if the reference count drops to zero.
> > + */
> > +void drm_panel_put(struct drm_panel *panel)
> > +{
> > +	if (panel)
> > +		kref_put(&panel->refcount, __drm_panel_free);
> > +}
> > +EXPORT_SYMBOL(drm_panel_put);
> > +
> > +/**
> > + * drm_panel_put_void - wrapper to drm_panel_put() taking a void pointer
> > + *
> > + * @data: pointer to @struct drm_panel, cast to a void pointer
> > + *
> > + * Wrapper of drm_panel_put() to be used when a function taking a void
> > + * pointer is needed, for example as a devm action.
> > + */
> > +static void drm_panel_put_void(void *data)
> > +{
> > +	struct drm_panel *panel = (struct drm_panel *)data;
> > +
> > +	drm_panel_put(panel);
> > +}
> > +
> >  void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
> >  			     const struct drm_panel_funcs *funcs,
> >  			     int connector_type)
> >  {
> >  	void *container;
> >  	struct drm_panel *panel;
> > +	int err;
> >  
> >  	if (!funcs) {
> >  		dev_warn(dev, "Missing funcs pointer\n");
> >  		return ERR_PTR(-EINVAL);
> >  	}
> >  
> > -	container = devm_kzalloc(dev, size, GFP_KERNEL);
> > +	container = kzalloc(size, GFP_KERNEL);
> >  	if (!container)
> >  		return ERR_PTR(-ENOMEM);
> >  
> >  	panel = container + offset;
> > +	panel->container = container;
> >  	panel->funcs = funcs;
> > +	kref_init(&panel->refcount);
> 
> Hi Anusha, this should be done in drm_panel_init() instead.
>
> There are many users of drm_panel that don't use devm_drm_panel_alloc()
> but allocate separately, and call drm_panel_init() only.

That wouldn't really work, because then drivers would have allocated the
panel with devm_kzalloc and thus the structure would be freed when the
device is removed, no matter the reference counting state.

> They'll all have refcount set to 0 instead of 1 like kref_init() does.
> 
> This means all subsequent get/put pairs on such panels will lead to
> __drm_panel_free() being called! But through a lucky coincidence, that
> will be a nop because panel->container is also not initialized...
> 
> I'm sorry to say, the drm refcounting interface is quite broken for such
> use cases.

The plan is to convert all panel drivers to that function, and Anusha
already sent series to do. It still needs a bit of work, but it should
land soon-ish.

For the transitional period though, it's not clear to me what you think
is broken at the moment, and / or what should be fixed.

Would you prefer an explicit check on container not being 0, with a
comment?

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)