[PATCH] drm/fb_helper: Allow leaking fbdev smem_start

[PATCH] drm/fb_helper: Allow leaking fbdev smem_start

[Neil Armstrong]

Since "drm/fb: Stop leaking physical address", the default behaviour of
the DRM fbdev emulation is to set the smem_base to 0 and pass the new
FBINFO_HIDE_SMEM_START flag.

The main reason is to avoid leaking physical addresse to user-space, and
it follows a general move over the kernel code to avoid user-space to
manipulate physical addresses and then use some other mechanisms like
dma-buf to transfer physical buffer handles over multiple subsystems.

But, a lot of devices depends on closed sources binaries to enable
OpenGL hardware acceleration that uses this smem_start value to
pass physical addresses to out-of-tree modules in order to render
into these physical adresses. These should use dma-buf buffers allocated
from the DRM display device instead and stop relying on fbdev overallocation
to gather DMA memory (some HW vendors delivers GBM and Wayland capable
binaries, but older unsupported devices won't have these new binaries
and are doomed until an Open Source solution like Lima finalizes).

Since these devices heavily depends on this kind of software and because
the smem_start population was available for years, it's a breakage to
stop leaking smem_start without any alternative solutions.

This patch adds a Kconfig depending on the EXPERT config and an unsafe
kernel module parameter tainting the kernel when enabled.

A clear comment and Kconfig help text was added to clarify why and when
this patch should be reverted, but in the meantime it's a necessary
feature to keep.

Cc: Dave Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Ben Skeggs <skeggsb@gmail.com>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/Kconfig         | 15 +++++++++++++++
 drivers/gpu/drm/drm_fb_helper.c | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 8871b3f..b07c298 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -110,6 +110,21 @@ config DRM_FBDEV_OVERALLOC
 	  is 100. Typical values for double buffering will be 200,
 	  triple buffering 300.
 
+config DRM_FBDEV_LEAK_PHYS_SMEM
+	bool "Shamelessly allow leaking of fbdev physical address (DANGEROUS)"
+	depends on DRM_FBDEV_EMULATION && EXPERT
+	default n
+	help
+	  In order to keep user-space compatibility, we want in certain
+	  use-cases to keep leaking the fbdev physical address to the
+	  user-space program handling the fbdev buffer.
+	  This option is a shame and should be removed as soon as possible
+	  and be considered as a broken and legacy behaviour from a modern
+	  fbdev device driver.
+
+	  If in doubt, say "N" or spread the word to your closed source
+	  library vendor.
+
 config DRM_LOAD_EDID_FIRMWARE
 	bool "Allow to specify an EDID data set instead of probing for it"
 	depends on DRM
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index bf2190c..a354944 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -56,6 +56,25 @@ MODULE_PARM_DESC(drm_fbdev_overalloc,
 		 "Overallocation of the fbdev buffer (%) [default="
 		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
 
+/*
+ * In order to keep user-space compatibility, we want in certain use-cases
+ * to keep leaking the fbdev physical address to the user-space program
+ * handling the fbdev buffer.
+ * This is a bad habit essentially kept into closed source opengl driver
+ * that should really be moved into open-source upstream projects instead
+ * of using legacy physical addresses in user space to communicate with
+ * other out-of-tree kernel modules.
+ *
+ * This module_param *should* be removed as soon as possible and be
+ * considered as a broken and legacy behaviour from a modern fbdev device.
+ */
+#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
+static bool drm_leak_fbdev_smem = false;
+module_param_unsafe(drm_leak_fbdev_smem, bool, 0600);
+MODULE_PARM_DESC(fbdev_emulation,
+		 "Allow unsafe leaking fbdev physical smem address [default=false]");
+#endif
+
 static LIST_HEAD(kernel_fb_helper_list);
 static DEFINE_MUTEX(kernel_fb_helper_lock);
 
@@ -2673,8 +2692,12 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper,
 
 	info = fb_helper->fbdev;
 	info->var.pixclock = 0;
-	/* don't leak any physical addresses to userspace */
-	info->flags |= FBINFO_HIDE_SMEM_START;
+	/* Shamelessly allow physical address leaking to userspace */
+#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
+	if (!drm_leak_fbdev_smem)
+#endif
+		/* don't leak any physical addresses to userspace */
+		info->flags |= FBINFO_HIDE_SMEM_START;
 
 	/* Need to drop locks to avoid recursive deadlock in
 	 * register_framebuffer. This is ok because the only thing left to do is
@@ -3083,6 +3106,12 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
 	fbi->fbops = &drm_fbdev_fb_ops;
 	fbi->screen_size = fb->height * fb->pitches[0];
 	fbi->fix.smem_len = fbi->screen_size;
+	/* Shamelessly leak the physical address to user-space */
+#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
+	if (drm_leak_fbdev_smem)
+		fbi->fix.smem_start =
+			page_to_phys(virt_to_page(fbi->screen_buffer));
+#endif
 	fbi->screen_buffer = buffer->vaddr;
 	strcpy(fbi->fix.id, "DRM emulated");
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb_helper: Allow leaking fbdev smem_start

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 6503 bytes --]

Hi,

On Thu, Sep 20, 2018 at 03:04:12PM +0200, Neil Armstrong wrote:
> Since "drm/fb: Stop leaking physical address", the default behaviour of
> the DRM fbdev emulation is to set the smem_base to 0 and pass the new
> FBINFO_HIDE_SMEM_START flag.
> 
> The main reason is to avoid leaking physical addresse to user-space, and
> it follows a general move over the kernel code to avoid user-space to
> manipulate physical addresses and then use some other mechanisms like
> dma-buf to transfer physical buffer handles over multiple subsystems.
> 
> But, a lot of devices depends on closed sources binaries to enable
> OpenGL hardware acceleration that uses this smem_start value to
> pass physical addresses to out-of-tree modules in order to render
> into these physical adresses. These should use dma-buf buffers allocated
> from the DRM display device instead and stop relying on fbdev overallocation
> to gather DMA memory (some HW vendors delivers GBM and Wayland capable
> binaries, but older unsupported devices won't have these new binaries
> and are doomed until an Open Source solution like Lima finalizes).
> 
> Since these devices heavily depends on this kind of software and because
> the smem_start population was available for years, it's a breakage to
> stop leaking smem_start without any alternative solutions.
> 
> This patch adds a Kconfig depending on the EXPERT config and an unsafe
> kernel module parameter tainting the kernel when enabled.
> 
> A clear comment and Kconfig help text was added to clarify why and when
> this patch should be reverted, but in the meantime it's a necessary
> feature to keep.
> 
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Ben Skeggs <skeggsb@gmail.com>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/Kconfig         | 15 +++++++++++++++
>  drivers/gpu/drm/drm_fb_helper.c | 33 +++++++++++++++++++++++++++++++--
>  2 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 8871b3f..b07c298 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -110,6 +110,21 @@ config DRM_FBDEV_OVERALLOC
>  	  is 100. Typical values for double buffering will be 200,
>  	  triple buffering 300.
>  
> +config DRM_FBDEV_LEAK_PHYS_SMEM
> +	bool "Shamelessly allow leaking of fbdev physical address (DANGEROUS)"
> +	depends on DRM_FBDEV_EMULATION && EXPERT
> +	default n
> +	help
> +	  In order to keep user-space compatibility, we want in certain
> +	  use-cases to keep leaking the fbdev physical address to the
> +	  user-space program handling the fbdev buffer.
> +	  This option is a shame and should be removed as soon as possible
> +	  and be considered as a broken and legacy behaviour from a modern
> +	  fbdev device driver.
> +
> +	  If in doubt, say "N" or spread the word to your closed source
> +	  library vendor.
> +
>  config DRM_LOAD_EDID_FIRMWARE
>  	bool "Allow to specify an EDID data set instead of probing for it"
>  	depends on DRM
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index bf2190c..a354944 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -56,6 +56,25 @@ MODULE_PARM_DESC(drm_fbdev_overalloc,
>  		 "Overallocation of the fbdev buffer (%) [default="
>  		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
>  
> +/*
> + * In order to keep user-space compatibility, we want in certain use-cases
> + * to keep leaking the fbdev physical address to the user-space program
> + * handling the fbdev buffer.
> + * This is a bad habit essentially kept into closed source opengl driver
> + * that should really be moved into open-source upstream projects instead
> + * of using legacy physical addresses in user space to communicate with
> + * other out-of-tree kernel modules.
> + *
> + * This module_param *should* be removed as soon as possible and be
> + * considered as a broken and legacy behaviour from a modern fbdev device.
> + */
> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> +static bool drm_leak_fbdev_smem = false;
> +module_param_unsafe(drm_leak_fbdev_smem, bool, 0600);
> +MODULE_PARM_DESC(fbdev_emulation,
> +		 "Allow unsafe leaking fbdev physical smem address [default=false]");
> +#endif
> +
>  static LIST_HEAD(kernel_fb_helper_list);
>  static DEFINE_MUTEX(kernel_fb_helper_lock);
>  
> @@ -2673,8 +2692,12 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper,
>  
>  	info = fb_helper->fbdev;
>  	info->var.pixclock = 0;
> -	/* don't leak any physical addresses to userspace */
> -	info->flags |= FBINFO_HIDE_SMEM_START;
> +	/* Shamelessly allow physical address leaking to userspace */
> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> +	if (!drm_leak_fbdev_smem)
> +#endif

You can use the IS_ENABLED directy within the if statement. It would
make the code a bit nicer.

> +		/* don't leak any physical addresses to userspace */
> +		info->flags |= FBINFO_HIDE_SMEM_START;
>  
>  	/* Need to drop locks to avoid recursive deadlock in
>  	 * register_framebuffer. This is ok because the only thing left to do is
> @@ -3083,6 +3106,12 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
>  	fbi->fbops = &drm_fbdev_fb_ops;
>  	fbi->screen_size = fb->height * fb->pitches[0];
>  	fbi->fix.smem_len = fbi->screen_size;
> +	/* Shamelessly leak the physical address to user-space */
> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> +	if (drm_leak_fbdev_smem)
> +		fbi->fix.smem_start =
> +			page_to_phys(virt_to_page(fbi->screen_buffer));
> +#endif

fbi->screen_buffer is only set..

>  	fbi->screen_buffer = buffer->vaddr;

.. at the next line. Moving that line before the if statement works.

With that fixed, and the IS_ENABLED in the if

Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com>
Tested-by: Maxime Ripard <maxime.ripard@bootlin.com>

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb_helper: Allow leaking fbdev smem_start

[Daniel Vetter]

On Fri, Sep 21, 2018 at 04:07:40PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Sep 20, 2018 at 03:04:12PM +0200, Neil Armstrong wrote:
> > Since "drm/fb: Stop leaking physical address", the default behaviour of
> > the DRM fbdev emulation is to set the smem_base to 0 and pass the new
> > FBINFO_HIDE_SMEM_START flag.
> > 
> > The main reason is to avoid leaking physical addresse to user-space, and
> > it follows a general move over the kernel code to avoid user-space to
> > manipulate physical addresses and then use some other mechanisms like
> > dma-buf to transfer physical buffer handles over multiple subsystems.
> > 
> > But, a lot of devices depends on closed sources binaries to enable
> > OpenGL hardware acceleration that uses this smem_start value to
> > pass physical addresses to out-of-tree modules in order to render
> > into these physical adresses. These should use dma-buf buffers allocated
> > from the DRM display device instead and stop relying on fbdev overallocation
> > to gather DMA memory (some HW vendors delivers GBM and Wayland capable
> > binaries, but older unsupported devices won't have these new binaries
> > and are doomed until an Open Source solution like Lima finalizes).
> > 
> > Since these devices heavily depends on this kind of software and because
> > the smem_start population was available for years, it's a breakage to
> > stop leaking smem_start without any alternative solutions.
> > 
> > This patch adds a Kconfig depending on the EXPERT config and an unsafe
> > kernel module parameter tainting the kernel when enabled.
> > 
> > A clear comment and Kconfig help text was added to clarify why and when
> > this patch should be reverted, but in the meantime it's a necessary
> > feature to keep.
> > 
> > Cc: Dave Airlie <airlied@gmail.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > Cc: Noralf Trønnes <noralf@tronnes.org>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Ben Skeggs <skeggsb@gmail.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> > ---
> >  drivers/gpu/drm/Kconfig         | 15 +++++++++++++++
> >  drivers/gpu/drm/drm_fb_helper.c | 33 +++++++++++++++++++++++++++++++--
> >  2 files changed, 46 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 8871b3f..b07c298 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -110,6 +110,21 @@ config DRM_FBDEV_OVERALLOC
> >  	  is 100. Typical values for double buffering will be 200,
> >  	  triple buffering 300.
> >  
> > +config DRM_FBDEV_LEAK_PHYS_SMEM
> > +	bool "Shamelessly allow leaking of fbdev physical address (DANGEROUS)"
> > +	depends on DRM_FBDEV_EMULATION && EXPERT
> > +	default n
> > +	help
> > +	  In order to keep user-space compatibility, we want in certain
> > +	  use-cases to keep leaking the fbdev physical address to the
> > +	  user-space program handling the fbdev buffer.
> > +	  This option is a shame and should be removed as soon as possible
> > +	  and be considered as a broken and legacy behaviour from a modern
> > +	  fbdev device driver.

s/shame/not supported by upstream developers/

And maybe add "Please send any bug reports when using this to your
proprietary software vendor that requires this."

I'd also be in favour of just calling this the ARM Mali blob on
$affected_devices. Makes it easier for people to find the magic knob
they're looking for, if they're unlucky enough to have such a soc.

> > +
> > +	  If in doubt, say "N" or spread the word to your closed source
> > +	  library vendor.
> > +
> >  config DRM_LOAD_EDID_FIRMWARE
> >  	bool "Allow to specify an EDID data set instead of probing for it"
> >  	depends on DRM
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index bf2190c..a354944 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -56,6 +56,25 @@ MODULE_PARM_DESC(drm_fbdev_overalloc,
> >  		 "Overallocation of the fbdev buffer (%) [default="
> >  		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
> >  
> > +/*
> > + * In order to keep user-space compatibility, we want in certain use-cases
> > + * to keep leaking the fbdev physical address to the user-space program
> > + * handling the fbdev buffer.
> > + * This is a bad habit essentially kept into closed source opengl driver
> > + * that should really be moved into open-source upstream projects instead
> > + * of using legacy physical addresses in user space to communicate with
> > + * other out-of-tree kernel modules.
> > + *
> > + * This module_param *should* be removed as soon as possible and be
> > + * considered as a broken and legacy behaviour from a modern fbdev device.
> > + */
> > +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> > +static bool drm_leak_fbdev_smem = false;
> > +module_param_unsafe(drm_leak_fbdev_smem, bool, 0600);
> > +MODULE_PARM_DESC(fbdev_emulation,
> > +		 "Allow unsafe leaking fbdev physical smem address [default=false]");
> > +#endif
> > +
> >  static LIST_HEAD(kernel_fb_helper_list);
> >  static DEFINE_MUTEX(kernel_fb_helper_lock);
> >  
> > @@ -2673,8 +2692,12 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper,
> >  
> >  	info = fb_helper->fbdev;
> >  	info->var.pixclock = 0;
> > -	/* don't leak any physical addresses to userspace */
> > -	info->flags |= FBINFO_HIDE_SMEM_START;
> > +	/* Shamelessly allow physical address leaking to userspace */
> > +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> > +	if (!drm_leak_fbdev_smem)
> > +#endif
> 
> You can use the IS_ENABLED directy within the if statement. It would
> make the code a bit nicer.
> 
> > +		/* don't leak any physical addresses to userspace */
> > +		info->flags |= FBINFO_HIDE_SMEM_START;
> >  
> >  	/* Need to drop locks to avoid recursive deadlock in
> >  	 * register_framebuffer. This is ok because the only thing left to do is
> > @@ -3083,6 +3106,12 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
> >  	fbi->fbops = &drm_fbdev_fb_ops;
> >  	fbi->screen_size = fb->height * fb->pitches[0];
> >  	fbi->fix.smem_len = fbi->screen_size;
> > +	/* Shamelessly leak the physical address to user-space */
> > +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> > +	if (drm_leak_fbdev_smem)
> > +		fbi->fix.smem_start =
> > +			page_to_phys(virt_to_page(fbi->screen_buffer));
> > +#endif
> 
> fbi->screen_buffer is only set..
> 
> >  	fbi->screen_buffer = buffer->vaddr;
> 
> .. at the next line. Moving that line before the if statement works.
> 
> With that fixed, and the IS_ENABLED in the if
> 
> Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com>
> Tested-by: Maxime Ripard <maxime.ripard@bootlin.com>

I think all the help text is sufficiently to scare people away from using
this by accident.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Thanks!
> Maxime
> 
> -- 
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb_helper: Allow leaking fbdev smem_start

[Neil Armstrong]

Hi Maxime, Daniel,

On 22/09/2018 10:56, Daniel Vetter wrote:
> On Fri, Sep 21, 2018 at 04:07:40PM +0200, Maxime Ripard wrote:
>> Hi,
>>
>> On Thu, Sep 20, 2018 at 03:04:12PM +0200, Neil Armstrong wrote:
>>> Since "drm/fb: Stop leaking physical address", the default behaviour of
>>> the DRM fbdev emulation is to set the smem_base to 0 and pass the new
>>> FBINFO_HIDE_SMEM_START flag.
>>>
>>> The main reason is to avoid leaking physical addresse to user-space, and
>>> it follows a general move over the kernel code to avoid user-space to
>>> manipulate physical addresses and then use some other mechanisms like
>>> dma-buf to transfer physical buffer handles over multiple subsystems.
>>>
>>> But, a lot of devices depends on closed sources binaries to enable
>>> OpenGL hardware acceleration that uses this smem_start value to
>>> pass physical addresses to out-of-tree modules in order to render
>>> into these physical adresses. These should use dma-buf buffers allocated
>>> from the DRM display device instead and stop relying on fbdev overallocation
>>> to gather DMA memory (some HW vendors delivers GBM and Wayland capable
>>> binaries, but older unsupported devices won't have these new binaries
>>> and are doomed until an Open Source solution like Lima finalizes).
>>>
>>> Since these devices heavily depends on this kind of software and because
>>> the smem_start population was available for years, it's a breakage to
>>> stop leaking smem_start without any alternative solutions.
>>>
>>> This patch adds a Kconfig depending on the EXPERT config and an unsafe
>>> kernel module parameter tainting the kernel when enabled.
>>>
>>> A clear comment and Kconfig help text was added to clarify why and when
>>> this patch should be reverted, but in the meantime it's a necessary
>>> feature to keep.
>>>
>>> Cc: Dave Airlie <airlied@gmail.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>> Cc: Noralf Trønnes <noralf@tronnes.org>
>>> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>> Cc: Rob Clark <robdclark@gmail.com>
>>> Cc: Ben Skeggs <skeggsb@gmail.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/gpu/drm/Kconfig         | 15 +++++++++++++++
>>>  drivers/gpu/drm/drm_fb_helper.c | 33 +++++++++++++++++++++++++++++++--
>>>  2 files changed, 46 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>> index 8871b3f..b07c298 100644
>>> --- a/drivers/gpu/drm/Kconfig
>>> +++ b/drivers/gpu/drm/Kconfig
>>> @@ -110,6 +110,21 @@ config DRM_FBDEV_OVERALLOC
>>>  	  is 100. Typical values for double buffering will be 200,
>>>  	  triple buffering 300.
>>>  
>>> +config DRM_FBDEV_LEAK_PHYS_SMEM
>>> +	bool "Shamelessly allow leaking of fbdev physical address (DANGEROUS)"
>>> +	depends on DRM_FBDEV_EMULATION && EXPERT
>>> +	default n
>>> +	help
>>> +	  In order to keep user-space compatibility, we want in certain
>>> +	  use-cases to keep leaking the fbdev physical address to the
>>> +	  user-space program handling the fbdev buffer.
>>> +	  This option is a shame and should be removed as soon as possible
>>> +	  and be considered as a broken and legacy behaviour from a modern
>>> +	  fbdev device driver.
> 
> s/shame/not supported by upstream developers/

Ack

> 
> And maybe add "Please send any bug reports when using this to your
> proprietary software vendor that requires this."

It's (somehow) on the following line :

>>> +	  If in doubt, say "N" or spread the word to your closed source
>>> +	  library vendor.

I will make it even scarier !

> 
> I'd also be in favour of just calling this the ARM Mali blob on
> $affected_devices. Makes it easier for people to find the magic knob
> they're looking for, if they're unlucky enough to have such a soc.

I'm not sure it's only for Mali... Anyway I'll add "Mali" to help
people findind it.

> 
>>> +
>>> +	  If in doubt, say "N" or spread the word to your closed source
>>> +	  library vendor.
>>> +
>>>  config DRM_LOAD_EDID_FIRMWARE
>>>  	bool "Allow to specify an EDID data set instead of probing for it"
>>>  	depends on DRM
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>> index bf2190c..a354944 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -56,6 +56,25 @@ MODULE_PARM_DESC(drm_fbdev_overalloc,
>>>  		 "Overallocation of the fbdev buffer (%) [default="
>>>  		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
>>>  
>>> +/*
>>> + * In order to keep user-space compatibility, we want in certain use-cases
>>> + * to keep leaking the fbdev physical address to the user-space program
>>> + * handling the fbdev buffer.
>>> + * This is a bad habit essentially kept into closed source opengl driver
>>> + * that should really be moved into open-source upstream projects instead
>>> + * of using legacy physical addresses in user space to communicate with
>>> + * other out-of-tree kernel modules.
>>> + *
>>> + * This module_param *should* be removed as soon as possible and be
>>> + * considered as a broken and legacy behaviour from a modern fbdev device.
>>> + */
>>> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
>>> +static bool drm_leak_fbdev_smem = false;
>>> +module_param_unsafe(drm_leak_fbdev_smem, bool, 0600);
>>> +MODULE_PARM_DESC(fbdev_emulation,
>>> +		 "Allow unsafe leaking fbdev physical smem address [default=false]");
>>> +#endif
>>> +
>>>  static LIST_HEAD(kernel_fb_helper_list);
>>>  static DEFINE_MUTEX(kernel_fb_helper_lock);
>>>  
>>> @@ -2673,8 +2692,12 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper,
>>>  
>>>  	info = fb_helper->fbdev;
>>>  	info->var.pixclock = 0;
>>> -	/* don't leak any physical addresses to userspace */
>>> -	info->flags |= FBINFO_HIDE_SMEM_START;
>>> +	/* Shamelessly allow physical address leaking to userspace */
>>> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
>>> +	if (!drm_leak_fbdev_smem)
>>> +#endif
>>
>> You can use the IS_ENABLED directy within the if statement. It would
>> make the code a bit nicer.

It would, but 01.org send me ugly mails with undeclared errors...

>>
>>> +		/* don't leak any physical addresses to userspace */
>>> +		info->flags |= FBINFO_HIDE_SMEM_START;
>>>  
>>>  	/* Need to drop locks to avoid recursive deadlock in
>>>  	 * register_framebuffer. This is ok because the only thing left to do is
>>> @@ -3083,6 +3106,12 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
>>>  	fbi->fbops = &drm_fbdev_fb_ops;
>>>  	fbi->screen_size = fb->height * fb->pitches[0];
>>>  	fbi->fix.smem_len = fbi->screen_size;
>>> +	/* Shamelessly leak the physical address to user-space */
>>> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
>>> +	if (drm_leak_fbdev_smem)
>>> +		fbi->fix.smem_start =
>>> +			page_to_phys(virt_to_page(fbi->screen_buffer));
>>> +#endif
>>
>> fbi->screen_buffer is only set..
>>
>>>  	fbi->screen_buffer = buffer->vaddr;
>>
>> .. at the next line. Moving that line before the if statement works.

Oops, will fix...

>>
>> With that fixed, and the IS_ENABLED in the if
>>
>> Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> Tested-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> I think all the help text is sufficiently to scare people away from using
> this by accident.
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks,
Neil

>>
>> Thanks!
>> Maxime
>>
>> -- 
>> Maxime Ripard, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
> 
> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fb_helper: Allow leaking fbdev smem_start

[Daniel Vetter]

On Mon, Sep 24, 2018 at 10:44:05AM +0200, Neil Armstrong wrote:
> Hi Maxime, Daniel,
> 
> On 22/09/2018 10:56, Daniel Vetter wrote:
> > On Fri, Sep 21, 2018 at 04:07:40PM +0200, Maxime Ripard wrote:
> >> Hi,
> >>
> >> On Thu, Sep 20, 2018 at 03:04:12PM +0200, Neil Armstrong wrote:
> >>> Since "drm/fb: Stop leaking physical address", the default behaviour of
> >>> the DRM fbdev emulation is to set the smem_base to 0 and pass the new
> >>> FBINFO_HIDE_SMEM_START flag.
> >>>
> >>> The main reason is to avoid leaking physical addresse to user-space, and
> >>> it follows a general move over the kernel code to avoid user-space to
> >>> manipulate physical addresses and then use some other mechanisms like
> >>> dma-buf to transfer physical buffer handles over multiple subsystems.
> >>>
> >>> But, a lot of devices depends on closed sources binaries to enable
> >>> OpenGL hardware acceleration that uses this smem_start value to
> >>> pass physical addresses to out-of-tree modules in order to render
> >>> into these physical adresses. These should use dma-buf buffers allocated
> >>> from the DRM display device instead and stop relying on fbdev overallocation
> >>> to gather DMA memory (some HW vendors delivers GBM and Wayland capable
> >>> binaries, but older unsupported devices won't have these new binaries
> >>> and are doomed until an Open Source solution like Lima finalizes).
> >>>
> >>> Since these devices heavily depends on this kind of software and because
> >>> the smem_start population was available for years, it's a breakage to
> >>> stop leaking smem_start without any alternative solutions.
> >>>
> >>> This patch adds a Kconfig depending on the EXPERT config and an unsafe
> >>> kernel module parameter tainting the kernel when enabled.
> >>>
> >>> A clear comment and Kconfig help text was added to clarify why and when
> >>> this patch should be reverted, but in the meantime it's a necessary
> >>> feature to keep.
> >>>
> >>> Cc: Dave Airlie <airlied@gmail.com>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> >>> Cc: Noralf Trønnes <noralf@tronnes.org>
> >>> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> >>> Cc: Eric Anholt <eric@anholt.net>
> >>> Cc: Lucas Stach <l.stach@pengutronix.de>
> >>> Cc: Rob Clark <robdclark@gmail.com>
> >>> Cc: Ben Skeggs <skeggsb@gmail.com>
> >>> Cc: Christian König <christian.koenig@amd.com>
> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >>> ---
> >>>  drivers/gpu/drm/Kconfig         | 15 +++++++++++++++
> >>>  drivers/gpu/drm/drm_fb_helper.c | 33 +++++++++++++++++++++++++++++++--
> >>>  2 files changed, 46 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >>> index 8871b3f..b07c298 100644
> >>> --- a/drivers/gpu/drm/Kconfig
> >>> +++ b/drivers/gpu/drm/Kconfig
> >>> @@ -110,6 +110,21 @@ config DRM_FBDEV_OVERALLOC
> >>>  	  is 100. Typical values for double buffering will be 200,
> >>>  	  triple buffering 300.
> >>>  
> >>> +config DRM_FBDEV_LEAK_PHYS_SMEM
> >>> +	bool "Shamelessly allow leaking of fbdev physical address (DANGEROUS)"
> >>> +	depends on DRM_FBDEV_EMULATION && EXPERT
> >>> +	default n
> >>> +	help
> >>> +	  In order to keep user-space compatibility, we want in certain
> >>> +	  use-cases to keep leaking the fbdev physical address to the
> >>> +	  user-space program handling the fbdev buffer.
> >>> +	  This option is a shame and should be removed as soon as possible
> >>> +	  and be considered as a broken and legacy behaviour from a modern
> >>> +	  fbdev device driver.
> > 
> > s/shame/not supported by upstream developers/
> 
> Ack
> 
> > 
> > And maybe add "Please send any bug reports when using this to your
> > proprietary software vendor that requires this."
> 
> It's (somehow) on the following line :
> 
> >>> +	  If in doubt, say "N" or spread the word to your closed source
> >>> +	  library vendor.
> 
> I will make it even scarier !
> 
> > 
> > I'd also be in favour of just calling this the ARM Mali blob on
> > $affected_devices. Makes it easier for people to find the magic knob
> > they're looking for, if they're unlucky enough to have such a soc.
> 
> I'm not sure it's only for Mali... Anyway I'll add "Mali" to help
> people findind it.
> 
> > 
> >>> +
> >>> +	  If in doubt, say "N" or spread the word to your closed source
> >>> +	  library vendor.
> >>> +
> >>>  config DRM_LOAD_EDID_FIRMWARE
> >>>  	bool "Allow to specify an EDID data set instead of probing for it"
> >>>  	depends on DRM
> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>> index bf2190c..a354944 100644
> >>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>> @@ -56,6 +56,25 @@ MODULE_PARM_DESC(drm_fbdev_overalloc,
> >>>  		 "Overallocation of the fbdev buffer (%) [default="
> >>>  		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
> >>>  
> >>> +/*
> >>> + * In order to keep user-space compatibility, we want in certain use-cases
> >>> + * to keep leaking the fbdev physical address to the user-space program
> >>> + * handling the fbdev buffer.
> >>> + * This is a bad habit essentially kept into closed source opengl driver
> >>> + * that should really be moved into open-source upstream projects instead
> >>> + * of using legacy physical addresses in user space to communicate with
> >>> + * other out-of-tree kernel modules.
> >>> + *
> >>> + * This module_param *should* be removed as soon as possible and be
> >>> + * considered as a broken and legacy behaviour from a modern fbdev device.
> >>> + */
> >>> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> >>> +static bool drm_leak_fbdev_smem = false;
> >>> +module_param_unsafe(drm_leak_fbdev_smem, bool, 0600);
> >>> +MODULE_PARM_DESC(fbdev_emulation,
> >>> +		 "Allow unsafe leaking fbdev physical smem address [default=false]");
> >>> +#endif
> >>> +
> >>>  static LIST_HEAD(kernel_fb_helper_list);
> >>>  static DEFINE_MUTEX(kernel_fb_helper_lock);
> >>>  
> >>> @@ -2673,8 +2692,12 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper,
> >>>  
> >>>  	info = fb_helper->fbdev;
> >>>  	info->var.pixclock = 0;
> >>> -	/* don't leak any physical addresses to userspace */
> >>> -	info->flags |= FBINFO_HIDE_SMEM_START;
> >>> +	/* Shamelessly allow physical address leaking to userspace */
> >>> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> >>> +	if (!drm_leak_fbdev_smem)
> >>> +#endif
> >>
> >> You can use the IS_ENABLED directy within the if statement. It would
> >> make the code a bit nicer.
> 
> It would, but 01.org send me ugly mails with undeclared errors...
> 
> >>
> >>> +		/* don't leak any physical addresses to userspace */
> >>> +		info->flags |= FBINFO_HIDE_SMEM_START;
> >>>  
> >>>  	/* Need to drop locks to avoid recursive deadlock in
> >>>  	 * register_framebuffer. This is ok because the only thing left to do is
> >>> @@ -3083,6 +3106,12 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
> >>>  	fbi->fbops = &drm_fbdev_fb_ops;
> >>>  	fbi->screen_size = fb->height * fb->pitches[0];
> >>>  	fbi->fix.smem_len = fbi->screen_size;
> >>> +	/* Shamelessly leak the physical address to user-space */
> >>> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> >>> +	if (drm_leak_fbdev_smem)
> >>> +		fbi->fix.smem_start =
> >>> +			page_to_phys(virt_to_page(fbi->screen_buffer));

One more: Maybe only overwrite smem_start when it's 0. This way drivers
can set it to something special still. Doesn't matter for anything using
cma, but does matter for some of the desktop gpus where the framebuffer is
behind a pci bar.
-Daniel

> >>> +#endif
> >>
> >> fbi->screen_buffer is only set..
> >>
> >>>  	fbi->screen_buffer = buffer->vaddr;
> >>
> >> .. at the next line. Moving that line before the if statement works.
> 
> Oops, will fix...
> 
> >>
> >> With that fixed, and the IS_ENABLED in the if
> >>
> >> Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> Tested-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > 
> > I think all the help text is sufficiently to scare people away from using
> > this by accident.
> > 
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Thanks,
> Neil
> 
> >>
> >> Thanks!
> >> Maxime
> >>
> >> -- 
> >> Maxime Ripard, Bootlin
> >> Embedded Linux and Kernel engineering
> >> https://bootlin.com
> > 
> > 
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel