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@bootlin.com>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Anusha Srivatsa <asrivats@redhat.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);
+
+	err = devm_add_action_or_reset(dev, drm_panel_put_void, panel);
+	if (err)
+		return ERR_PTR(err);
 
 	drm_panel_init(panel, dev, funcs, connector_type);
 
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 415e85e8b76a15679f59c944ea152367dc3e0488..31d84f901c514c93ab6cbc09f445853ef897bc95 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -28,6 +28,7 @@
 #include <linux/errno.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/kref.h>
 
 struct backlight_device;
 struct dentry;
@@ -266,6 +267,17 @@ struct drm_panel {
 	 * If true then the panel has been enabled.
 	 */
 	bool enabled;
+
+	/**
+	 * @container: Pointer to the private driver struct embedding this
+	 * @struct drm_panel.
+	 */
+	void *container;
+
+	/**
+	 * @refcount: reference count of users referencing this panel.
+	 */
+	struct kref refcount;
 };
 
 void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
@@ -282,6 +294,10 @@ void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
  * @connector_type: the connector type (DRM_MODE_CONNECTOR_*) corresponding to
  * the panel interface
  *
+ * The reference count of the returned panel is initialized to 1. This
+ * reference will be automatically dropped via devm (by calling
+ * drm_panel_put()) when @dev is removed.
+ *
  * Returns:
  * Pointer to container structure embedding the panel, ERR_PTR on failure.
  */
@@ -294,6 +310,9 @@ void drm_panel_init(struct drm_panel *panel, struct device *dev,
 		    const struct drm_panel_funcs *funcs,
 		    int connector_type);
 
+struct drm_panel *drm_panel_get(struct drm_panel *panel);
+void drm_panel_put(struct drm_panel *panel);
+
 void drm_panel_add(struct drm_panel *panel);
 void drm_panel_remove(struct drm_panel *panel);
 

-- 
2.48.1

On Mon, 31 Mar 2025, Anusha Srivatsa <asrivats@redhat.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@bootlin.com>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Anusha Srivatsa <asrivats@redhat.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. 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.


BR,
Jani.


> +
> +	err = devm_add_action_or_reset(dev, drm_panel_put_void, panel);
> +	if (err)
> +		return ERR_PTR(err);
>  
>  	drm_panel_init(panel, dev, funcs, connector_type);
>  
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 415e85e8b76a15679f59c944ea152367dc3e0488..31d84f901c514c93ab6cbc09f445853ef897bc95 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -28,6 +28,7 @@
>  #include <linux/errno.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/kref.h>
>  
>  struct backlight_device;
>  struct dentry;
> @@ -266,6 +267,17 @@ struct drm_panel {
>  	 * If true then the panel has been enabled.
>  	 */
>  	bool enabled;
> +
> +	/**
> +	 * @container: Pointer to the private driver struct embedding this
> +	 * @struct drm_panel.
> +	 */
> +	void *container;
> +
> +	/**
> +	 * @refcount: reference count of users referencing this panel.
> +	 */
> +	struct kref refcount;
>  };
>  
>  void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
> @@ -282,6 +294,10 @@ void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
>   * @connector_type: the connector type (DRM_MODE_CONNECTOR_*) corresponding to
>   * the panel interface
>   *
> + * The reference count of the returned panel is initialized to 1. This
> + * reference will be automatically dropped via devm (by calling
> + * drm_panel_put()) when @dev is removed.
> + *
>   * Returns:
>   * Pointer to container structure embedding the panel, ERR_PTR on failure.
>   */
> @@ -294,6 +310,9 @@ void drm_panel_init(struct drm_panel *panel, struct device *dev,
>  		    const struct drm_panel_funcs *funcs,
>  		    int connector_type);
>  
> +struct drm_panel *drm_panel_get(struct drm_panel *panel);
> +void drm_panel_put(struct drm_panel *panel);
> +
>  void drm_panel_add(struct drm_panel *panel);
>  void drm_panel_remove(struct drm_panel *panel);

-- 
Jani Nikula, Intel

Hi Jani,

On Mon, Apr 28, 2025 at 07:31:50PM +0300, Jani Nikula wrote:
> On Mon, 31 Mar 2025, Anusha Srivatsa <asrivats@redhat.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@bootlin.com>
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Anusha Srivatsa <asrivats@redhat.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

On Tue, 29 Apr 2025, Maxime Ripard <mripard@kernel.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@redhat.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@bootlin.com>
>> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
>> > Signed-off-by: Anusha Srivatsa <asrivats@redhat.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

Hi Jani,

On Tue, Apr 29, 2025 at 12:22:00PM +0300, Jani Nikula wrote:
> On Tue, 29 Apr 2025, Maxime Ripard <mripard@kernel.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@redhat.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@bootlin.com>
> >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> >> > Signed-off-by: Anusha Srivatsa <asrivats@redhat.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...

I'm not entirely sure why you would need to allocate it from i915? The
drm_panel structure is only meant to be allocated by panel drivers, and
afaik no panel interface controller is allocating it.

> 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?

I mean, there is because the plan so far was to remove drm_panel_init() :)

Maxime