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

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

[Thadeu Lima de Souza Cascardo]

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(-)

-- 
2.34.1

[PATCH 1/4] wifi: rtlwifi: remove unused check_buddy_priv

[Thadeu Lima de Souza Cascardo]

Commit 2461c7d60f9f ("rtlwifi: Update header file") introduced a global
list of private data structures.

Later on, commit 26634c4b1868 ("rtlwifi Modify existing bits to match
vendor version 2013.02.07") started adding the private data to that list at
probe time and added a hook, check_buddy_priv to find the private data from
a similar device.

However, that function was never used.

Besides, though there is a lock for that list, it is never used. And when
the probe fails, the private data is never removed from the list. This
would cause a second probe to access freed memory.

Remove the unused hook, structures and members, which will prevent the
potential race condition on the list and its corruption during a second
probe when probe fails.

Fixes: 26634c4b1868 ("rtlwifi Modify existing bits to match vendor version 2013.02.07")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/net/wireless/realtek/rtlwifi/base.c |  7 ----
 drivers/net/wireless/realtek/rtlwifi/base.h |  1 -
 drivers/net/wireless/realtek/rtlwifi/pci.c  | 44 ---------------------
 drivers/net/wireless/realtek/rtlwifi/wifi.h | 12 ------
 4 files changed, 64 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
index aab4605de9c4..fd28c7a722d8 100644
--- a/drivers/net/wireless/realtek/rtlwifi/base.c
+++ b/drivers/net/wireless/realtek/rtlwifi/base.c
@@ -2696,9 +2696,6 @@ MODULE_AUTHOR("Larry Finger	<Larry.FInger@lwfinger.net>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Realtek 802.11n PCI wireless core");
 
-struct rtl_global_var rtl_global_var = {};
-EXPORT_SYMBOL_GPL(rtl_global_var);
-
 static int __init rtl_core_module_init(void)
 {
 	BUILD_BUG_ON(TX_PWR_BY_RATE_NUM_RATE < TX_PWR_BY_RATE_NUM_SECTION);
@@ -2712,10 +2709,6 @@ static int __init rtl_core_module_init(void)
 	/* add debugfs */
 	rtl_debugfs_add_topdir();
 
-	/* init some global vars */
-	INIT_LIST_HEAD(&rtl_global_var.glb_priv_list);
-	spin_lock_init(&rtl_global_var.glb_list_lock);
-
 	return 0;
 }
 
diff --git a/drivers/net/wireless/realtek/rtlwifi/base.h b/drivers/net/wireless/realtek/rtlwifi/base.h
index f081a9a90563..f3a6a43a42ec 100644
--- a/drivers/net/wireless/realtek/rtlwifi/base.h
+++ b/drivers/net/wireless/realtek/rtlwifi/base.h
@@ -124,7 +124,6 @@ int rtl_send_smps_action(struct ieee80211_hw *hw,
 u8 *rtl_find_ie(u8 *data, unsigned int len, u8 ie);
 void rtl_recognize_peer(struct ieee80211_hw *hw, u8 *data, unsigned int len);
 u8 rtl_tid_to_ac(u8 tid);
-extern struct rtl_global_var rtl_global_var;
 void rtl_phy_scan_operation_backup(struct ieee80211_hw *hw, u8 operation);
 
 #endif
diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index 40fc3c297a8a..4388066eb9e2 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -295,46 +295,6 @@ static bool rtl_pci_get_amd_l1_patch(struct ieee80211_hw *hw)
 	return status;
 }
 
-static bool rtl_pci_check_buddy_priv(struct ieee80211_hw *hw,
-				     struct rtl_priv **buddy_priv)
-{
-	struct rtl_priv *rtlpriv = rtl_priv(hw);
-	struct rtl_pci_priv *pcipriv = rtl_pcipriv(hw);
-	struct rtl_priv *tpriv = NULL, *iter;
-	struct rtl_pci_priv *tpcipriv = NULL;
-
-	if (!list_empty(&rtlpriv->glb_var->glb_priv_list)) {
-		list_for_each_entry(iter, &rtlpriv->glb_var->glb_priv_list,
-				    list) {
-			tpcipriv = (struct rtl_pci_priv *)iter->priv;
-			rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
-				"pcipriv->ndis_adapter.funcnumber %x\n",
-				pcipriv->ndis_adapter.funcnumber);
-			rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
-				"tpcipriv->ndis_adapter.funcnumber %x\n",
-				tpcipriv->ndis_adapter.funcnumber);
-
-			if (pcipriv->ndis_adapter.busnumber ==
-			    tpcipriv->ndis_adapter.busnumber &&
-			    pcipriv->ndis_adapter.devnumber ==
-			    tpcipriv->ndis_adapter.devnumber &&
-			    pcipriv->ndis_adapter.funcnumber !=
-			    tpcipriv->ndis_adapter.funcnumber) {
-				tpriv = iter;
-				break;
-			}
-		}
-	}
-
-	rtl_dbg(rtlpriv, COMP_INIT, DBG_LOUD,
-		"find_buddy_priv %d\n", tpriv != NULL);
-
-	if (tpriv)
-		*buddy_priv = tpriv;
-
-	return tpriv != NULL;
-}
-
 static void rtl_pci_parse_configuration(struct pci_dev *pdev,
 					struct ieee80211_hw *hw)
 {
@@ -2011,7 +1971,6 @@ static bool _rtl_pci_find_adapter(struct pci_dev *pdev,
 		pcipriv->ndis_adapter.amd_l1_patch);
 
 	rtl_pci_parse_configuration(pdev, hw);
-	list_add_tail(&rtlpriv->list, &rtlpriv->glb_var->glb_priv_list);
 
 	return true;
 }
@@ -2158,7 +2117,6 @@ int rtl_pci_probe(struct pci_dev *pdev,
 	rtlpriv->rtlhal.interface = INTF_PCI;
 	rtlpriv->cfg = (struct rtl_hal_cfg *)(id->driver_data);
 	rtlpriv->intf_ops = &rtl_pci_ops;
-	rtlpriv->glb_var = &rtl_global_var;
 	rtl_efuse_ops_init(hw);
 
 	/* MEM map */
@@ -2316,7 +2274,6 @@ void rtl_pci_disconnect(struct pci_dev *pdev)
 	if (rtlpci->using_msi)
 		pci_disable_msi(rtlpci->pdev);
 
-	list_del(&rtlpriv->list);
 	if (rtlpriv->io.pci_mem_start != 0) {
 		pci_iounmap(pdev, (void __iomem *)rtlpriv->io.pci_mem_start);
 		pci_release_regions(pdev);
@@ -2375,7 +2332,6 @@ EXPORT_SYMBOL(rtl_pci_resume);
 const struct rtl_intf_ops rtl_pci_ops = {
 	.adapter_start = rtl_pci_start,
 	.adapter_stop = rtl_pci_stop,
-	.check_buddy_priv = rtl_pci_check_buddy_priv,
 	.adapter_tx = rtl_pci_tx,
 	.flush = rtl_pci_flush,
 	.reset_trx_ring = rtl_pci_reset_trx_ring,
diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h
index ae6e351bc83c..f1830ddcdd8c 100644
--- a/drivers/net/wireless/realtek/rtlwifi/wifi.h
+++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h
@@ -2270,8 +2270,6 @@ struct rtl_intf_ops {
 	/*com */
 	int (*adapter_start)(struct ieee80211_hw *hw);
 	void (*adapter_stop)(struct ieee80211_hw *hw);
-	bool (*check_buddy_priv)(struct ieee80211_hw *hw,
-				 struct rtl_priv **buddy_priv);
 
 	int (*adapter_tx)(struct ieee80211_hw *hw,
 			  struct ieee80211_sta *sta,
@@ -2514,14 +2512,6 @@ struct dig_t {
 	u32 rssi_max;
 };
 
-struct rtl_global_var {
-	/* from this list we can get
-	 * other adapter's rtl_priv
-	 */
-	struct list_head glb_priv_list;
-	spinlock_t glb_list_lock;
-};
-
 #define IN_4WAY_TIMEOUT_TIME	(30 * MSEC_PER_SEC)	/* 30 seconds */
 
 struct rtl_btc_info {
@@ -2667,9 +2657,7 @@ struct rtl_scan_list {
 struct rtl_priv {
 	struct ieee80211_hw *hw;
 	struct completion firmware_loading_complete;
-	struct list_head list;
 	struct rtl_priv *buddy_priv;
-	struct rtl_global_var *glb_var;
 	struct rtl_dmsp_ctl dmsp_ctl;
 	struct rtl_locks locks;
 	struct rtl_works works;
-- 
2.34.1

[PATCH 2/4] wifi: rltwifi: destroy workqueue at rtl_deinit_core

[Thadeu Lima de Souza Cascardo]

rtl_wq is allocated at rtl_init_core, so it makes more sense to destroy it
at rtl_deinit_core. In the case of USB, where _rtl_usb_init does not
require anything to be undone, that is fine. But for PCI, rtl_pci_init,
which is called after rtl_init_core, needs to deallocate data, but only if
it has been called.

That means that destroying the workqueue needs to be done whether
rtl_pci_init has been called or not. And since rtl_pci_deinit was doing it,
it has to be moved out of there.

It makes more sense to move it to rtl_deinit_core and have it done in both
cases, USB and PCI.

Since this is a requirement for a followup memory leak fix, mark this as
fixing such memory leak.

Fixes: 0c8173385e54 ("rtl8192ce: Add new driver")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/net/wireless/realtek/rtlwifi/base.c | 5 +++++
 drivers/net/wireless/realtek/rtlwifi/pci.c  | 2 --
 drivers/net/wireless/realtek/rtlwifi/usb.c  | 5 -----
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
index fd28c7a722d8..062d5a0a4c11 100644
--- a/drivers/net/wireless/realtek/rtlwifi/base.c
+++ b/drivers/net/wireless/realtek/rtlwifi/base.c
@@ -575,9 +575,14 @@ static void rtl_free_entries_from_ack_queue(struct ieee80211_hw *hw,
 
 void rtl_deinit_core(struct ieee80211_hw *hw)
 {
+	struct rtl_priv *rtlpriv = rtl_priv(hw);
 	rtl_c2hcmd_launcher(hw, 0);
 	rtl_free_entries_from_scan_list(hw);
 	rtl_free_entries_from_ack_queue(hw, false);
+	if (rtlpriv->works.rtl_wq) {
+		destroy_workqueue(rtlpriv->works.rtl_wq);
+		rtlpriv->works.rtl_wq = NULL;
+	}
 }
 EXPORT_SYMBOL_GPL(rtl_deinit_core);
 
diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index 4388066eb9e2..e60ac910e750 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -1656,8 +1656,6 @@ static void rtl_pci_deinit(struct ieee80211_hw *hw)
 	synchronize_irq(rtlpci->pdev->irq);
 	tasklet_kill(&rtlpriv->works.irq_tasklet);
 	cancel_work_sync(&rtlpriv->works.lps_change_work);
-
-	destroy_workqueue(rtlpriv->works.rtl_wq);
 }
 
 static int rtl_pci_init(struct ieee80211_hw *hw, struct pci_dev *pdev)
diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
index 0368ecea2e81..f5718e570011 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -629,11 +629,6 @@ static void _rtl_usb_cleanup_rx(struct ieee80211_hw *hw)
 	tasklet_kill(&rtlusb->rx_work_tasklet);
 	cancel_work_sync(&rtlpriv->works.lps_change_work);
 
-	if (rtlpriv->works.rtl_wq) {
-		destroy_workqueue(rtlpriv->works.rtl_wq);
-		rtlpriv->works.rtl_wq = NULL;
-	}
-
 	skb_queue_purge(&rtlusb->rx_queue);
 
 	while ((urb = usb_get_from_anchor(&rtlusb->rx_cleanup_urbs))) {
-- 
2.34.1

[PATCH 3/4] wifi: rtlwifi: fix memory leaks and invalid access at probe error path

[Thadeu Lima de Souza Cascardo]

Deinitialize at reverse order when probe fails.

When init_sw_vars fails, rtl_deinit_core should not be called, specially
now that it destroys the rtl_wq workqueue.

And call rtl_pci_deinit and deinit_sw_vars, otherwise, memory will be
leaked.

Remove pci_set_drvdata call as it will already be cleaned up by the core
driver code and could lead to memory leaks too. cf. commit 8d450935ae7f
("wireless: rtlwifi: remove unnecessary pci_set_drvdata()") and commit
3d86b93064c7 ("rtlwifi: Fix PCI probe error path orphaned memory").

Fixes: 0c8173385e54 ("rtl8192ce: Add new driver")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/net/wireless/realtek/rtlwifi/pci.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index e60ac910e750..a870117cf12a 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -2165,7 +2165,7 @@ int rtl_pci_probe(struct pci_dev *pdev,
 	if (rtlpriv->cfg->ops->init_sw_vars(hw)) {
 		pr_err("Can't init_sw_vars\n");
 		err = -ENODEV;
-		goto fail3;
+		goto fail2;
 	}
 	rtl_init_sw_leds(hw);
 
@@ -2183,14 +2183,14 @@ int rtl_pci_probe(struct pci_dev *pdev,
 	err = rtl_pci_init(hw, pdev);
 	if (err) {
 		pr_err("Failed to init PCI\n");
-		goto fail3;
+		goto fail4;
 	}
 
 	err = ieee80211_register_hw(hw);
 	if (err) {
 		pr_err("Can't register mac80211 hw.\n");
 		err = -ENODEV;
-		goto fail3;
+		goto fail5;
 	}
 	rtlpriv->mac80211.mac80211_registered = 1;
 
@@ -2213,9 +2213,12 @@ int rtl_pci_probe(struct pci_dev *pdev,
 	set_bit(RTL_STATUS_INTERFACE_START, &rtlpriv->status);
 	return 0;
 
-fail3:
-	pci_set_drvdata(pdev, NULL);
+fail5:
+	rtl_pci_deinit(hw);
+fail4:
 	rtl_deinit_core(hw);
+fail3:
+	rtlpriv->cfg->ops->deinit_sw_vars(hw);
 
 fail2:
 	if (rtlpriv->io.pci_mem_start != 0)
-- 
2.34.1

[PATCH 4/4] wifi: rtlwifi: pci: wait for firmware loading before releasing memory

[Thadeu Lima de Souza Cascardo]

At probe error path, the firmware loading work may have already been
queued. In such a case, it will try to access memory allocated by the probe
function, which is about to be released. In such paths, wait for the
firmware worker to finish before releasing memory.

Fixes: 3d86b93064c7 ("rtlwifi: Fix PCI probe error path orphaned memory")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/net/wireless/realtek/rtlwifi/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index a870117cf12a..0eafc4d125f9 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -2218,6 +2218,7 @@ int rtl_pci_probe(struct pci_dev *pdev,
 fail4:
 	rtl_deinit_core(hw);
 fail3:
+	wait_for_completion(&rtlpriv->firmware_loading_complete);
 	rtlpriv->cfg->ops->deinit_sw_vars(hw);
 
 fail2:
-- 
2.34.1

RE: [PATCH 1/4] wifi: rtlwifi: remove unused check_buddy_priv

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> Commit 2461c7d60f9f ("rtlwifi: Update header file") introduced a global
> list of private data structures.
> 
> Later on, commit 26634c4b1868 ("rtlwifi Modify existing bits to match
> vendor version 2013.02.07") started adding the private data to that list at
> probe time and added a hook, check_buddy_priv to find the private data from
> a similar device.
> 
> However, that function was never used.
> 
> Besides, though there is a lock for that list, it is never used. And when
> the probe fails, the private data is never removed from the list. This
> would cause a second probe to access freed memory.
> 
> Remove the unused hook, structures and members, which will prevent the
> potential race condition on the list and its corruption during a second
> probe when probe fails.
> 
> Fixes: 26634c4b1868 ("rtlwifi Modify existing bits to match vendor version 2013.02.07")

This is a cleanup patch, so I don't think we need a strong Fixes tag. 

RE: [PATCH 2/4] wifi: rltwifi: destroy workqueue at rtl_deinit_core

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> 
> rtl_wq is allocated at rtl_init_core, so it makes more sense to destroy it
> at rtl_deinit_core. In the case of USB, where _rtl_usb_init does not
> require anything to be undone, that is fine. But for PCI, rtl_pci_init,
> which is called after rtl_init_core, needs to deallocate data, but only if
> it has been called.
> 
> That means that destroying the workqueue needs to be done whether
> rtl_pci_init has been called or not. And since rtl_pci_deinit was doing it,
> it has to be moved out of there.
> 
> It makes more sense to move it to rtl_deinit_core and have it done in both
> cases, USB and PCI.
> 
> Since this is a requirement for a followup memory leak fix, mark this as
> fixing such memory leak.
> 
> Fixes: 0c8173385e54 ("rtl8192ce: Add new driver")

Like patch 1/3, this is a cleanup patch, so I don't think a Fixes is needed. 

> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

"rtlwifi" is a typo in subject.

> ---
>  drivers/net/wireless/realtek/rtlwifi/base.c | 5 +++++
>  drivers/net/wireless/realtek/rtlwifi/pci.c  | 2 --
>  drivers/net/wireless/realtek/rtlwifi/usb.c  | 5 -----
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
> index fd28c7a722d8..062d5a0a4c11 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/base.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/base.c
> @@ -575,9 +575,14 @@ static void rtl_free_entries_from_ack_queue(struct ieee80211_hw *hw,
> 
>  void rtl_deinit_core(struct ieee80211_hw *hw)
>  {
> +       struct rtl_priv *rtlpriv = rtl_priv(hw);

A blank line between declarations and statements. 

>         rtl_c2hcmd_launcher(hw, 0);
>         rtl_free_entries_from_scan_list(hw);
>         rtl_free_entries_from_ack_queue(hw, false);
> +       if (rtlpriv->works.rtl_wq) {
> +               destroy_workqueue(rtlpriv->works.rtl_wq);
> +               rtlpriv->works.rtl_wq = NULL;
> +       }
>  }
>  EXPORT_SYMBOL_GPL(rtl_deinit_core);
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
> index 4388066eb9e2..e60ac910e750 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/pci.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
> @@ -1656,8 +1656,6 @@ static void rtl_pci_deinit(struct ieee80211_hw *hw)
>         synchronize_irq(rtlpci->pdev->irq);
>         tasklet_kill(&rtlpriv->works.irq_tasklet);
>         cancel_work_sync(&rtlpriv->works.lps_change_work);
> -
> -       destroy_workqueue(rtlpriv->works.rtl_wq);
>  }
> 
>  static int rtl_pci_init(struct ieee80211_hw *hw, struct pci_dev *pdev)
> diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> index 0368ecea2e81..f5718e570011 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> @@ -629,11 +629,6 @@ static void _rtl_usb_cleanup_rx(struct ieee80211_hw *hw)
>         tasklet_kill(&rtlusb->rx_work_tasklet);
>         cancel_work_sync(&rtlpriv->works.lps_change_work);
> 
> -       if (rtlpriv->works.rtl_wq) {
> -               destroy_workqueue(rtlpriv->works.rtl_wq);
> -               rtlpriv->works.rtl_wq = NULL;
> -       }
> -
>         skb_queue_purge(&rtlusb->rx_queue);
> 
>         while ((urb = usb_get_from_anchor(&rtlusb->rx_cleanup_urbs))) {
> --
> 2.34.1

RE: [PATCH 3/4] wifi: rtlwifi: fix memory leaks and invalid access at probe error path

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> Deinitialize at reverse order when probe fails.
> 
> When init_sw_vars fails, rtl_deinit_core should not be called, specially
> now that it destroys the rtl_wq workqueue.
> 
> And call rtl_pci_deinit and deinit_sw_vars, otherwise, memory will be
> leaked.
> 
> Remove pci_set_drvdata call as it will already be cleaned up by the core
> driver code and could lead to memory leaks too. cf. commit 8d450935ae7f
> ("wireless: rtlwifi: remove unnecessary pci_set_drvdata()") and commit
> 3d86b93064c7 ("rtlwifi: Fix PCI probe error path orphaned memory").
> 
> Fixes: 0c8173385e54 ("rtl8192ce: Add new driver")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

RE: [PATCH 4/4] wifi: rtlwifi: pci: wait for firmware loading before releasing memory

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> 
> At probe error path, the firmware loading work may have already been
> queued. In such a case, it will try to access memory allocated by the probe
> function, which is about to be released. In such paths, wait for the
> firmware worker to finish before releasing memory.
> 
> Fixes: 3d86b93064c7 ("rtlwifi: Fix PCI probe error path orphaned memory")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

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

[Ping-Ke Shih]

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. 

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

[Thadeu Lima de Souza Cascardo]

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.

Re: [PATCH 1/4] wifi: rtlwifi: remove unused check_buddy_priv

[Thadeu Lima de Souza Cascardo]

On Wed, Nov 27, 2024 at 05:32:41AM +0000, Ping-Ke Shih wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> > Commit 2461c7d60f9f ("rtlwifi: Update header file") introduced a global
> > list of private data structures.
> > 
> > Later on, commit 26634c4b1868 ("rtlwifi Modify existing bits to match
> > vendor version 2013.02.07") started adding the private data to that list at
> > probe time and added a hook, check_buddy_priv to find the private data from
> > a similar device.
> > 
> > However, that function was never used.
> > 
> > Besides, though there is a lock for that list, it is never used. And when
> > the probe fails, the private data is never removed from the list. This
> > would cause a second probe to access freed memory.
> > 
> > Remove the unused hook, structures and members, which will prevent the
> > potential race condition on the list and its corruption during a second
> > probe when probe fails.
> > 
> > Fixes: 26634c4b1868 ("rtlwifi Modify existing bits to match vendor version 2013.02.07")
> 
> This is a cleanup patch, so I don't think we need a strong Fixes tag. 
> 
> 

Well, there is a real bug here. Since the private data is not removed in
the probe error path, a second probe leads to the corruption of the list.
But since that list is not used for anything useful (the check_buddy_priv
is removed as part of this patch as it was never used), instead of adding
the list removal in the error path, we simply remove the entire list.

Thanks.
Cascardo.

Re: [PATCH 2/4] wifi: rltwifi: destroy workqueue at rtl_deinit_core

[Thadeu Lima de Souza Cascardo]

On Wed, Nov 27, 2024 at 05:35:23AM +0000, Ping-Ke Shih wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> > 
> > rtl_wq is allocated at rtl_init_core, so it makes more sense to destroy it
> > at rtl_deinit_core. In the case of USB, where _rtl_usb_init does not
> > require anything to be undone, that is fine. But for PCI, rtl_pci_init,
> > which is called after rtl_init_core, needs to deallocate data, but only if
> > it has been called.
> > 
> > That means that destroying the workqueue needs to be done whether
> > rtl_pci_init has been called or not. And since rtl_pci_deinit was doing it,
> > it has to be moved out of there.
> > 
> > It makes more sense to move it to rtl_deinit_core and have it done in both
> > cases, USB and PCI.
> > 
> > Since this is a requirement for a followup memory leak fix, mark this as
> > fixing such memory leak.
> > 
> > Fixes: 0c8173385e54 ("rtl8192ce: Add new driver")
> 
> Like patch 1/3, this is a cleanup patch, so I don't think a Fixes is needed. 
> 

This is a pre-requisite for patch 3/4. Without it, the workqueue
destruction in the PCI probe error path would not happen when it should,
resulting in memory leaks and NULL pointer dereferences.

> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> 
> "rtlwifi" is a typo in subject.
> 

Can you fix that when applying, or should I send a v2?

> > ---
> >  drivers/net/wireless/realtek/rtlwifi/base.c | 5 +++++
> >  drivers/net/wireless/realtek/rtlwifi/pci.c  | 2 --
> >  drivers/net/wireless/realtek/rtlwifi/usb.c  | 5 -----
> >  3 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
> > index fd28c7a722d8..062d5a0a4c11 100644
> > --- a/drivers/net/wireless/realtek/rtlwifi/base.c
> > +++ b/drivers/net/wireless/realtek/rtlwifi/base.c
> > @@ -575,9 +575,14 @@ static void rtl_free_entries_from_ack_queue(struct ieee80211_hw *hw,
> > 
> >  void rtl_deinit_core(struct ieee80211_hw *hw)
> >  {
> > +       struct rtl_priv *rtlpriv = rtl_priv(hw);
> 
> A blank line between declarations and statements. 
> 

Same here.

> >         rtl_c2hcmd_launcher(hw, 0);
> >         rtl_free_entries_from_scan_list(hw);
> >         rtl_free_entries_from_ack_queue(hw, false);
> > +       if (rtlpriv->works.rtl_wq) {
> > +               destroy_workqueue(rtlpriv->works.rtl_wq);
> > +               rtlpriv->works.rtl_wq = NULL;
> > +       }
> >  }
> >  EXPORT_SYMBOL_GPL(rtl_deinit_core);
> > 
> > diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
> > index 4388066eb9e2..e60ac910e750 100644
> > --- a/drivers/net/wireless/realtek/rtlwifi/pci.c
> > +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
> > @@ -1656,8 +1656,6 @@ static void rtl_pci_deinit(struct ieee80211_hw *hw)
> >         synchronize_irq(rtlpci->pdev->irq);
> >         tasklet_kill(&rtlpriv->works.irq_tasklet);
> >         cancel_work_sync(&rtlpriv->works.lps_change_work);
> > -
> > -       destroy_workqueue(rtlpriv->works.rtl_wq);
> >  }
> > 
> >  static int rtl_pci_init(struct ieee80211_hw *hw, struct pci_dev *pdev)
> > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > index 0368ecea2e81..f5718e570011 100644
> > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > @@ -629,11 +629,6 @@ static void _rtl_usb_cleanup_rx(struct ieee80211_hw *hw)
> >         tasklet_kill(&rtlusb->rx_work_tasklet);
> >         cancel_work_sync(&rtlpriv->works.lps_change_work);
> > 
> > -       if (rtlpriv->works.rtl_wq) {
> > -               destroy_workqueue(rtlpriv->works.rtl_wq);
> > -               rtlpriv->works.rtl_wq = NULL;
> > -       }
> > -
> >         skb_queue_purge(&rtlusb->rx_queue);
> > 
> >         while ((urb = usb_get_from_anchor(&rtlusb->rx_cleanup_urbs))) {
> > --
> > 2.34.1
> 

RE: [PATCH 2/4] wifi: rltwifi: destroy workqueue at rtl_deinit_core

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> On Wed, Nov 27, 2024 at 05:35:23AM +0000, Ping-Ke Shih wrote:
> > Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> > > index fd28c7a722d8..062d5a0a4c11 100644
> > > --- a/drivers/net/wireless/realtek/rtlwifi/base.c
> > > +++ b/drivers/net/wireless/realtek/rtlwifi/base.c
> > > @@ -575,9 +575,14 @@ static void rtl_free_entries_from_ack_queue(struct ieee80211_hw *hw,
> > >
> > >  void rtl_deinit_core(struct ieee80211_hw *hw)
> > >  {
> > > +       struct rtl_priv *rtlpriv = rtl_priv(hw);
> >
> > A blank line between declarations and statements.
> >
> 
> Same here.

Prefer you send v2 to prevent I mess up the code.