mbox series

[0/4] wifi: rtlwifi probe error path fixes

Message ID 20241122172718.465539-1-cascardo@igalia.com
Headers show
Series wifi: rtlwifi probe error path fixes | expand

Message

Thadeu Lima de Souza Cascardo Nov. 22, 2024, 5:27 p.m. UTC
These fix different bugs when the probe fails. One of them is the addition
to a global list, which is not removed during the error path. That list has
been removed.

Then, some memory leaks are fixed, which require a change in where the
workqueue is destroyed.

Finally, the firmware completion is waited to prevent its callback from
accessing freed data.

These were tested against an "emulated" rtl8192se. It was a changed rtl8139
device under qemu with the rtl8192se PCI ID. Memory allocation failures
were injected over 4 different places: init_sw_vars, rtl_pci_init,
rtl_init_core and ieee80211_register_hw.

Thadeu Lima de Souza Cascardo (4):
  wifi: rtlwifi: remove unused check_buddy_priv
  wifi: rltwifi: destroy workqueue at rtl_deinit_core
  wifi: rtlwifi: fix memory leaks and invalid access at probe error path
  wifi: rtlwifi: pci: wait for firmware loading before releasing memory

 drivers/net/wireless/realtek/rtlwifi/base.c | 12 ++---
 drivers/net/wireless/realtek/rtlwifi/base.h |  1 -
 drivers/net/wireless/realtek/rtlwifi/pci.c  | 60 ++++-----------------
 drivers/net/wireless/realtek/rtlwifi/usb.c  |  5 --
 drivers/net/wireless/realtek/rtlwifi/wifi.h | 12 -----
 5 files changed, 14 insertions(+), 76 deletions(-)

Comments

Ping-Ke Shih Nov. 27, 2024, 6:44 a.m. UTC | #1
Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> These fix different bugs when the probe fails. One of them is the addition
> to a global list, which is not removed during the error path. That list has
> been removed.
> 
> Then, some memory leaks are fixed, which require a change in where the
> workqueue is destroyed.
> 
> Finally, the firmware completion is waited to prevent its callback from
> accessing freed data.
> 
> These were tested against an "emulated" rtl8192se. It was a changed rtl8139
> device under qemu with the rtl8192se PCI ID.

Interesting. Does it mean qemu can support PCI pass-through to work with
real hardware? 

> Memory allocation failures
> were injected over 4 different places: init_sw_vars, rtl_pci_init,
> rtl_init_core and ieee80211_register_hw.
> 

For the Fixes tag of cleanup patches, I'm not sure if it should be or not.
We can keep them and leave maintainers to decide taking to stable tree or not.
If that happens, please carefully check the dependency of these patches.
Thadeu Lima de Souza Cascardo Nov. 27, 2024, 9:13 a.m. UTC | #2
On Wed, Nov 27, 2024 at 06:44:44AM +0000, Ping-Ke Shih wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> > These fix different bugs when the probe fails. One of them is the addition
> > to a global list, which is not removed during the error path. That list has
> > been removed.
> > 
> > Then, some memory leaks are fixed, which require a change in where the
> > workqueue is destroyed.
> > 
> > Finally, the firmware completion is waited to prevent its callback from
> > accessing freed data.
> > 
> > These were tested against an "emulated" rtl8192se. It was a changed rtl8139
> > device under qemu with the rtl8192se PCI ID.
> 
> Interesting. Does it mean qemu can support PCI pass-through to work with
> real hardware? 
> 

Well, it does, but since I don't have real hardware available, I did a
quick change in qemu such that linux would probe the rtl8192se driver. It
wouldn't work as a complete device, but it would be sufficient to complete
the probe process and let me test the many error paths.

> > Memory allocation failures
> > were injected over 4 different places: init_sw_vars, rtl_pci_init,
> > rtl_init_core and ieee80211_register_hw.
> > 
> 
> For the Fixes tag of cleanup patches, I'm not sure if it should be or not.
> We can keep them and leave maintainers to decide taking to stable tree or not.
> If that happens, please carefully check the dependency of these patches. 
> 
> 

I decided to add the Fixes: tags on the cleanup patches as they are either
dependencies and necessary for the followup patches or really fix a problem
of their own. I will comment on those two patches.

Thanks.
Cascardo.