| Message ID | 20250312-drm-panel-v1-1-e99cd69f6136@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/panel: Refcounted panel allocation | expand |
Hello Anusha, On Wed, 12 Mar 2025 20:54:42 -0400 Anusha Srivatsa <asrivats@redhat.com> wrote: > Introduce reference counted allocations for panels to avoid > use-after-free. The patch adds the macro devm_drm_bridge_alloc() > to allocate a new refcounted panel. Followed the documentation for > drmm_encoder_alloc() and devm_drm_dev_alloc and other similar > implementations for this purpose. > > Also adding drm_panel_get() and drm_panel_put() to suitably > increment and decrement the refcount > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> I'm very happy to see the very first step of the panel rework mentioned by Maxime see the light! :-) This patch looks mostly good to me, and the similarity with my bridge refcounting work is by itself reassuring. I have a few notes, one is relevant and the others are minor details, see below. In the Subject line: s/allocatons/allocations/ [...] > +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset, > + const struct drm_panel_funcs *funcs) > +{ > + void *container; > + struct drm_panel *panel; > + int err; > + > + if (!funcs) { > + dev_warn(dev, "Missing funcs pointer\n"); > + return ERR_PTR(-EINVAL); > + } > + > + container = kzalloc(size, GFP_KERNEL); > + if (!container) > + return ERR_PTR(-ENOMEM); > + > + panel = container + offset; > + panel->container_offset = offset; > + 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, panel->connector_type); panel->connector_type here is uninitialized. You are passing panel->connector_type to drm_panel_init(), which will then copy it into panel->connector_type itself. > + > + /** > + * @container_offset: Offset of this struct within the container > + * struct embedding it. Used for refcounted panels to free the > + * embeddeing struct when the refcount drops to zero. > + */ > + size_t container_offset; While storing the offset obviously works, and that's what I had implemented in my latest bridge refcounting series, after some discussion with Maxime we agreed storing a container pointer instead of the offset is cleaner. I think it would be good here as well. See: https://lore.kernel.org/lkml/20250227-macho-convivial-tody-cea7dc@houat/ > +/** > + * drm_panel_get - Acquire a panel reference > + * @panel: DRM panel > + * > + * This function increments the panel's refcount. > + * > + */ > +static inline void drm_panel_get(struct drm_panel *panel) > +{ > + Remove empty line. > +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset, > + const struct drm_panel_funcs *funcs); > + > +/** > + * devm_drm_panel_alloc - Allocate and initialize an refcounted panel s/an/a/ -- same typo as in my bridge series so I'm fixing it in my series as well :) > + * @dev: struct device of the panel device > + * @type: the type of the struct which contains struct &drm_panel > + * @member: the name of the &drm_panel within @type > + * @funcs: callbacks for this panel > + * > + * The returned refcount is initialised to 1 In my opinion it is important to clarify that the caller does not have to explicitly call drm_panel_put() on the returned pointer, because devm will do it. Without clarifying, a user might think they need to, and that would result in an extra put, which would be a bug. Adapting from [0], that would be: * The returned refcount is initialized to 1. This reference will be * automatically dropped via devm (by calling drm_panel_put()) when @dev * is removed. [0] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-14-9d6f2c9c3058@bootlin.com/ Luca
On Thu, Mar 13, 2025 at 11:09:44AM +0100, Luca Ceresoli wrote: > Hello Anusha, > > On Wed, 12 Mar 2025 20:54:42 -0400 > Anusha Srivatsa <asrivats@redhat.com> wrote: > > > Introduce reference counted allocations for panels to avoid > > use-after-free. The patch adds the macro devm_drm_bridge_alloc() > > to allocate a new refcounted panel. Followed the documentation for > > drmm_encoder_alloc() and devm_drm_dev_alloc and other similar > > implementations for this purpose. > > > > Also adding drm_panel_get() and drm_panel_put() to suitably > > increment and decrement the refcount > > > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > > I'm very happy to see the very first step of the panel rework mentioned > by Maxime see the light! :-) > > This patch looks mostly good to me, and the similarity with my bridge > refcounting work is by itself reassuring. > > I have a few notes, one is relevant and the others are minor details, > see below. > > In the Subject line: s/allocatons/allocations/ > > [...] > > > +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset, > > + const struct drm_panel_funcs *funcs) > > +{ > > + void *container; > > + struct drm_panel *panel; > > + int err; > > + > > + if (!funcs) { > > + dev_warn(dev, "Missing funcs pointer\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + container = kzalloc(size, GFP_KERNEL); > > + if (!container) > > + return ERR_PTR(-ENOMEM); > > + > > + panel = container + offset; > > + panel->container_offset = offset; > > + 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, panel->connector_type); > > panel->connector_type here is uninitialized. You are passing > panel->connector_type to drm_panel_init(), which will then copy it into > panel->connector_type itself. Yeah, we need to have the connector type as a parameter. Maxime
Hi Anusha, In addition to the feedback Luca already provided, I have a few comments On Wed, Mar 12, 2025 at 08:54:42PM -0400, Anusha Srivatsa wrote: > Introduce reference counted allocations for panels to avoid > use-after-free. The patch adds the macro devm_drm_bridge_alloc() > to allocate a new refcounted panel. Followed the documentation for > drmm_encoder_alloc() and devm_drm_dev_alloc and other similar > implementations for this purpose. > > Also adding drm_panel_get() and drm_panel_put() to suitably > increment and decrement the refcount > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > --- > drivers/gpu/drm/drm_panel.c | 50 ++++++++++++++++++++++++++++++++++++++ > include/drm/drm_panel.h | 58 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 108 insertions(+) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index c627e42a7ce70459f50eb5095fffc806ca45dabf..b55e380e4a2f7ffd940c207e841c197d85113907 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -79,6 +79,7 @@ EXPORT_SYMBOL(drm_panel_init); > */ > void drm_panel_add(struct drm_panel *panel) > { > + drm_panel_get(panel); > mutex_lock(&panel_lock); > list_add_tail(&panel->list, &panel_list); > mutex_unlock(&panel_lock); > @@ -96,6 +97,7 @@ void drm_panel_remove(struct drm_panel *panel) > mutex_lock(&panel_lock); > list_del_init(&panel->list); > mutex_unlock(&panel_lock); > + drm_panel_put(panel); > } > EXPORT_SYMBOL(drm_panel_remove); I think these two should be added as a separate patch, with some additional comment on why it's needed (because we store a pointer in the panel list). > > @@ -355,6 +357,54 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np) > } > EXPORT_SYMBOL(of_drm_find_panel); > > +/* Internal function (for refcounted panels) */ > +void __drm_panel_free(struct kref *kref) > +{ > + struct drm_panel *panel = container_of(kref, struct drm_panel, refcount); > + void *container = ((void *)panel) - panel->container_offset; > + > + kfree(container); > +} > +EXPORT_SYMBOL(__drm_panel_free); > + > +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) > +{ > + void *container; > + struct drm_panel *panel; > + int err; > + > + if (!funcs) { > + dev_warn(dev, "Missing funcs pointer\n"); > + return ERR_PTR(-EINVAL); > + } > + > + container = kzalloc(size, GFP_KERNEL); > + if (!container) > + return ERR_PTR(-ENOMEM); > + > + panel = container + offset; > + panel->container_offset = offset; > + 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, panel->connector_type); > + > + return container; > +} > +EXPORT_SYMBOL(__devm_drm_panel_alloc); Similarly, here, I think we'd need to split that some more. Ideally, we should have a series of patches doing 1: Adding that allocation function you have right now, but using devm_kzalloc 2: Adding the reference counting to drm_panel, with drm_panel_get / drm_panel_put and the devm_action to put the reference in __devm_drm_panel_alloc() 3: Adding X patches to add calls to drm_bridge_get/drm_bridge_put everywhere it's needed, starting indeed by drm_panel_add/drm_panel_put. We don't have to do all of them in that series though. of_drm_find_panel though will probably merit a series of its own, given we'd have to fix all its callers too. 4: Convert some panels to the new allocation function. You already did that with panel_simple so there's nothing to change yet, but once we agree on the API we should mass convert all the panels. Maxime
On Thu, Mar 13, 2025 at 6:10 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote: > Hello Anusha, > > On Wed, 12 Mar 2025 20:54:42 -0400 > Anusha Srivatsa <asrivats@redhat.com> wrote: > > > Introduce reference counted allocations for panels to avoid > > use-after-free. The patch adds the macro devm_drm_bridge_alloc() > > to allocate a new refcounted panel. Followed the documentation for > > drmm_encoder_alloc() and devm_drm_dev_alloc and other similar > > implementations for this purpose. > > > > Also adding drm_panel_get() and drm_panel_put() to suitably > > increment and decrement the refcount > > > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > > I'm very happy to see the very first step of the panel rework mentioned > by Maxime see the light! :-) > > This patch looks mostly good to me, and the similarity with my bridge > refcounting work is by itself reassuring. > > I have a few notes, one is relevant and the others are minor details, > see below. > > In the Subject line: s/allocatons/allocations/ > good catch. > > [...] > > > +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t > offset, > > + const struct drm_panel_funcs *funcs) > > +{ > > + void *container; > > + struct drm_panel *panel; > > + int err; > > + > > + if (!funcs) { > > + dev_warn(dev, "Missing funcs pointer\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + container = kzalloc(size, GFP_KERNEL); > > + if (!container) > > + return ERR_PTR(-ENOMEM); > > + > > + panel = container + offset; > > + panel->container_offset = offset; > > + 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, panel->connector_type); > > panel->connector_type here is uninitialized. You are passing > panel->connector_type to drm_panel_init(), which will then copy it into > panel->connector_type itself. > > So you mean I pass connector_type from the driver calling the helper, so there is access to the connector type here? > > + > > + /** > > + * @container_offset: Offset of this struct within the container > > + * struct embedding it. Used for refcounted panels to free the > > + * embeddeing struct when the refcount drops to zero. > > + */ > > + size_t container_offset; > > While storing the offset obviously works, and that's what I had > implemented in my latest bridge refcounting series, after some > discussion with Maxime we agreed storing a container pointer instead of > the offset is cleaner. I think it would be good here as well. > > See: > https://lore.kernel.org/lkml/20250227-macho-convivial-tody-cea7dc@houat/ > so just void *container instead of size_t container_offset. > > +/** > > + * drm_panel_get - Acquire a panel reference > > + * @panel: DRM panel > > + * > > + * This function increments the panel's refcount. > > + * > > + */ > > +static inline void drm_panel_get(struct drm_panel *panel) > > +{ > > + > > Remove empty line. > will do. > > > +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t > offset, > > + const struct drm_panel_funcs *funcs); > > + > > +/** > > + * devm_drm_panel_alloc - Allocate and initialize an refcounted panel > > s/an/a/ -- same typo as in my bridge series so I'm fixing it in my > series as well :) > > > + * @dev: struct device of the panel device > > + * @type: the type of the struct which contains struct &drm_panel > > + * @member: the name of the &drm_panel within @type > > + * @funcs: callbacks for this panel > > + * > > + * The returned refcount is initialised to 1 > > In my opinion it is important to clarify that the caller does not have > to explicitly call drm_panel_put() on the returned pointer, because > devm will do it. Without clarifying, a user might think they need to, > and that would result in an extra put, which would be a bug. > > Adapting from [0], that would be: > > * The returned refcount is initialized to 1. This reference will be > * automatically dropped via devm (by calling drm_panel_put()) when @dev > * is removed. > > [0] > https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-14-9d6f2c9c3058@bootlin.com/ > WIll make this change. Thanks for the feedback! Anusha > Luca > > -- > Luca Ceresoli, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > >
On Thu, Mar 13, 2025 at 10:42 AM Maxime Ripard <mripard@kernel.org> wrote: > Hi Anusha, > > In addition to the feedback Luca already provided, I have a few comments > > On Wed, Mar 12, 2025 at 08:54:42PM -0400, Anusha Srivatsa wrote: > > Introduce reference counted allocations for panels to avoid > > use-after-free. The patch adds the macro devm_drm_bridge_alloc() > > to allocate a new refcounted panel. Followed the documentation for > > drmm_encoder_alloc() and devm_drm_dev_alloc and other similar > > implementations for this purpose. > > > > Also adding drm_panel_get() and drm_panel_put() to suitably > > increment and decrement the refcount > > > > Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> > > --- > > drivers/gpu/drm/drm_panel.c | 50 ++++++++++++++++++++++++++++++++++++++ > > include/drm/drm_panel.h | 58 > +++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 108 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > index > c627e42a7ce70459f50eb5095fffc806ca45dabf..b55e380e4a2f7ffd940c207e841c197d85113907 > 100644 > > --- a/drivers/gpu/drm/drm_panel.c > > +++ b/drivers/gpu/drm/drm_panel.c > > @@ -79,6 +79,7 @@ EXPORT_SYMBOL(drm_panel_init); > > */ > > void drm_panel_add(struct drm_panel *panel) > > { > > + drm_panel_get(panel); > > mutex_lock(&panel_lock); > > list_add_tail(&panel->list, &panel_list); > > mutex_unlock(&panel_lock); > > @@ -96,6 +97,7 @@ void drm_panel_remove(struct drm_panel *panel) > > mutex_lock(&panel_lock); > > list_del_init(&panel->list); > > mutex_unlock(&panel_lock); > > + drm_panel_put(panel); > > } > > EXPORT_SYMBOL(drm_panel_remove); > > I think these two should be added as a separate patch, with some > additional comment on why it's needed (because we store a pointer in the > panel list). > Sounds good. > > > > @@ -355,6 +357,54 @@ struct drm_panel *of_drm_find_panel(const struct > device_node *np) > > } > > EXPORT_SYMBOL(of_drm_find_panel); > > > > +/* Internal function (for refcounted panels) */ > > +void __drm_panel_free(struct kref *kref) > > +{ > > + struct drm_panel *panel = container_of(kref, struct drm_panel, > refcount); > > + void *container = ((void *)panel) - panel->container_offset; > > + > > + kfree(container); > > +} > > +EXPORT_SYMBOL(__drm_panel_free); > > + > > +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) > > +{ > > + void *container; > > + struct drm_panel *panel; > > + int err; > > + > > + if (!funcs) { > > + dev_warn(dev, "Missing funcs pointer\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + container = kzalloc(size, GFP_KERNEL); > > + if (!container) > > + return ERR_PTR(-ENOMEM); > > + > > + panel = container + offset; > > + panel->container_offset = offset; > > + 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, panel->connector_type); > > + > > + return container; > > +} > > +EXPORT_SYMBOL(__devm_drm_panel_alloc); > > Similarly, here, I think we'd need to split that some more. Ideally, we > should have a series of patches doing > > 1: Adding that allocation function you have right now, but using > devm_kzalloc > > 2: Adding the reference counting to drm_panel, with drm_panel_get / > drm_panel_put and the devm_action to put the reference in > __devm_drm_panel_alloc() > > 3: Adding X patches to add calls to drm_bridge_get/drm_bridge_put > everywhere it's needed, starting indeed by > drm_panel_add/drm_panel_put. We don't have to do all of them in that > series though. of_drm_find_panel though will probably merit a series > of its own, given we'd have to fix all its callers too. > > 4: Convert some panels to the new allocation function. You already did > that with panel_simple so there's nothing to change yet, but once we > agree on the API we should mass convert all the panels. > > I want to get the API right before making mass conversion of drivers. Will split this patch as you have suggested above. Will leave out fixing of of_drm_find_panel() callers to a separate series as well. Thanks! Anusha > Maxime >
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index c627e42a7ce70459f50eb5095fffc806ca45dabf..b55e380e4a2f7ffd940c207e841c197d85113907 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -79,6 +79,7 @@ EXPORT_SYMBOL(drm_panel_init); */ void drm_panel_add(struct drm_panel *panel) { + drm_panel_get(panel); mutex_lock(&panel_lock); list_add_tail(&panel->list, &panel_list); mutex_unlock(&panel_lock); @@ -96,6 +97,7 @@ void drm_panel_remove(struct drm_panel *panel) mutex_lock(&panel_lock); list_del_init(&panel->list); mutex_unlock(&panel_lock); + drm_panel_put(panel); } EXPORT_SYMBOL(drm_panel_remove); @@ -355,6 +357,54 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np) } EXPORT_SYMBOL(of_drm_find_panel); +/* Internal function (for refcounted panels) */ +void __drm_panel_free(struct kref *kref) +{ + struct drm_panel *panel = container_of(kref, struct drm_panel, refcount); + void *container = ((void *)panel) - panel->container_offset; + + kfree(container); +} +EXPORT_SYMBOL(__drm_panel_free); + +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) +{ + void *container; + struct drm_panel *panel; + int err; + + if (!funcs) { + dev_warn(dev, "Missing funcs pointer\n"); + return ERR_PTR(-EINVAL); + } + + container = kzalloc(size, GFP_KERNEL); + if (!container) + return ERR_PTR(-ENOMEM); + + panel = container + offset; + panel->container_offset = offset; + 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, panel->connector_type); + + return container; +} +EXPORT_SYMBOL(__devm_drm_panel_alloc); + /** * of_drm_get_panel_orientation - look up the orientation of the panel through * the "rotation" binding from a device tree node diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index a9c042c8dea1a82ef979c7a68204e0b55483fc28..f7cfda0039c066ea2c2b26da5062015e61880971 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,8 +267,65 @@ struct drm_panel { * If true then the panel has been enabled. */ bool enabled; + + /** + * @container_offset: Offset of this struct within the container + * struct embedding it. Used for refcounted panels to free the + * embeddeing struct when the refcount drops to zero. + */ + size_t container_offset; + /** + * @refcount: reference count for panels with dynamic lifetime + */ + struct kref refcount; }; +void __drm_panel_free(struct kref *kref); + +/** + * drm_panel_get - Acquire a panel reference + * @panel: DRM panel + * + * This function increments the panel's refcount. + * + */ +static inline void drm_panel_get(struct drm_panel *panel) +{ + + kref_get(&panel->refcount); +} + +/** + * 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. + */ +static inline void drm_panel_put(struct drm_panel *panel) +{ + kref_put(&panel->refcount, __drm_panel_free); +} + +void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset, + const struct drm_panel_funcs *funcs); + +/** + * devm_drm_panel_alloc - Allocate and initialize an refcounted panel + * @dev: struct device of the panel device + * @type: the type of the struct which contains struct &drm_panel + * @member: the name of the &drm_panel within @type + * @funcs: callbacks for this panel + * + * The returned refcount is initialised to 1 + * + * Returns: + * Pointer to new panel, or ERR_PTR on failure. + */ +#define devm_drm_panel_alloc(dev, type, member, funcs) \ + ((type *)__devm_drm_panel_alloc(dev, sizeof(type), \ + offsetof(type, member), funcs)) + void drm_panel_init(struct drm_panel *panel, struct device *dev, const struct drm_panel_funcs *funcs, int connector_type);
Introduce reference counted allocations for panels to avoid use-after-free. The patch adds the macro devm_drm_bridge_alloc() to allocate a new refcounted panel. Followed the documentation for drmm_encoder_alloc() and devm_drm_dev_alloc and other similar implementations for this purpose. Also adding drm_panel_get() and drm_panel_put() to suitably increment and decrement the refcount Signed-off-by: Anusha Srivatsa <asrivats@redhat.com> --- drivers/gpu/drm/drm_panel.c | 50 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_panel.h | 58 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+)