Re: [PATCH] fbdev: Use device_create_with_groups() to fix sysfs groups registration race

[Helge Deller <deller@gmx.de>]

On 12/21/25 17:57, Hans de Goede wrote:
> The fbdev sysfs attributes are registered after sending the uevent for
> the device creation, leaving a race window where e.g. udev rules may
> not be able to access the sysfs attributes because the registration is
> not done yet.
> 
> Fix this by switching to device_create_with_groups(). This also results in
> a nice cleanup. After switching to device_create_with_groups() all that
> is left of fb_init_device() is setting the drvdata and that can be passed
> to device_create[_with_groups]() too. After which fb_init_device() can
> be completely removed.
> 
> Dropping fb_init_device() + fb_cleanup_device() in turn allows removing
> fb_info.class_flag as they were the only user of this field.
> 
> Fixes: 5fc830d6aca1 ("fbdev: Register sysfs groups through device_add_group")
> Cc: stable@vger.kernel.org
> Cc: Shixiong Ou <oushixiong@kylinos.cn>
> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

applied to fbdev git tree.

Thanks!
Helge


> ---
> Note the fixes tag is technically wrong. This race has existed forever.
> The commit I picked for the fixes tag is a dependency of this change not
> the commit introducing the race. I don't believe that backporting this
> back any further is useful which is why I went with this commit.
> ---
>   drivers/video/fbdev/core/fbsysfs.c | 36 +++---------------------------
>   include/linux/fb.h                 |  1 -
>   2 files changed, 3 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index b8344c40073b..baa2bae0fb5b 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c
> @@ -12,8 +12,6 @@
>   
>   #include "fb_internal.h"
>   
> -#define FB_SYSFS_FLAG_ATTR 1
> -
>   static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
>   {
>   	int err;
> @@ -451,33 +449,7 @@ static struct attribute *fb_device_attrs[] = {
>   	NULL,
>   };
>   
> -static const struct attribute_group fb_device_attr_group = {
> -	.attrs          = fb_device_attrs,
> -};
> -
> -static int fb_init_device(struct fb_info *fb_info)
> -{
> -	int ret;
> -
> -	dev_set_drvdata(fb_info->dev, fb_info);
> -
> -	fb_info->class_flag |= FB_SYSFS_FLAG_ATTR;
> -
> -	ret = device_add_group(fb_info->dev, &fb_device_attr_group);
> -	if (ret)
> -		fb_info->class_flag &= ~FB_SYSFS_FLAG_ATTR;
> -
> -	return 0;
> -}
> -
> -static void fb_cleanup_device(struct fb_info *fb_info)
> -{
> -	if (fb_info->class_flag & FB_SYSFS_FLAG_ATTR) {
> -		device_remove_group(fb_info->dev, &fb_device_attr_group);
> -
> -		fb_info->class_flag &= ~FB_SYSFS_FLAG_ATTR;
> -	}
> -}
> +ATTRIBUTE_GROUPS(fb_device);
>   
>   int fb_device_create(struct fb_info *fb_info)
>   {
> @@ -485,14 +457,13 @@ int fb_device_create(struct fb_info *fb_info)
>   	dev_t devt = MKDEV(FB_MAJOR, node);
>   	int ret;
>   
> -	fb_info->dev = device_create(fb_class, fb_info->device, devt, NULL, "fb%d", node);
> +	fb_info->dev = device_create_with_groups(fb_class, fb_info->device, devt, fb_info,
> +						 fb_device_groups, "fb%d", node);
>   	if (IS_ERR(fb_info->dev)) {
>   		/* Not fatal */
>   		ret = PTR_ERR(fb_info->dev);
>   		pr_warn("Unable to create device for framebuffer %d; error %d\n", node, ret);
>   		fb_info->dev = NULL;
> -	} else {
> -		fb_init_device(fb_info);
>   	}
>   
>   	return 0;
> @@ -505,7 +476,6 @@ void fb_device_destroy(struct fb_info *fb_info)
>   	if (!fb_info->dev)
>   		return;
>   
> -	fb_cleanup_device(fb_info);
>   	device_destroy(fb_class, devt);
>   	fb_info->dev = NULL;
>   }
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 05cc251035da..c3302d513546 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -497,7 +497,6 @@ struct fb_info {
>   #if defined(CONFIG_FB_DEVICE)
>   	struct device *dev;		/* This is this fb device */
>   #endif
> -	int class_flag;                    /* private sysfs flags */
>   #ifdef CONFIG_FB_TILEBLITTING
>   	struct fb_tile_ops *tileops;    /* Tile Blitting */
>   #endif