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@xxxxxxxxx>
> >> ---
> >> 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
