| 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
| ||
|
Message-ID: <87o6wfwcef.fsf@intel.com>
Date: Tue, 29 Apr 2025 12:22:00 +0300
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Maxime Ripard <mripard@...nel.org>
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
On Tue, 29 Apr 2025, Maxime Ripard <mripard@...nel.org> wrote:
> 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?
I'm looking at what it would take to add drm_panel support to i915 so
that you could have drm_panel_followers on it. There are gaps of course,
but initially it would mean allocating and freeing drm_panel ourselves,
not via devm_drm_panel_alloc() nor devm_kzalloc(), because none of the
other stuff is allocated that way. drm_panel would just sit as a
sub-struct inside struct intel_panel, which is a sub-struct of struct
intel_connector, which has its own allocation...
But basically in its current state, the refcounting would not be
reliable for that use case. I guess with panel->container being NULL
nothing happens, but the idea that the refcount drops back to 0 after a
get/put is a bit scary.
Anyway, I think there should be no harm in moving the kref init to
drm_panel_init(), right?
BR,
Jani.
--
Jani Nikula, Intel
Powered by blists - more mailing lists