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

[Neil Armstrong <narmstrong@baylibre.com>]

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