Re: [PATCH 30/30] fbdev: Make support for userspace interfaces configurable

Hi

Am 11.06.23 um 18:37 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Mon, Jun 05, 2023 at 04:48:12PM +0200, Thomas Zimmermann wrote:
>> Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
>> device optional. If the new option has not been selected, fbdev
>> does not create a files in devfs or sysfs.
> s/ a//
>>
>> Most modern Linux systems run a DRM-based graphics stack that uses
>> the kernel's framebuffer console, but has otherwise deprecated fbdev
>> support. Yet fbdev userspace interfaces are still present.
>>
>> The option makes it possible to use the fbdev subsystem as console
>> implementation without support for userspace. This closes potential
>> entry points to manipulate kernel or I/O memory via framebuffers. It
>> also prevents the execution of driver code via ioctl or sysfs, both
>> of which might allow malicious software to exploit bugs in the fbdev
>> code.
>>
>> A small number of fbdev drivers require struct fbinfo.dev to be
>> initialized, usually for the support of sysfs interface. Make these
>> drivers depend on FB_DEVICE. They can later be fixed if necessary.
> Should that be a TODO in gpu/todo.rst?
> Otherwise the amount of people knowing about this
> is very close to 1.
> As an alternative add a TODO to each Kconfig file.
> 
>>
>> Signed-off-by: Thomas Zimmermann <[email protected]>
>> ---
>>   drivers/staging/fbtft/Kconfig            |  1 +
>>   drivers/video/fbdev/Kconfig              | 12 +++++++++
>>   drivers/video/fbdev/core/Makefile        |  7 +++---
>>   drivers/video/fbdev/core/fb_internal.h   | 32 ++++++++++++++++++++++++
>>   drivers/video/fbdev/omap2/omapfb/Kconfig |  2 +-
>>   include/linux/fb.h                       |  2 ++
>>   6 files changed, 52 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
>> index 4d29e8c1014e..5dda3c65a38e 100644
>> --- a/drivers/staging/fbtft/Kconfig
>> +++ b/drivers/staging/fbtft/Kconfig
>> @@ -2,6 +2,7 @@
>>   menuconfig FB_TFT
>>   	tristate "Support for small TFT LCD display modules"
>>   	depends on FB && SPI
>> +	depends on FB_DEVICE
>>   	depends on GPIOLIB || COMPILE_TEST
>>   	select FB_SYS_FILLRECT
>>   	select FB_SYS_COPYAREA
>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> index 6df9bd09454a..48d9a14f889c 100644
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>>   	  combination with certain motherboards and monitors are known to
>>   	  suffer from this problem.
>>   
>> +config FB_DEVICE
>> +        bool "Provide legacy /dev/fb* device"
>> +        depends on FB
>> +        help
>> +	  Say Y here if you want the legacy /dev/fb* device file. It's
>> +	  only required if you have userspace programs that depend on
>> +	  fbdev for graphics output. This does not effect the framebuffer
>> +	  console.
> tabs to spaces to indent the above correct.
> 
>> +
>>   config FB_DDC
>>   	tristate
>>   	depends on FB
>> @@ -1545,6 +1554,7 @@ config FB_3DFX_I2C
>>   config FB_VOODOO1
>>   	tristate "3Dfx Voodoo Graphics (sst1) support"
>>   	depends on FB && PCI
>> +	depends on FB_DEVICE
>>   	select FB_CFB_FILLRECT
>>   	select FB_CFB_COPYAREA
>>   	select FB_CFB_IMAGEBLIT
>> @@ -1862,6 +1872,7 @@ config FB_SH_MOBILE_LCDC
>>   	tristate "SuperH Mobile LCDC framebuffer support"
>>   	depends on FB && HAVE_CLK && HAS_IOMEM
>>   	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
>> +	depends on FB_DEVICE
>>   	select FB_SYS_FILLRECT
>>   	select FB_SYS_COPYAREA
>>   	select FB_SYS_IMAGEBLIT
>> @@ -1930,6 +1941,7 @@ config FB_SMSCUFX
>>   config FB_UDL
>>   	tristate "Displaylink USB Framebuffer support"
>>   	depends on FB && USB
>> +	depends on FB_DEVICE
>>   	select FB_MODE_HELPERS
>>   	select FB_SYS_FILLRECT
>>   	select FB_SYS_COPYAREA
>> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
>> index 125d24f50c36..d5e8772620f8 100644
>> --- a/drivers/video/fbdev/core/Makefile
>> +++ b/drivers/video/fbdev/core/Makefile
>> @@ -2,12 +2,13 @@
>>   obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
>>   obj-$(CONFIG_FB)                  += fb.o
>>   fb-y                              := fb_backlight.o \
>> -                                     fb_devfs.o \
>>                                        fb_info.o \
>> -                                     fb_procfs.o \
>> -                                     fbmem.o fbmon.o fbcmap.o fbsysfs.o \
>> +                                     fbmem.o fbmon.o fbcmap.o \
>>                                        modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o
>>   fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
>> +fb-$(CONFIG_FB_DEVICE)            += fb_devfs.o \
>> +                                     fb_procfs.o \
>> +                                     fbsysfs.o
> Maybe change this to one line to avoid '\'?
> 
>>   
>>   ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
>>   fb-y				  += fbcon.o bitblit.o softcursor.o
>> diff --git a/drivers/video/fbdev/core/fb_internal.h b/drivers/video/fbdev/core/fb_internal.h
>> index 0b43c0cd5096..b8a28466db79 100644
>> --- a/drivers/video/fbdev/core/fb_internal.h
>> +++ b/drivers/video/fbdev/core/fb_internal.h
>> @@ -3,12 +3,22 @@
>>   #ifndef _FB_INTERNAL_H
>>   #define _FB_INTERNAL_H
>>   
>> +#include <linux/device.h>
>>   #include <linux/fb.h>
>>   #include <linux/mutex.h>
>>   
>>   /* fb_devfs.c */
>> +#if defined(CONFIG_FB_DEVICE)
>>   int fb_register_chrdev(void);
>>   void fb_unregister_chrdev(void);
>> +#else
>> +static inline int fb_register_chrdev(void)
>> +{
>> +	return 0;
>> +}
>> +static inline void fb_unregister_chrdev(void)
>> +{ }
>> +#endif
>>   
>>   /* fbmem.c */
>>   extern struct class *fb_class;
>> @@ -19,11 +29,33 @@ struct fb_info *get_fb_info(unsigned int idx);
>>   void put_fb_info(struct fb_info *fb_info);
>>   
>>   /* fb_procfs.c */
>> +#if defined(CONFIG_FB_DEVICE)
>>   int fb_init_procfs(void);
>>   void fb_cleanup_procfs(void);
>> +#else
>> +static inline int fb_init_procfs(void)
>> +{
>> +	return 0;
>> +}
>> +static inline void fb_cleanup_procfs(void)
>> +{ }
>> +#endif
>>   
>>   /* fbsysfs.c */
>> +#if defined(CONFIG_FB_DEVICE)
>>   int fb_device_create(struct fb_info *fb_info);
>>   void fb_device_destroy(struct fb_info *fb_info);
>> +#else
>> +static inline int fb_device_create(struct fb_info *fb_info)
>> +{
>> +	get_device(fb_info->device); // as in device_add()
>> +
>> +	return 0;
>> +}
>> +static inline void fb_device_destroy(struct fb_info *fb_info)
>> +{
>> +	put_device(fb_info->device); // as in device_del()
>> +}
>> +#endif
> I do not see why fb_device_{create,destroy} needs to call
> {get,put}_device - and it is not explained.
> A short explanation in the commit maybe?

Ok, I'll do that.

This get_device() is normally done within device_create() from within 
register_framebuffer(). The put is within unregister_frambuffer(). 
Without, someone could attempt to unplug the Linux hardware device and 
our pointer would go stale. So we have to hold the reference within fbdev.

Best regards
Thomas

> 
> With my comments addressed:
> Reviewed-by: Sam Ravnborg <[email protected]>
> 
> Note: I do not engage in the thread about the best Kconfig
> solution - I trust the involved people will find a good solution.
> 
> 	Sam
> 
>>   
>>   #endif
>> diff --git a/drivers/video/fbdev/omap2/omapfb/Kconfig b/drivers/video/fbdev/omap2/omapfb/Kconfig
>> index 69f9cb03507e..21069fdb7cc2 100644
>> --- a/drivers/video/fbdev/omap2/omapfb/Kconfig
>> +++ b/drivers/video/fbdev/omap2/omapfb/Kconfig
>> @@ -5,9 +5,9 @@ config OMAP2_VRFB
>>   menuconfig FB_OMAP2
>>   	tristate "OMAP2+ frame buffer support"
>>   	depends on FB
>> +	depends on FB_DEVICE
>>   	depends on DRM_OMAP = n
>>   	depends on GPIOLIB
>> -
>>   	select FB_OMAP2_DSS
>>   	select OMAP2_VRFB if ARCH_OMAP2 || ARCH_OMAP3
>>   	select FB_CFB_FILLRECT
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index 541a0e3ce21f..40ed1028160c 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -481,7 +481,9 @@ struct fb_info {
>>   
>>   	const struct fb_ops *fbops;
>>   	struct device *device;		/* This is the parent */
>> +#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 */
>> -- 
>> 2.40.1

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)