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

From: Sam Ravnborg <sam@ravnborg.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: daniel@ffwll.ch, javierm@redhat.com, deller@gmx.de,
	geert+renesas@glider.be, lee@kernel.org,
	daniel.thompson@linaro.org, jingoohan1@gmail.com,
	linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-sh@vger.kernel.org, linux-omap@vger.kernel.org,
	linux-staging@lists.linux.dev
Subject: Re: [PATCH 30/30] fbdev: Make support for userspace interfaces configurable
Date: Sun, 11 Jun 2023 18:37:16 +0200	[thread overview]
Message-ID: <20230611163716.GA1139759@ravnborg.org> (raw)
In-Reply-To: <20230605144812.15241-31-tzimmermann@suse.de>

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 <tzimmermann@suse.de>
> ---
>  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?

With my comments addressed:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

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