| Message ID | 9e5567f6-c27e-4875-9db8-0435669a7d7c@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Series with core improvements/refactorings | expand |
On Sun, Sep 08, 2024 at 02:11:25PM GMT, Heiner Kallweit wrote: > Define class drm statically and constify it. This ensure that no user > of the exported struct class can tamper with it. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/gpu/drm/drm_internal.h | 2 +- > drivers/gpu/drm/drm_privacy_screen.c | 2 +- > drivers/gpu/drm/drm_sysfs.c | 32 ++++++++++++++-------------- > 3 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index 1705bfc90..6e0df44b6 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -139,7 +139,7 @@ bool drm_master_internal_acquire(struct drm_device *dev); > void drm_master_internal_release(struct drm_device *dev); > > /* drm_sysfs.c */ > -extern struct class *drm_class; > +extern const struct class drm_class; > > int drm_sysfs_init(void); > void drm_sysfs_destroy(void); > diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c > index 6cc39e307..2fbd24ba5 100644 > --- a/drivers/gpu/drm/drm_privacy_screen.c > +++ b/drivers/gpu/drm/drm_privacy_screen.c > @@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register( > mutex_init(&priv->lock); > BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head); > > - priv->dev.class = drm_class; > + priv->dev.class = &drm_class; > priv->dev.type = &drm_privacy_screen_type; > priv->dev.parent = parent; > priv->dev.release = drm_privacy_screen_device_release; > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index f8577043e..f443f9a76 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -58,7 +58,15 @@ static struct device_type drm_sysfs_device_connector = { > .name = "drm_connector", > }; > > -struct class *drm_class; > +static char *drm_devnode(const struct device *dev, umode_t *mode) > +{ > + return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev)); > +} > + > +const struct class drm_class = { > + .name = "drm", > + .devnode = drm_devnode, > +}; > > #ifdef CONFIG_ACPI > static bool drm_connector_acpi_bus_match(struct device *dev) > @@ -93,11 +101,6 @@ static void drm_sysfs_acpi_register(void) { } > static void drm_sysfs_acpi_unregister(void) { } > #endif > > -static char *drm_devnode(const struct device *dev, umode_t *mode) > -{ > - return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev)); > -} > - Please keep this function in place and move drm_class declarattion next to it. It simplifies reviewing the code. > static int typec_connector_bind(struct device *dev, > struct device *typec_connector, void *data) > { > @@ -138,14 +141,12 @@ static const struct component_ops typec_connector_ops = { > */ > int drm_sysfs_init(void) > { > - drm_class = class_create("drm"); > - if (IS_ERR(drm_class)) > - return PTR_ERR(drm_class); > + int ret = class_register(&drm_class); > > - drm_class->devnode = drm_devnode; > + if (!ret) > + drm_sysfs_acpi_register(); > > - drm_sysfs_acpi_register(); > - return 0; > + return ret; > } > > /** > @@ -156,8 +157,7 @@ int drm_sysfs_init(void) > void drm_sysfs_destroy(void) > { > drm_sysfs_acpi_unregister(); > - class_destroy(drm_class); > - drm_class = NULL; > + class_unregister(&drm_class); This code makes me wonder: can we define static classes in unloadable modules? What happens if userspace holds the reference on the class in sysfs, while we remove the module ? > } > > static void drm_sysfs_release(struct device *dev) > @@ -337,7 +337,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector) > return -ENOMEM; > > device_initialize(kdev); > - kdev->class = drm_class; > + kdev->class = &drm_class; > kdev->type = &drm_sysfs_device_connector; > kdev->parent = dev->primary->kdev; > kdev->groups = connector_dev_groups; > @@ -516,7 +516,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor) > minor_str = "card%d"; > > kdev->devt = MKDEV(DRM_MAJOR, minor->index); > - kdev->class = drm_class; > + kdev->class = &drm_class; > kdev->type = &drm_sysfs_device_minor; > } > > -- > 2.46.0 > >
On 22.09.2024 17:11, Dmitry Baryshkov wrote: > On Sun, Sep 08, 2024 at 02:11:25PM GMT, Heiner Kallweit wrote: >> Define class drm statically and constify it. This ensure that no user >> of the exported struct class can tamper with it. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/gpu/drm/drm_internal.h | 2 +- >> drivers/gpu/drm/drm_privacy_screen.c | 2 +- >> drivers/gpu/drm/drm_sysfs.c | 32 ++++++++++++++-------------- >> 3 files changed, 18 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h >> index 1705bfc90..6e0df44b6 100644 >> --- a/drivers/gpu/drm/drm_internal.h >> +++ b/drivers/gpu/drm/drm_internal.h >> @@ -139,7 +139,7 @@ bool drm_master_internal_acquire(struct drm_device *dev); >> void drm_master_internal_release(struct drm_device *dev); >> >> /* drm_sysfs.c */ >> -extern struct class *drm_class; >> +extern const struct class drm_class; >> >> int drm_sysfs_init(void); >> void drm_sysfs_destroy(void); >> diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c >> index 6cc39e307..2fbd24ba5 100644 >> --- a/drivers/gpu/drm/drm_privacy_screen.c >> +++ b/drivers/gpu/drm/drm_privacy_screen.c >> @@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register( >> mutex_init(&priv->lock); >> BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head); >> >> - priv->dev.class = drm_class; >> + priv->dev.class = &drm_class; >> priv->dev.type = &drm_privacy_screen_type; >> priv->dev.parent = parent; >> priv->dev.release = drm_privacy_screen_device_release; >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >> index f8577043e..f443f9a76 100644 >> --- a/drivers/gpu/drm/drm_sysfs.c >> +++ b/drivers/gpu/drm/drm_sysfs.c >> @@ -58,7 +58,15 @@ static struct device_type drm_sysfs_device_connector = { >> .name = "drm_connector", >> }; >> >> -struct class *drm_class; >> +static char *drm_devnode(const struct device *dev, umode_t *mode) >> +{ >> + return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev)); >> +} >> + >> +const struct class drm_class = { >> + .name = "drm", >> + .devnode = drm_devnode, >> +}; >> >> #ifdef CONFIG_ACPI >> static bool drm_connector_acpi_bus_match(struct device *dev) >> @@ -93,11 +101,6 @@ static void drm_sysfs_acpi_register(void) { } >> static void drm_sysfs_acpi_unregister(void) { } >> #endif >> >> -static char *drm_devnode(const struct device *dev, umode_t *mode) >> -{ >> - return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev)); >> -} >> - > > Please keep this function in place and move drm_class declarattion next > to it. It simplifies reviewing the code. > >> static int typec_connector_bind(struct device *dev, >> struct device *typec_connector, void *data) >> { >> @@ -138,14 +141,12 @@ static const struct component_ops typec_connector_ops = { >> */ >> int drm_sysfs_init(void) >> { >> - drm_class = class_create("drm"); >> - if (IS_ERR(drm_class)) >> - return PTR_ERR(drm_class); >> + int ret = class_register(&drm_class); >> >> - drm_class->devnode = drm_devnode; >> + if (!ret) >> + drm_sysfs_acpi_register(); >> >> - drm_sysfs_acpi_register(); >> - return 0; >> + return ret; >> } >> >> /** >> @@ -156,8 +157,7 @@ int drm_sysfs_init(void) >> void drm_sysfs_destroy(void) >> { >> drm_sysfs_acpi_unregister(); >> - class_destroy(drm_class); >> - drm_class = NULL; >> + class_unregister(&drm_class); > > This code makes me wonder: can we define static classes in unloadable > modules? What happens if userspace holds the reference on the class in > sysfs, while we remove the module ? > I'm not sure, just saw that this isn't an unusual scenario. Let's ask the drivers/base maintainers. +Greg/Rafael >> } >> >> static void drm_sysfs_release(struct device *dev) >> @@ -337,7 +337,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector) >> return -ENOMEM; >> >> device_initialize(kdev); >> - kdev->class = drm_class; >> + kdev->class = &drm_class; >> kdev->type = &drm_sysfs_device_connector; >> kdev->parent = dev->primary->kdev; >> kdev->groups = connector_dev_groups; >> @@ -516,7 +516,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor) >> minor_str = "card%d"; >> >> kdev->devt = MKDEV(DRM_MAJOR, minor->index); >> - kdev->class = drm_class; >> + kdev->class = &drm_class; >> kdev->type = &drm_sysfs_device_minor; >> } >> >> -- >> 2.46.0 >> >> >
On Sat, Nov 02, 2024 at 10:33:24PM +0100, Heiner Kallweit wrote: > On 22.09.2024 17:11, Dmitry Baryshkov wrote: > > On Sun, Sep 08, 2024 at 02:11:25PM GMT, Heiner Kallweit wrote: > >> Define class drm statically and constify it. This ensure that no user > >> of the exported struct class can tamper with it. > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> drivers/gpu/drm/drm_internal.h | 2 +- > >> drivers/gpu/drm/drm_privacy_screen.c | 2 +- > >> drivers/gpu/drm/drm_sysfs.c | 32 ++++++++++++++-------------- > >> 3 files changed, 18 insertions(+), 18 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > >> index 1705bfc90..6e0df44b6 100644 > >> --- a/drivers/gpu/drm/drm_internal.h > >> +++ b/drivers/gpu/drm/drm_internal.h > >> @@ -139,7 +139,7 @@ bool drm_master_internal_acquire(struct drm_device *dev); > >> void drm_master_internal_release(struct drm_device *dev); > >> > >> /* drm_sysfs.c */ > >> -extern struct class *drm_class; > >> +extern const struct class drm_class; > >> > >> int drm_sysfs_init(void); > >> void drm_sysfs_destroy(void); > >> diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c > >> index 6cc39e307..2fbd24ba5 100644 > >> --- a/drivers/gpu/drm/drm_privacy_screen.c > >> +++ b/drivers/gpu/drm/drm_privacy_screen.c > >> @@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register( > >> mutex_init(&priv->lock); > >> BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head); > >> > >> - priv->dev.class = drm_class; > >> + priv->dev.class = &drm_class; > >> priv->dev.type = &drm_privacy_screen_type; > >> priv->dev.parent = parent; > >> priv->dev.release = drm_privacy_screen_device_release; > >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > >> index f8577043e..f443f9a76 100644 > >> --- a/drivers/gpu/drm/drm_sysfs.c > >> +++ b/drivers/gpu/drm/drm_sysfs.c > >> @@ -58,7 +58,15 @@ static struct device_type drm_sysfs_device_connector = { > >> .name = "drm_connector", > >> }; > >> > >> -struct class *drm_class; > >> +static char *drm_devnode(const struct device *dev, umode_t *mode) > >> +{ > >> + return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev)); > >> +} > >> + > >> +const struct class drm_class = { > >> + .name = "drm", > >> + .devnode = drm_devnode, > >> +}; > >> > >> #ifdef CONFIG_ACPI > >> static bool drm_connector_acpi_bus_match(struct device *dev) > >> @@ -93,11 +101,6 @@ static void drm_sysfs_acpi_register(void) { } > >> static void drm_sysfs_acpi_unregister(void) { } > >> #endif > >> > >> -static char *drm_devnode(const struct device *dev, umode_t *mode) > >> -{ > >> - return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev)); > >> -} > >> - > > > > Please keep this function in place and move drm_class declarattion next > > to it. It simplifies reviewing the code. > > > >> static int typec_connector_bind(struct device *dev, > >> struct device *typec_connector, void *data) > >> { > >> @@ -138,14 +141,12 @@ static const struct component_ops typec_connector_ops = { > >> */ > >> int drm_sysfs_init(void) > >> { > >> - drm_class = class_create("drm"); > >> - if (IS_ERR(drm_class)) > >> - return PTR_ERR(drm_class); > >> + int ret = class_register(&drm_class); > >> > >> - drm_class->devnode = drm_devnode; > >> + if (!ret) > >> + drm_sysfs_acpi_register(); > >> > >> - drm_sysfs_acpi_register(); > >> - return 0; > >> + return ret; > >> } > >> > >> /** > >> @@ -156,8 +157,7 @@ int drm_sysfs_init(void) > >> void drm_sysfs_destroy(void) > >> { > >> drm_sysfs_acpi_unregister(); > >> - class_destroy(drm_class); > >> - drm_class = NULL; > >> + class_unregister(&drm_class); > > > > This code makes me wonder: can we define static classes in unloadable > > modules? What happens if userspace holds the reference on the class in > > sysfs, while we remove the module ? Bad things happen, don't do that :) Good news is it's really hard to hold onto a class structure from userspace, sysfs should not be doing this, so there shouldn't be really any other code paths that cause this to happen, unless you do something odd in your driver. thanks, greg k-h
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 1705bfc90..6e0df44b6 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -139,7 +139,7 @@ bool drm_master_internal_acquire(struct drm_device *dev); void drm_master_internal_release(struct drm_device *dev); /* drm_sysfs.c */ -extern struct class *drm_class; +extern const struct class drm_class; int drm_sysfs_init(void); void drm_sysfs_destroy(void); diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c index 6cc39e307..2fbd24ba5 100644 --- a/drivers/gpu/drm/drm_privacy_screen.c +++ b/drivers/gpu/drm/drm_privacy_screen.c @@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register( mutex_init(&priv->lock); BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head); - priv->dev.class = drm_class; + priv->dev.class = &drm_class; priv->dev.type = &drm_privacy_screen_type; priv->dev.parent = parent; priv->dev.release = drm_privacy_screen_device_release; diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index f8577043e..f443f9a76 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -58,7 +58,15 @@ static struct device_type drm_sysfs_device_connector = { .name = "drm_connector", }; -struct class *drm_class; +static char *drm_devnode(const struct device *dev, umode_t *mode) +{ + return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev)); +} + +const struct class drm_class = { + .name = "drm", + .devnode = drm_devnode, +}; #ifdef CONFIG_ACPI static bool drm_connector_acpi_bus_match(struct device *dev) @@ -93,11 +101,6 @@ static void drm_sysfs_acpi_register(void) { } static void drm_sysfs_acpi_unregister(void) { } #endif -static char *drm_devnode(const struct device *dev, umode_t *mode) -{ - return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev)); -} - static int typec_connector_bind(struct device *dev, struct device *typec_connector, void *data) { @@ -138,14 +141,12 @@ static const struct component_ops typec_connector_ops = { */ int drm_sysfs_init(void) { - drm_class = class_create("drm"); - if (IS_ERR(drm_class)) - return PTR_ERR(drm_class); + int ret = class_register(&drm_class); - drm_class->devnode = drm_devnode; + if (!ret) + drm_sysfs_acpi_register(); - drm_sysfs_acpi_register(); - return 0; + return ret; } /** @@ -156,8 +157,7 @@ int drm_sysfs_init(void) void drm_sysfs_destroy(void) { drm_sysfs_acpi_unregister(); - class_destroy(drm_class); - drm_class = NULL; + class_unregister(&drm_class); } static void drm_sysfs_release(struct device *dev) @@ -337,7 +337,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector) return -ENOMEM; device_initialize(kdev); - kdev->class = drm_class; + kdev->class = &drm_class; kdev->type = &drm_sysfs_device_connector; kdev->parent = dev->primary->kdev; kdev->groups = connector_dev_groups; @@ -516,7 +516,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor) minor_str = "card%d"; kdev->devt = MKDEV(DRM_MAJOR, minor->index); - kdev->class = drm_class; + kdev->class = &drm_class; kdev->type = &drm_sysfs_device_minor; }
Define class drm statically and constify it. This ensure that no user of the exported struct class can tamper with it. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/gpu/drm/drm_internal.h | 2 +- drivers/gpu/drm/drm_privacy_screen.c | 2 +- drivers/gpu/drm/drm_sysfs.c | 32 ++++++++++++++-------------- 3 files changed, 18 insertions(+), 18 deletions(-)