From: Greg Kroah-Hartman <[email protected]>
[ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
The brcmfmac driver ignores any errors on initialization with the
different busses by deferring the initialization to a workqueue and
ignoring all possible errors that might happen. Fix up all of this by
only allowing the module to load if all bus registering worked properly.
Cc: Kalle Valo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
.../broadcom/brcm80211/brcmfmac/bcmsdh.c | 8 +---
.../broadcom/brcm80211/brcmfmac/bus.h | 19 ++++++++-
.../broadcom/brcm80211/brcmfmac/core.c | 42 ++++++++-----------
.../broadcom/brcm80211/brcmfmac/pcie.c | 9 +---
.../broadcom/brcm80211/brcmfmac/pcie.h | 5 ---
.../broadcom/brcm80211/brcmfmac/usb.c | 4 +-
6 files changed, 41 insertions(+), 46 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index fc12598b2dd3..c492d2d2db1d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1168,13 +1168,9 @@ static struct sdio_driver brcmf_sdmmc_driver = {
},
};
-void brcmf_sdio_register(void)
+int brcmf_sdio_register(void)
{
- int ret;
-
- ret = sdio_register_driver(&brcmf_sdmmc_driver);
- if (ret)
- brcmf_err("sdio_register_driver failed: %d\n", ret);
+ return sdio_register_driver(&brcmf_sdmmc_driver);
}
void brcmf_sdio_exit(void)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index 623c0168da79..8b27494a5d3d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -274,11 +274,26 @@ void brcmf_bus_add_txhdrlen(struct device *dev, uint len);
#ifdef CONFIG_BRCMFMAC_SDIO
void brcmf_sdio_exit(void);
-void brcmf_sdio_register(void);
+int brcmf_sdio_register(void);
+#else
+static inline void brcmf_sdio_exit(void) { }
+static inline int brcmf_sdio_register(void) { return 0; }
#endif
+
#ifdef CONFIG_BRCMFMAC_USB
void brcmf_usb_exit(void);
-void brcmf_usb_register(void);
+int brcmf_usb_register(void);
+#else
+static inline void brcmf_usb_exit(void) { }
+static inline int brcmf_usb_register(void) { return 0; }
+#endif
+
+#ifdef CONFIG_BRCMFMAC_PCIE
+void brcmf_pcie_exit(void);
+int brcmf_pcie_register(void);
+#else
+static inline void brcmf_pcie_exit(void) { }
+static inline int brcmf_pcie_register(void) { return 0; }
#endif
#endif /* BRCMFMAC_BUS_H */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index e9bb8dbdc9aa..edb79e9665dc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -1438,40 +1438,34 @@ void brcmf_bus_change_state(struct brcmf_bus *bus, enum brcmf_bus_state state)
}
}
-static void brcmf_driver_register(struct work_struct *work)
-{
-#ifdef CONFIG_BRCMFMAC_SDIO
- brcmf_sdio_register();
-#endif
-#ifdef CONFIG_BRCMFMAC_USB
- brcmf_usb_register();
-#endif
-#ifdef CONFIG_BRCMFMAC_PCIE
- brcmf_pcie_register();
-#endif
-}
-static DECLARE_WORK(brcmf_driver_work, brcmf_driver_register);
-
int __init brcmf_core_init(void)
{
- if (!schedule_work(&brcmf_driver_work))
- return -EBUSY;
+ int err;
+
+ err = brcmf_sdio_register();
+ if (err)
+ return err;
+
+ err = brcmf_usb_register();
+ if (err)
+ goto error_usb_register;
+ err = brcmf_pcie_register();
+ if (err)
+ goto error_pcie_register;
return 0;
+
+error_pcie_register:
+ brcmf_usb_exit();
+error_usb_register:
+ brcmf_sdio_exit();
+ return err;
}
void __exit brcmf_core_exit(void)
{
- cancel_work_sync(&brcmf_driver_work);
-
-#ifdef CONFIG_BRCMFMAC_SDIO
brcmf_sdio_exit();
-#endif
-#ifdef CONFIG_BRCMFMAC_USB
brcmf_usb_exit();
-#endif
-#ifdef CONFIG_BRCMFMAC_PCIE
brcmf_pcie_exit();
-#endif
}
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index cb68f54a9c56..bda042138e96 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -2137,15 +2137,10 @@ static struct pci_driver brcmf_pciedrvr = {
};
-void brcmf_pcie_register(void)
+int brcmf_pcie_register(void)
{
- int err;
-
brcmf_dbg(PCIE, "Enter\n");
- err = pci_register_driver(&brcmf_pciedrvr);
- if (err)
- brcmf_err(NULL, "PCIE driver registration failed, err=%d\n",
- err);
+ return pci_register_driver(&brcmf_pciedrvr);
}
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h
index d026401d2001..8e6c227e8315 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h
@@ -11,9 +11,4 @@ struct brcmf_pciedev {
struct brcmf_pciedev_info *devinfo;
};
-
-void brcmf_pcie_exit(void);
-void brcmf_pcie_register(void);
-
-
#endif /* BRCMFMAC_PCIE_H */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 6f41d28930e4..3b897f040371 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1558,8 +1558,8 @@ void brcmf_usb_exit(void)
usb_deregister(&brcmf_usbdrvr);
}
-void brcmf_usb_register(void)
+int brcmf_usb_register(void)
{
brcmf_dbg(USB, "Enter\n");
- usb_register(&brcmf_usbdrvr);
+ return usb_register(&brcmf_usbdrvr);
}
--
2.30.2
On 5/24/2021 4:48 PM, Sasha Levin wrote:
> From: Greg Kroah-Hartman <[email protected]>
>
> [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
>
> The brcmfmac driver ignores any errors on initialization with the
> different busses by deferring the initialization to a workqueue and
> ignoring all possible errors that might happen. Fix up all of this by
> only allowing the module to load if all bus registering worked properly.
Hi Greg,
Saw this one flying by for stable kernel. Actually the first time I saw
this patch, because I don't follow LKML as much as linux-wireless. The
patch is fine, but wanted to give some context on the workqueue
approach. It was there for historic reasons. Back then we had the UMH to
provide firmware loading and because we request firmware during driver
probe we could cause kernel boot to show significant delay when driver
was built-in. Hence the workqueue which allowed kernel boot to proceed
and driver probe was running in another thread context. These days we
have direct firmware loading from the kernel and brcmfmac uses the
asynchronous firmware loading API so there is indeed no longer a need
for the workqueue.
Just for my understanding could you explain the motivation behind this
change. In the preceding revert patch I saw this remark:
"""
The original commit here did nothing to actually help if usb_register()
failed, so it gives a "false sense of security" when there is none. The
correct solution is to correctly unwind from this error.
"""
Does this mean the patch is addressing some security issue. Before your
patch the module would remain loaded despite a bus register failure. I
guess there is a story behind this that I am curious about.
Regards,
Arend
> Cc: Kalle Valo <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 8 +---
> .../broadcom/brcm80211/brcmfmac/bus.h | 19 ++++++++-
> .../broadcom/brcm80211/brcmfmac/core.c | 42 ++++++++-----------
> .../broadcom/brcm80211/brcmfmac/pcie.c | 9 +---
> .../broadcom/brcm80211/brcmfmac/pcie.h | 5 ---
> .../broadcom/brcm80211/brcmfmac/usb.c | 4 +-
> 6 files changed, 41 insertions(+), 46 deletions(-)
On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
> On 5/24/2021 4:48 PM, Sasha Levin wrote:
> > From: Greg Kroah-Hartman <[email protected]>
> >
> > [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
> >
> > The brcmfmac driver ignores any errors on initialization with the
> > different busses by deferring the initialization to a workqueue and
> > ignoring all possible errors that might happen. Fix up all of this by
> > only allowing the module to load if all bus registering worked properly.
>
> Hi Greg,
>
> Saw this one flying by for stable kernel. Actually the first time I saw this
> patch, because I don't follow LKML as much as linux-wireless. The patch is
> fine, but wanted to give some context on the workqueue approach. It was
> there for historic reasons. Back then we had the UMH to provide firmware
> loading and because we request firmware during driver probe we could cause
> kernel boot to show significant delay when driver was built-in. Hence the
> workqueue which allowed kernel boot to proceed and driver probe was running
> in another thread context. These days we have direct firmware loading from
> the kernel and brcmfmac uses the asynchronous firmware loading API so there
> is indeed no longer a need for the workqueue.
>
> Just for my understanding could you explain the motivation behind this
> change. In the preceding revert patch I saw this remark:
>
> """
> The original commit here did nothing to actually help if usb_register()
> failed, so it gives a "false sense of security" when there is none. The
> correct solution is to correctly unwind from this error.
> """
>
> Does this mean the patch is addressing some security issue. Before your
> patch the module would remain loaded despite a bus register failure. I guess
> there is a story behind this that I am curious about.
The module would remain loaded, yes, but nothing would work, and so no
one would have any idea that something went wrong. The original commit
was wrong, it did not actually solve anything.
This commit properly propagates any error that happens back to the user,
like any other module being loaded.
thanks,
greg k-h
On 5/25/2021 8:43 AM, Greg Kroah-Hartman wrote:
> On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
>> On 5/24/2021 4:48 PM, Sasha Levin wrote:
>>> From: Greg Kroah-Hartman <[email protected]>
>>>
>>> [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
>>>
>>> The brcmfmac driver ignores any errors on initialization with the
>>> different busses by deferring the initialization to a workqueue and
>>> ignoring all possible errors that might happen. Fix up all of this by
>>> only allowing the module to load if all bus registering worked properly.
>>
>> Hi Greg,
>>
>> Saw this one flying by for stable kernel. Actually the first time I saw this
>> patch, because I don't follow LKML as much as linux-wireless. The patch is
>> fine, but wanted to give some context on the workqueue approach. It was
>> there for historic reasons. Back then we had the UMH to provide firmware
>> loading and because we request firmware during driver probe we could cause
>> kernel boot to show significant delay when driver was built-in. Hence the
>> workqueue which allowed kernel boot to proceed and driver probe was running
>> in another thread context. These days we have direct firmware loading from
>> the kernel and brcmfmac uses the asynchronous firmware loading API so there
>> is indeed no longer a need for the workqueue.
>>
>> Just for my understanding could you explain the motivation behind this
>> change. In the preceding revert patch I saw this remark:
>>
>> """
>> The original commit here did nothing to actually help if usb_register()
>> failed, so it gives a "false sense of security" when there is none. The
>> correct solution is to correctly unwind from this error.
>> """
>>
>> Does this mean the patch is addressing some security issue. Before your
>> patch the module would remain loaded despite a bus register failure. I guess
>> there is a story behind this that I am curious about.
>
> The module would remain loaded, yes, but nothing would work, and so no
> one would have any idea that something went wrong. The original commit
> was wrong, it did not actually solve anything.
Agree.
> This commit properly propagates any error that happens back to the user,
> like any other module being loaded.
I understand, but this might cause a regression for the user. For
instance if the usb_register() fails, but the other driver registrations
succeed and the user has a wireless PCIe device. Before this change the
user would have a functioning wifi device, but with this change it does not?
Regards,
Arend
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
On Tue, May 25, 2021 at 09:04:59AM +0200, Arend Van Spriel wrote:
> --
> This electronic communication and the information and any files transmitted
> with it, or attached to it, are confidential and are intended solely for the
> use of the individual or entity to whom it is addressed and may contain
> information that is confidential, legally privileged, protected by privacy
> laws, or otherwise restricted from disclosure to anyone else. If you are not
> the intended recipient or the person responsible for delivering the e-mail
> to the intended recipient, you are hereby notified that any use, copying,
> distributing, dissemination, forwarding, printing, or copying of this e-mail
> is strictly prohibited. If you received this e-mail in error, please return
> the e-mail to the sender, delete it from your computer, and destroy any
> printed copy of it.
Now deleted.
Resending without disclaimer
On 5/25/2021 9:04 AM, Arend Van Spriel wrote:
>
>
> On 5/25/2021 8:43 AM, Greg Kroah-Hartman wrote:
>> On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
>>> On 5/24/2021 4:48 PM, Sasha Levin wrote:
>>>> From: Greg Kroah-Hartman <[email protected]>
>>>>
>>>> [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
>>>>
>>>> The brcmfmac driver ignores any errors on initialization with the
>>>> different busses by deferring the initialization to a workqueue and
>>>> ignoring all possible errors that might happen. Fix up all of this by
>>>> only allowing the module to load if all bus registering worked
>>>> properly.
>>>
>>> Hi Greg,
>>>
>>> Saw this one flying by for stable kernel. Actually the first time I
>>> saw this
>>> patch, because I don't follow LKML as much as linux-wireless. The
>>> patch is
>>> fine, but wanted to give some context on the workqueue approach. It was
>>> there for historic reasons. Back then we had the UMH to provide firmware
>>> loading and because we request firmware during driver probe we could
>>> cause
>>> kernel boot to show significant delay when driver was built-in. Hence
>>> the
>>> workqueue which allowed kernel boot to proceed and driver probe was
>>> running
>>> in another thread context. These days we have direct firmware loading
>>> from
>>> the kernel and brcmfmac uses the asynchronous firmware loading API so
>>> there
>>> is indeed no longer a need for the workqueue.
>>>
>>> Just for my understanding could you explain the motivation behind this
>>> change. In the preceding revert patch I saw this remark:
>>>
>>> """
>>> The original commit here did nothing to actually help if usb_register()
>>> failed, so it gives a "false sense of security" when there is none. The
>>> correct solution is to correctly unwind from this error.
>>> """
>>>
>>> Does this mean the patch is addressing some security issue. Before your
>>> patch the module would remain loaded despite a bus register failure.
>>> I guess
>>> there is a story behind this that I am curious about.
>>
>> The module would remain loaded, yes, but nothing would work, and so no
>> one would have any idea that something went wrong. The original commit
>> was wrong, it did not actually solve anything.
>
> Agree.
>
>> This commit properly propagates any error that happens back to the user,
>> like any other module being loaded.
>
> I understand, but this might cause a regression for the user. For
> instance if the usb_register() fails, but the other driver registrations
> succeed and the user has a wireless PCIe device. Before this change the
> user would have a functioning wifi device, but with this change it does
> not?
>
> Regards,
> Arend
Sorry for that. Corporate email can be a drag.
Regards,
Arend
On Tue, May 25, 2021 at 09:23:41AM +0200, Arend van Spriel wrote:
> Resending without disclaimer
>
> On 5/25/2021 9:04 AM, Arend Van Spriel wrote:
> >
> >
> > On 5/25/2021 8:43 AM, Greg Kroah-Hartman wrote:
> > > On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
> > > > On 5/24/2021 4:48 PM, Sasha Levin wrote:
> > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > >
> > > > > [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
> > > > >
> > > > > The brcmfmac driver ignores any errors on initialization with the
> > > > > different busses by deferring the initialization to a workqueue and
> > > > > ignoring all possible errors that might happen.? Fix up all of this by
> > > > > only allowing the module to load if all bus registering
> > > > > worked properly.
> > > >
> > > > Hi Greg,
> > > >
> > > > Saw this one flying by for stable kernel. Actually the first
> > > > time I saw this
> > > > patch, because I don't follow LKML as much as linux-wireless.
> > > > The patch is
> > > > fine, but wanted to give some context on the workqueue approach. It was
> > > > there for historic reasons. Back then we had the UMH to provide firmware
> > > > loading and because we request firmware during driver probe we
> > > > could cause
> > > > kernel boot to show significant delay when driver was built-in.
> > > > Hence the
> > > > workqueue which allowed kernel boot to proceed and driver probe
> > > > was running
> > > > in another thread context. These days we have direct firmware
> > > > loading from
> > > > the kernel and brcmfmac uses the asynchronous firmware loading
> > > > API so there
> > > > is indeed no longer a need for the workqueue.
> > > >
> > > > Just for my understanding could you explain the motivation behind this
> > > > change. In the preceding revert patch I saw this remark:
> > > >
> > > > """
> > > > The original commit here did nothing to actually help if usb_register()
> > > > failed, so it gives a "false sense of security" when there is none.? The
> > > > correct solution is to correctly unwind from this error.
> > > > """
> > > >
> > > > Does this mean the patch is addressing some security issue. Before your
> > > > patch the module would remain loaded despite a bus register
> > > > failure. I guess
> > > > there is a story behind this that I am curious about.
> > >
> > > The module would remain loaded, yes, but nothing would work, and so no
> > > one would have any idea that something went wrong.? The original commit
> > > was wrong, it did not actually solve anything.
> >
> > Agree.
> >
> > > This commit properly propagates any error that happens back to the user,
> > > like any other module being loaded.
> >
> > I understand, but this might cause a regression for the user. For
> > instance if the usb_register() fails, but the other driver registrations
> > succeed and the user has a wireless PCIe device. Before this change the
> > user would have a functioning wifi device, but with this change it does
> > not?
If registering one of those other busses fails, you have major system
problems that need to be resolved and lots of other things will also
break.
You shouldn't just "eat error messages" and ignore them, as that's what
is what was happening here, you could have had errors and never knew it.
thanks,
greg k-h
On 5/25/2021 9:26 AM, Greg Kroah-Hartman wrote:
> On Tue, May 25, 2021 at 09:23:41AM +0200, Arend van Spriel wrote:
>> Resending without disclaimer
>>
>> On 5/25/2021 9:04 AM, Arend Van Spriel wrote:
>>>
>>>
>>> On 5/25/2021 8:43 AM, Greg Kroah-Hartman wrote:
>>>> On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
>>>>> On 5/24/2021 4:48 PM, Sasha Levin wrote:
>>>>>> From: Greg Kroah-Hartman <[email protected]>
>>>>>>
>>>>>> [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
>>>>>>
>>>>>> The brcmfmac driver ignores any errors on initialization with the
>>>>>> different busses by deferring the initialization to a workqueue and
>>>>>> ignoring all possible errors that might happen. Fix up all of this by
>>>>>> only allowing the module to load if all bus registering
>>>>>> worked properly.
>>>>>
>>>>> Hi Greg,
>>>>>
>>>>> Saw this one flying by for stable kernel. Actually the first
>>>>> time I saw this
>>>>> patch, because I don't follow LKML as much as linux-wireless.
>>>>> The patch is
>>>>> fine, but wanted to give some context on the workqueue approach. It was
>>>>> there for historic reasons. Back then we had the UMH to provide firmware
>>>>> loading and because we request firmware during driver probe we
>>>>> could cause
>>>>> kernel boot to show significant delay when driver was built-in.
>>>>> Hence the
>>>>> workqueue which allowed kernel boot to proceed and driver probe
>>>>> was running
>>>>> in another thread context. These days we have direct firmware
>>>>> loading from
>>>>> the kernel and brcmfmac uses the asynchronous firmware loading
>>>>> API so there
>>>>> is indeed no longer a need for the workqueue.
>>>>>
>>>>> Just for my understanding could you explain the motivation behind this
>>>>> change. In the preceding revert patch I saw this remark:
>>>>>
>>>>> """
>>>>> The original commit here did nothing to actually help if usb_register()
>>>>> failed, so it gives a "false sense of security" when there is none. The
>>>>> correct solution is to correctly unwind from this error.
>>>>> """
>>>>>
>>>>> Does this mean the patch is addressing some security issue. Before your
>>>>> patch the module would remain loaded despite a bus register
>>>>> failure. I guess
>>>>> there is a story behind this that I am curious about.
>>>>
>>>> The module would remain loaded, yes, but nothing would work, and so no
>>>> one would have any idea that something went wrong. The original commit
>>>> was wrong, it did not actually solve anything.
>>>
>>> Agree.
>>>
>>>> This commit properly propagates any error that happens back to the user,
>>>> like any other module being loaded.
>>>
>>> I understand, but this might cause a regression for the user. For
>>> instance if the usb_register() fails, but the other driver registrations
>>> succeed and the user has a wireless PCIe device. Before this change the
>>> user would have a functioning wifi device, but with this change it does
>>> not?
>
> If registering one of those other busses fails, you have major system
> problems that need to be resolved and lots of other things will also
> break.
Right.
> You shouldn't just "eat error messages" and ignore them, as that's what
> is what was happening here, you could have had errors and never knew it.
As said earlier I agree with the patch. I just thought I might learn
something today, because there was more to it than I could find in the
commit message. :-p
Regards,
Arend