Message-ID: <99a8b131-8e30-4ff1-3561-4ccbfa538a60@redhat.com>
Date:   Wed, 11 May 2022 14:24:21 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        dri-devel@...ts.freedesktop.org, Jonathan Corbet <corbet@....net>,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v5 2/7] firmware: sysfb: Add helpers to unregister a pdev
 and disable registration

Hello Thomas,

On 5/11/22 14:02, Thomas Zimmermann wrote:

[snip]

>> +
>> +/**
>> + * sysfb_disable() - disable the Generic System Framebuffers support
>> + *
>> + * This disables the registration of system framebuffer devices that match the
>> + * generic drivers that make use of the system framebuffer set up by firmware.
>> + *
>> + * It also unregisters a device if this was already registered by sysfb_init().
> 
> Why? I still cannot wrap my mind around, why we need to store *pd at all 
> and use sysfb_try_unregister() for unregistering.
>

Because on sysfb_disable(), the registered platform device has to unregistered.

And sysfb has no way to know if it was unregistered already or not unless that
stage is maintained in sysfb itself.

Let's have some examples assuming that we don't have this helper in sysfb
(will use the vc4 DRM driver just to avoid typing "a real DRM driver).

a) simplefb probed and then vc4

   1) "simple-framebuffer" pdev is registered by sysfb
   2) simplefb is registered and matches "simple-framebuffer"
   3) a vc4 device is registered by OF when parsing the DTB
   4) vc4 driver is registered, matches vc4 and probes
   5) vc4 requests the conflicting framebuffers to be removed
      and fbmem unregisters "simple-framebuffer"
   6) fbmem calls sysfb_disable()
   7) sysfb_disable() should unregister the pdev but can't
      because has no way to know that fbmem already did that.
 
b) vc4 probed and then simplefb.ko module is loaded

   1) "simple-framebuffer" pdev is registered by sysfb
   2) a vc4 device is registered by OF when parsing the DTB
   3) vc4 driver is registered, matches vc4 and probes
   4) vc4 requests the conflicting framebuffers to be removed
      and fbmem unregisters "simple-framebuffer"
   5) fbmem calls sysfb_disable()
   6) sysfb_disable() should unregister the pdev but can't
      because has no way to know that fbmem already did that.
   7) simplefb.ko is loaded and simplefb driver registered
   8) simplefb matches the registered "simple-framebuffer"
      and will wrongly probe and register a DRM device.

In option (a), making sysfb_disable() to attempt to unregister the device
that register in sysfb_init() will lead to a use-after-free if this was
already unregistered by fbmem in remove_conflicting_framebuffers(), so
it can't attempt to do that.

Same for option (b), but sysfb_disable() can't rely on fbmem to do the
unregistration because it only does for devices that are associated with
an already registered fbdev.

[snip]

>> + * Return:
>> + * * true          - the device was unregistered successfully
>> + * * false         - the device was not unregistered
>> + */
>> +bool sysfb_try_unregister(struct device *dev)
> 
> As it stands, I strongly object the use of this function as still don't 

No worries, it's my bad since I clearly failed to explain the rationale in
the commit message and comments.

> really get the purpose. It looks like a glorified wrapper around 
> platform_device_unregister(). Do we need disable_lock to serialize with 
> something else?
>

Yes, it has to serialize with sysfb_init() and sysfb_disable().
 
> Best regards
> Thomas
> 
> 
-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Powered by blists - more mailing lists