staging: rtl8712: code improvment and bug fixes

PATCH New Pavel Skripkin 1 participant 4 messages Jun 13, 2021 Staging

Share & tools

Pavel Skripkin

author

2021-06-13 22:00 UTC #0 lore raw

cover staging: rtl8712: code improvment and bug fixes

In this patch series I fixed 2 memory leaks in rtl8712 driver and made some code improvment. In the fisrt patch I removed redundant branching to improve code speed and make it more straightforward.

The folowing 2 pathes fix 2 memory leaks in this driver. The first one fixes memory leak in the error handling path in r871xu_drv_init and the second one fixes memory leak in case of fw load failure.

Pavel Skripkin (3):
staging: rtl8712: remove redundant check in r871xu_drv_init
staging: rtl8712: fix error handling in r871xu_drv_init
staging: rtl8712: fix memory leak in rtl871x_load_fw_cb

 drivers/staging/rtl8712/hal_init.c |  3 +++
 drivers/staging/rtl8712/os_intfs.c |  4 ----
 drivers/staging/rtl8712/usb_intf.c | 32 ++++++++++++++++--------------
 3 files changed, 20 insertions(+), 19 deletions(-)

Signature · 1 line

2.32.0

Pavel Skripkin

author

2021-06-13 22:00 UTC #1 lore raw

PATCH 1/3 staging: rtl8712: remove redundant check in r871xu_drv_init

padapter->dvobj_init is initialized rigth before
initialization check. There is no need for any
branching here.

Signed-off-by Pavel Skripkin <[email protected]>

---
 drivers/staging/rtl8712/usb_intf.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

drivers/staging/rtl8712/usb_intf.c +4 -6

index dc21e7743349..c482a6d1998b 100644
@@ -380,13 +380,11 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
 	/* step 3.
 	 * initialize the dvobj_priv
 	 */
-	if (!padapter->dvobj_init) {
+
+	status = padapter->dvobj_init(padapter);
+	if (status != _SUCCESS)
 		goto error;
-	} else {
-		status = padapter->dvobj_init(padapter);
-		if (status != _SUCCESS)
-			goto error;
-	}
+
 	/* step 4. */
 	status = r8712_init_drv_sw(padapter);
 	if (status)

Signature · 1 line

2.32.0

Pavel Skripkin

author

2021-06-13 22:00 UTC #2 lore raw

PATCH 2/3 staging: rtl8712: fix error handling in r871xu_drv_init

In reply to Pavel Skripkin

Previous error handling path was unique for all possible errors and there was unnecessary branching. Also, one step for freeing drv_sw was missing. All these problems was fixed by restructuring error handling path.

Also, moved out free_netdev() from r8712_free_drv_sw() for correct error handling.

Fixes 2865d42c78a9 ("staging: r8712u: Add the new driver to the mainline kernel")

Signed-off-by Pavel Skripkin <[email protected]>

---
 drivers/staging/rtl8712/os_intfs.c |  4 ----
 drivers/staging/rtl8712/usb_intf.c | 22 +++++++++++++---------
 2 files changed, 13 insertions(+), 13 deletions(-)

drivers/staging/rtl8712/os_intfs.c +0 -4

index 0c3ae8495afb..2214aca09730 100644
@@ -328,8 +328,6 @@ int r8712_init_drv_sw(struct _adapter *padapter)
 
 void r8712_free_drv_sw(struct _adapter *padapter)
 {
-	struct net_device *pnetdev = padapter->pnetdev;
-
 	r8712_free_cmd_priv(&padapter->cmdpriv);
 	r8712_free_evt_priv(&padapter->evtpriv);
 	r8712_DeInitSwLeds(padapter);

@@ -339,8 +337,6 @@ void r8712_free_drv_sw(struct _adapter *padapter)
 	_r8712_free_sta_priv(&padapter->stapriv);
 	_r8712_free_recv_priv(&padapter->recvpriv);
 	mp871xdeinit(padapter);
-	if (pnetdev)
-		free_netdev(pnetdev);
 }
 
 static void enable_video_mode(struct _adapter *padapter, int cbw40_value)

drivers/staging/rtl8712/usb_intf.c +13 -9

index c482a6d1998b..f97830049fc8 100644
@@ -361,7 +361,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
 	/* step 1. */
 	pnetdev = r8712_init_netdev();
 	if (!pnetdev)
-		goto error;
+		goto put_dev;
 	padapter = netdev_priv(pnetdev);
 	disable_ht_for_spec_devid(pdid, padapter);
 	pdvobjpriv = &padapter->dvobjpriv;

@@ -383,12 +383,12 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
 
 	status = padapter->dvobj_init(padapter);
 	if (status != _SUCCESS)
-		goto error;
+		goto free_netdev;
 
 	/* step 4. */
 	status = r8712_init_drv_sw(padapter);
 	if (status)
-		goto error;
+		goto dvobj_deinit;
 	/* step 5. read efuse/eeprom data and get mac_addr */
 	{
 		int i, offset;

@@ -568,17 +568,20 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
 	}
 	/* step 6. Load the firmware asynchronously */
 	if (rtl871x_load_fw(padapter))
-		goto error;
+		goto deinit_drv_sw;
 	spin_lock_init(&padapter->lock_rx_ff0_filter);
 	mutex_init(&padapter->mutex_start);
 	return 0;
-error:
+
+deinit_drv_sw:
+	r8712_free_drv_sw(padapter);
+dvobj_deinit:
+	padapter->dvobj_deinit(padapter);
+free_netdev:
+	free_netdev(pnetdev);
+put_dev:
 	usb_put_dev(udev);
 	usb_set_intfdata(pusb_intf, NULL);
-	if (padapter && padapter->dvobj_deinit)
-		padapter->dvobj_deinit(padapter);
-	if (pnetdev)
-		free_netdev(pnetdev);
 	return -ENODEV;
 }

@@ -610,6 +613,7 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf)
 		r8712_stop_drv_timers(padapter);
 		r871x_dev_unload(padapter);
 		r8712_free_drv_sw(padapter);
+		free_netdev(pnetdev);
 
 		/* decrease the reference count of the usb device structure
 		 * when disconnect

Signature · 1 line

2.32.0

Pavel Skripkin

author

2021-06-13 22:00 UTC #3 lore raw

PATCH 3/3 staging: rtl8712: fix memory leak in rtl871x_load_fw_cb

In reply to Pavel Skripkin

There is a leak in rtl8712 driver.
The problem was in non-freed adapter data if
firmware load failed.

This leak can be reproduced with this code:
https://syzkaller.appspot.com/text?tag=ReproC&x=16612f02d00000,
Autoload must fail (to not hit memory leak reported by syzkaller)

There are 2 possible ways how rtl871x_load_fw_cb() and r871xu_dev_remove() can be called (in case of fw load error).

1st case:

	r871xu_dev_remove() then rtl871x_load_fw_cb()

	In this case r871xu_dev_remove() will wait for
	completion and then will jump to the end, because
	rtl871x_load_fw_cb() set intfdata to NULL:

	if (pnetdev) {
		struct _adapter *padapter = netdev_priv(pnetdev);

		/* never exit with a firmware callback pending */
		wait_for_completion(&padapter->rtl8712_fw_ready);
		pnetdev = usb_get_intfdata(pusb_intf);
		usb_set_intfdata(pusb_intf, NULL);
		if (!pnetdev)
			goto firmware_load_fail;

		... clean up code here ...
	}

2nd case:
rtl871x_load_fw_cb() then r871xu_dev_remove()

In this case pnetdev (from code snippet above) will be zero (because rtl871x_load_fw_cb() set it to NULL) And clean up code won't be executed again.

So, in all cases we need to free adapted data in rtl871x_load_fw_cb(), because disconnect function cannot take care of it. And there won't be any race conditions, because complete() call happens after setting intfdata to NULL.

In previous patch I moved out free_netdev() from r8712_free_drv_sw() and that's why now it's possible to free adapter data and then call complete.

Fixes 8c213fa59199 ("staging: r8712u: Use asynchronous firmware loading")

Signed-off-by Pavel Skripkin <[email protected]>

---
 drivers/staging/rtl8712/hal_init.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/staging/rtl8712/hal_init.c +3 -0

index 715f1fe8b472..22974277afa0 100644
@@ -40,7 +40,10 @@ static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
 		dev_err(&udev->dev, "r8712u: Firmware request failed\n");
 		usb_put_dev(udev);
 		usb_set_intfdata(usb_intf, NULL);
+		r8712_free_drv_sw(adapter);
+		adapter->dvobj_deinit(adapter);
 		complete(&adapter->rtl8712_fw_ready);
+		free_netdev(adapter->pnetdev);
 		return;
 	}
 	adapter->fw = firmware;

Signature · 1 line

2.32.0