| Message ID | 20220511112438.1251024-3-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | Fix some races between sysfb device registration and drivers probe | expand |
Hi Am 11.05.22 um 13:24 schrieb Javier Martinez Canillas: > These can be used by subsystems to unregister a platform device registered > by sysfb and also to disable future platform device registration in sysfb. > I find it very hard to review these patches, as related things are put into separate patches. I suggest to put the sysfb_disable() stuff into patch 5 and the rest into patch 4. Best regards Thomas > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > > (no changes since v4) > > Changes in v4: > - Make sysfb_disable() to also attempt to unregister a device. > > Changes in v2: > - Add kernel-doc comments and include in other_interfaces.rst (Daniel Vetter). > > .../driver-api/firmware/other_interfaces.rst | 6 ++ > drivers/firmware/sysfb.c | 87 +++++++++++++++++-- > include/linux/sysfb.h | 19 ++++ > 3 files changed, 106 insertions(+), 6 deletions(-) > > diff --git a/Documentation/driver-api/firmware/other_interfaces.rst b/Documentation/driver-api/firmware/other_interfaces.rst > index b81794e0cfbb..06ac89adaafb 100644 > --- a/Documentation/driver-api/firmware/other_interfaces.rst > +++ b/Documentation/driver-api/firmware/other_interfaces.rst > @@ -13,6 +13,12 @@ EDD Interfaces > .. kernel-doc:: drivers/firmware/edd.c > :internal: > > +Generic System Framebuffers Interface > +------------------------------------- > + > +.. kernel-doc:: drivers/firmware/sysfb.c > + :export: > + > Intel Stratix10 SoC Service Layer > --------------------------------- > Some features of the Intel Stratix10 SoC require a level of privilege > diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c > index b032f40a92de..6768968949e6 100644 > --- a/drivers/firmware/sysfb.c > +++ b/drivers/firmware/sysfb.c > @@ -34,21 +34,92 @@ > #include <linux/screen_info.h> > #include <linux/sysfb.h> > > +static struct platform_device *pd; > +static DEFINE_MUTEX(disable_lock); > +static bool disabled; > + > +static bool sysfb_unregister(void) > +{ > + if (IS_ERR_OR_NULL(pd)) > + return false; > + > + platform_device_unregister(pd); > + pd = NULL; > + > + return true; > +} > + > +/** > + * 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(). > + * > + * Context: The function can sleep. A @disable_lock mutex is acquired to serialize > + * against sysfb_init(), that registers a system framebuffer device and > + * sysfb_try_unregister(), that tries to unregister a framebuffer device. > + */ > +void sysfb_disable(void) > +{ > + mutex_lock(&disable_lock); > + sysfb_unregister(); > + disabled = true; > + mutex_unlock(&disable_lock); > +} > +EXPORT_SYMBOL_GPL(sysfb_disable); > + > +/** > + * sysfb_try_unregister() - attempt to unregister a system framebuffer device > + * @dev: device to unregister > + * > + * This tries to unregister a system framebuffer device if this was registered > + * by the Generic System Framebuffers. The device will only be unregistered if > + * it was registered by sysfb_init(), otherwise it will not be unregistered. > + * > + * Context: The function can sleep. a @load_lock mutex is acquired to serialize > + * against sysfb_init(), that registers a simple framebuffer device and > + * sysfb_disable(), that disables the Generic System Framebuffers support. > + * > + * Return: > + * * true - the device was unregistered successfully > + * * false - the device was not unregistered > + */ > +bool sysfb_try_unregister(struct device *dev) > +{ > + bool ret = false; > + > + mutex_lock(&disable_lock); > + if (IS_ERR_OR_NULL(pd) || pd != to_platform_device(dev)) > + goto unlock_mutex; > + > + ret = sysfb_unregister(); > + > +unlock_mutex: > + mutex_unlock(&disable_lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(sysfb_try_unregister); > + > static __init int sysfb_init(void) > { > struct screen_info *si = &screen_info; > struct simplefb_platform_data mode; > - struct platform_device *pd; > const char *name; > bool compatible; > - int ret; > + int ret = 0; > + > + mutex_lock(&disable_lock); > + if (disabled) > + goto unlock_mutex; > > /* try to create a simple-framebuffer device */ > compatible = sysfb_parse_mode(si, &mode); > if (compatible) { > pd = sysfb_create_simplefb(si, &mode); > if (!IS_ERR(pd)) > - return 0; > + goto unlock_mutex; > } > > /* if the FB is incompatible, create a legacy framebuffer device */ > @@ -60,8 +131,10 @@ static __init int sysfb_init(void) > name = "platform-framebuffer"; > > pd = platform_device_alloc(name, 0); > - if (!pd) > - return -ENOMEM; > + if (!pd) { > + ret = -ENOMEM; > + goto unlock_mutex; > + } > > sysfb_apply_efi_quirks(pd); > > @@ -73,9 +146,11 @@ static __init int sysfb_init(void) > if (ret) > goto err; > > - return 0; > + goto unlock_mutex; > err: > platform_device_put(pd); > +unlock_mutex: > + mutex_unlock(&disable_lock); > return ret; > } > > diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h > index 708152e9037b..e8c0313fac8f 100644 > --- a/include/linux/sysfb.h > +++ b/include/linux/sysfb.h > @@ -55,6 +55,25 @@ struct efifb_dmi_info { > int flags; > }; > > +#ifdef CONFIG_SYSFB > + > +void sysfb_disable(void); > +bool sysfb_try_unregister(struct device *dev); > + > +#else /* CONFIG_SYSFB */ > + > +static inline void sysfb_disable(void) > +{ > + > +} > + > +static inline bool sysfb_try_unregister(struct device *dev) > +{ > + return false; > +} > + > +#endif /* CONFIG_SYSFB */ > + > #ifdef CONFIG_EFI > > extern struct efifb_dmi_info efifb_dmi_list[];
Hello Thomas, On 5/11/22 13:54, Thomas Zimmermann wrote: > Hi > > Am 11.05.22 um 13:24 schrieb Javier Martinez Canillas: >> These can be used by subsystems to unregister a platform device registered >> by sysfb and also to disable future platform device registration in sysfb. >> > > I find it very hard to review these patches, as related things are put > into separate patches. > > I suggest to put the sysfb_disable() stuff into patch 5 and the rest > into patch 4. > Ok, you then want in the same patch to have both the helper definition and first usage. Other subsystems ask you to do the opposite, to split the definition and usage in separate patches. But I'm fine with merging those if you prefer. > Best regards > Thomas > >
Am 11.05.22 um 13:24 schrieb Javier Martinez Canillas: > These can be used by subsystems to unregister a platform device registered > by sysfb and also to disable future platform device registration in sysfb. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > > (no changes since v4) > > Changes in v4: > - Make sysfb_disable() to also attempt to unregister a device. > > Changes in v2: > - Add kernel-doc comments and include in other_interfaces.rst (Daniel Vetter). > > .../driver-api/firmware/other_interfaces.rst | 6 ++ > drivers/firmware/sysfb.c | 87 +++++++++++++++++-- > include/linux/sysfb.h | 19 ++++ > 3 files changed, 106 insertions(+), 6 deletions(-) > > diff --git a/Documentation/driver-api/firmware/other_interfaces.rst b/Documentation/driver-api/firmware/other_interfaces.rst > index b81794e0cfbb..06ac89adaafb 100644 > --- a/Documentation/driver-api/firmware/other_interfaces.rst > +++ b/Documentation/driver-api/firmware/other_interfaces.rst > @@ -13,6 +13,12 @@ EDD Interfaces > .. kernel-doc:: drivers/firmware/edd.c > :internal: > > +Generic System Framebuffers Interface > +------------------------------------- > + > +.. kernel-doc:: drivers/firmware/sysfb.c > + :export: > + > Intel Stratix10 SoC Service Layer > --------------------------------- > Some features of the Intel Stratix10 SoC require a level of privilege > diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c > index b032f40a92de..6768968949e6 100644 > --- a/drivers/firmware/sysfb.c > +++ b/drivers/firmware/sysfb.c > @@ -34,21 +34,92 @@ > #include <linux/screen_info.h> > #include <linux/sysfb.h> > > +static struct platform_device *pd; > +static DEFINE_MUTEX(disable_lock); > +static bool disabled; > + > +static bool sysfb_unregister(void) > +{ > + if (IS_ERR_OR_NULL(pd)) > + return false; > + > + platform_device_unregister(pd); > + pd = NULL; > + > + return true; > +} > + > +/** > + * 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. > + * > + * Context: The function can sleep. A @disable_lock mutex is acquired to serialize > + * against sysfb_init(), that registers a system framebuffer device and > + * sysfb_try_unregister(), that tries to unregister a framebuffer device. > + */ > +void sysfb_disable(void) > +{ > + mutex_lock(&disable_lock); > + sysfb_unregister(); > + disabled = true; > + mutex_unlock(&disable_lock); > +} > +EXPORT_SYMBOL_GPL(sysfb_disable); > + > +/** > + * sysfb_try_unregister() - attempt to unregister a system framebuffer device > + * @dev: device to unregister > + * > + * This tries to unregister a system framebuffer device if this was registered > + * by the Generic System Framebuffers. The device will only be unregistered if > + * it was registered by sysfb_init(), otherwise it will not be unregistered. > + * > + * Context: The function can sleep. a @load_lock mutex is acquired to serialize > + * against sysfb_init(), that registers a simple framebuffer device and > + * sysfb_disable(), that disables the Generic System Framebuffers support. > + * > + * 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 really get the purpose. It looks like a glorified wrapper around platform_device_unregister(). Do we need disable_lock to serialize with something else? Best regards Thomas > +{ > + bool ret = false; > + > + mutex_lock(&disable_lock); > + if (IS_ERR_OR_NULL(pd) || pd != to_platform_device(dev)) > + goto unlock_mutex; > + > + ret = sysfb_unregister(); > + > +unlock_mutex: > + mutex_unlock(&disable_lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(sysfb_try_unregister); > + > static __init int sysfb_init(void) > { > struct screen_info *si = &screen_info; > struct simplefb_platform_data mode; > - struct platform_device *pd; > const char *name; > bool compatible; > - int ret; > + int ret = 0; > + > + mutex_lock(&disable_lock); > + if (disabled) > + goto unlock_mutex; > > /* try to create a simple-framebuffer device */ > compatible = sysfb_parse_mode(si, &mode); > if (compatible) { > pd = sysfb_create_simplefb(si, &mode); > if (!IS_ERR(pd)) > - return 0; > + goto unlock_mutex; > } > > /* if the FB is incompatible, create a legacy framebuffer device */ > @@ -60,8 +131,10 @@ static __init int sysfb_init(void) > name = "platform-framebuffer"; > > pd = platform_device_alloc(name, 0); > - if (!pd) > - return -ENOMEM; > + if (!pd) { > + ret = -ENOMEM; > + goto unlock_mutex; > + } > > sysfb_apply_efi_quirks(pd); > > @@ -73,9 +146,11 @@ static __init int sysfb_init(void) > if (ret) > goto err; > > - return 0; > + goto unlock_mutex; > err: > platform_device_put(pd); > +unlock_mutex: > + mutex_unlock(&disable_lock); > return ret; > } > > diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h > index 708152e9037b..e8c0313fac8f 100644 > --- a/include/linux/sysfb.h > +++ b/include/linux/sysfb.h > @@ -55,6 +55,25 @@ struct efifb_dmi_info { > int flags; > }; > > +#ifdef CONFIG_SYSFB > + > +void sysfb_disable(void); > +bool sysfb_try_unregister(struct device *dev); > + > +#else /* CONFIG_SYSFB */ > + > +static inline void sysfb_disable(void) > +{ > + > +} > + > +static inline bool sysfb_try_unregister(struct device *dev) > +{ > + return false; > +} > + > +#endif /* CONFIG_SYSFB */ > + > #ifdef CONFIG_EFI > > extern struct efifb_dmi_info efifb_dmi_list[];
Hi Am 11.05.22 um 14:01 schrieb Javier Martinez Canillas: > Hello Thomas, > > On 5/11/22 13:54, Thomas Zimmermann wrote: >> Hi >> >> Am 11.05.22 um 13:24 schrieb Javier Martinez Canillas: >>> These can be used by subsystems to unregister a platform device registered >>> by sysfb and also to disable future platform device registration in sysfb. >>> >> >> I find it very hard to review these patches, as related things are put >> into separate patches. >> >> I suggest to put the sysfb_disable() stuff into patch 5 and the rest >> into patch 4. >> > > Ok, you then want in the same patch to have both the helper definition > and first usage. > > Other subsystems ask you to do the opposite, to split the definition and > usage in separate patches. But I'm fine with merging those if you prefer. Usually, I have no strong opinion on this. But in the case of this specific patchset, I have the feeling that I'm missing some important point because call and implementation are separate. See my other replies for that. Putting them next to each other will hopefully help. Sorry for the inconvenience. Best regards Thomas > >> Best regards >> Thomas >> >>
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 > >
Hello Thomas, On 5/11/22 14:05, Thomas Zimmermann wrote: [snip] >> >> Other subsystems ask you to do the opposite, to split the definition and >> usage in separate patches. But I'm fine with merging those if you prefer. > > Usually, I have no strong opinion on this. But in the case of this > specific patchset, I have the feeling that I'm missing some important > point because call and implementation are separate. See my other > replies for that. Putting them next to each other will hopefully help. > Sorry for the inconvenience. > No worries at all. Happy to do that change if the patches are easy to understand. It took me some time as well to wrap my head around all the race conditions and needed locking. Same for patch 3/7, but I'm convinced that dropping the lock is the correct thing to do than calling to drivers' .remove callbacks with a lock held.
diff --git a/Documentation/driver-api/firmware/other_interfaces.rst b/Documentation/driver-api/firmware/other_interfaces.rst index b81794e0cfbb..06ac89adaafb 100644 --- a/Documentation/driver-api/firmware/other_interfaces.rst +++ b/Documentation/driver-api/firmware/other_interfaces.rst @@ -13,6 +13,12 @@ EDD Interfaces .. kernel-doc:: drivers/firmware/edd.c :internal: +Generic System Framebuffers Interface +------------------------------------- + +.. kernel-doc:: drivers/firmware/sysfb.c + :export: + Intel Stratix10 SoC Service Layer --------------------------------- Some features of the Intel Stratix10 SoC require a level of privilege diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c index b032f40a92de..6768968949e6 100644 --- a/drivers/firmware/sysfb.c +++ b/drivers/firmware/sysfb.c @@ -34,21 +34,92 @@ #include <linux/screen_info.h> #include <linux/sysfb.h> +static struct platform_device *pd; +static DEFINE_MUTEX(disable_lock); +static bool disabled; + +static bool sysfb_unregister(void) +{ + if (IS_ERR_OR_NULL(pd)) + return false; + + platform_device_unregister(pd); + pd = NULL; + + return true; +} + +/** + * 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(). + * + * Context: The function can sleep. A @disable_lock mutex is acquired to serialize + * against sysfb_init(), that registers a system framebuffer device and + * sysfb_try_unregister(), that tries to unregister a framebuffer device. + */ +void sysfb_disable(void) +{ + mutex_lock(&disable_lock); + sysfb_unregister(); + disabled = true; + mutex_unlock(&disable_lock); +} +EXPORT_SYMBOL_GPL(sysfb_disable); + +/** + * sysfb_try_unregister() - attempt to unregister a system framebuffer device + * @dev: device to unregister + * + * This tries to unregister a system framebuffer device if this was registered + * by the Generic System Framebuffers. The device will only be unregistered if + * it was registered by sysfb_init(), otherwise it will not be unregistered. + * + * Context: The function can sleep. a @load_lock mutex is acquired to serialize + * against sysfb_init(), that registers a simple framebuffer device and + * sysfb_disable(), that disables the Generic System Framebuffers support. + * + * Return: + * * true - the device was unregistered successfully + * * false - the device was not unregistered + */ +bool sysfb_try_unregister(struct device *dev) +{ + bool ret = false; + + mutex_lock(&disable_lock); + if (IS_ERR_OR_NULL(pd) || pd != to_platform_device(dev)) + goto unlock_mutex; + + ret = sysfb_unregister(); + +unlock_mutex: + mutex_unlock(&disable_lock); + return ret; +} +EXPORT_SYMBOL_GPL(sysfb_try_unregister); + static __init int sysfb_init(void) { struct screen_info *si = &screen_info; struct simplefb_platform_data mode; - struct platform_device *pd; const char *name; bool compatible; - int ret; + int ret = 0; + + mutex_lock(&disable_lock); + if (disabled) + goto unlock_mutex; /* try to create a simple-framebuffer device */ compatible = sysfb_parse_mode(si, &mode); if (compatible) { pd = sysfb_create_simplefb(si, &mode); if (!IS_ERR(pd)) - return 0; + goto unlock_mutex; } /* if the FB is incompatible, create a legacy framebuffer device */ @@ -60,8 +131,10 @@ static __init int sysfb_init(void) name = "platform-framebuffer"; pd = platform_device_alloc(name, 0); - if (!pd) - return -ENOMEM; + if (!pd) { + ret = -ENOMEM; + goto unlock_mutex; + } sysfb_apply_efi_quirks(pd); @@ -73,9 +146,11 @@ static __init int sysfb_init(void) if (ret) goto err; - return 0; + goto unlock_mutex; err: platform_device_put(pd); +unlock_mutex: + mutex_unlock(&disable_lock); return ret; } diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h index 708152e9037b..e8c0313fac8f 100644 --- a/include/linux/sysfb.h +++ b/include/linux/sysfb.h @@ -55,6 +55,25 @@ struct efifb_dmi_info { int flags; }; +#ifdef CONFIG_SYSFB + +void sysfb_disable(void); +bool sysfb_try_unregister(struct device *dev); + +#else /* CONFIG_SYSFB */ + +static inline void sysfb_disable(void) +{ + +} + +static inline bool sysfb_try_unregister(struct device *dev) +{ + return false; +} + +#endif /* CONFIG_SYSFB */ + #ifdef CONFIG_EFI extern struct efifb_dmi_info efifb_dmi_list[];