Skip to contentSkip to conversation

wifi: rtw88: usb: fix memory leaks on USB write failures

PATCH Superseded [email protected] 2 participants 2 messages May 1 – May 6

Subsystems (2)
Stable branch Realtek wireless driver (rtw88)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 wifi: rtw88: usb: fix memory leaks on USB write failures

From: Luka Gejak <[email protected]>

When rtw_usb_write_port() fails to submit a USB Request Block (URB) (e.g., due to device disconnect or ENOMEM), the completion callback is never executed.

Currently, the driver ignores the return value of rtw_usb_write_port() in rtw_usb_write_data() and rtw_usb_tx_agg_skb(). Because these functions rely on the completion callback to free the socket buffers (skbs) and the transaction control block (txcb), a submission failure results in:
1. A memory leak of the allocated skb in rtw_usb_write_data().
2. A memory leak of the txcb structure and all aggregated skbs in rtw_usb_tx_agg_skb().

Fix this by checking the return value of rtw_usb_write_port(). If it fails, explicitly free the skb in rtw_usb_write_data(), and properly purge the tx_ack_queue and free the txcb in rtw_usb_tx_agg_skb().

Fixes e3037485c68e ("rtw88: new Realtek 802.11ac driver")
Cc [email protected]
Tested-by Luka Gejak <[email protected]>
Signed-off-by Luka Gejak <[email protected]>
---
 drivers/net/wireless/realtek/rtw88/usb.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/net/wireless/realtek/rtw88/usb.c +10 -2
index 718940ebba31..d430645a3ef3 100644
@@ -456,7 +456,13 @@ static bool rtw_usb_tx_agg_skb(struct rtw_usb *rtwusb, struct sk_buff_head *list
 	tx_desc = (struct rtw_tx_desc *)skb_head->data;
 	qsel = le32_get_bits(tx_desc->w1, RTW_TX_DESC_W1_QSEL);
 
-	rtw_usb_write_port(rtwdev, qsel, skb_head, rtw_usb_write_port_tx_complete, txcb);
+	if (rtw_usb_write_port(rtwdev, qsel, skb_head,
+			       rtw_usb_write_port_tx_complete, txcb)) {
+		/* URB submission failed, completion won't run, free the queue */
+		skb_queue_purge(&txcb->tx_ack_queue);
+		kfree(txcb);
+		return false;
+	}
 
 	return true;
 }

@@ -518,8 +524,10 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
 
 	ret = rtw_usb_write_port(rtwdev, qsel, skb,
 				 rtw_usb_write_port_complete, skb);
-	if (unlikely(ret))
+	if (unlikely(ret)) {
 		rtw_err(rtwdev, "failed to do USB write, ret=%d\n", ret);
+		dev_kfree_skb_any(skb);
+	}
 
 	return ret;
 }
Signature · 1 line
2.54.0
maintainer
#1 lore raw
Quoted 15 lines · [email protected] <[email protected]> wrote:

From: Luka Gejak <[email protected]>

When rtw_usb_write_port() fails to submit a USB Request Block (URB) (e.g., due to device disconnect or ENOMEM), the completion callback is never executed.

Currently, the driver ignores the return value of rtw_usb_write_port() in rtw_usb_write_data() and rtw_usb_tx_agg_skb(). Because these functions rely on the completion callback to free the socket buffers (skbs) and the transaction control block (txcb), a submission failure results in:
1. A memory leak of the allocated skb in rtw_usb_write_data().
2. A memory leak of the txcb structure and all aggregated skbs in rtw_usb_tx_agg_skb().

Quoted 5 lines

Fix this by checking the return value of rtw_usb_write_port(). If it fails, explicitly free the skb in rtw_usb_write_data(), and properly purge the tx_ack_queue and free the txcb in rtw_usb_tx_agg_skb().

Fixes e3037485c68e ("rtw88: new Realtek 802.11ac driver")

This commit doesn't introduce USB support.

Quoted 2 lines
Cc [email protected]
Tested-by Luka Gejak <[email protected]>

How did you test this patch? Have you really encountered leak problem and fixed it by this patch? Or make this patch by analysis, and test it without regression?

Quoted 10 lines
Signed-off-by Luka Gejak <[email protected]>
---
 drivers/net/wireless/realtek/rtw88/usb.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/net/wireless/realtek/rtw88/usb.c +0 -0
index 718940ebba31..d430645a3ef3 100644

drivers/net/wireless/realtek/rtw88/usb.c static bool rtw_usb_tx_agg_skb(struct rtw_usb *rtwusb, struct sk_buff_head *list · line 460

Quoted 6 lines
drivers/net/wireless/realtek/rtw88/usb.c +2 -1
@@ -456,7 +456,13 @@ static bool rtw_usb_tx_agg_skb(struct rtw_usb *rtwusb, struct sk_buff_head *list
        tx_desc = (struct rtw_tx_desc *)skb_head->data;
        qsel = le32_get_bits(tx_desc->w1, RTW_TX_DESC_W1_QSEL);
 
-       rtw_usb_write_port(rtwdev, qsel, skb_head, rtw_usb_write_port_tx_complete, txcb);
+       if (rtw_usb_write_port(rtwdev, qsel, skb_head,
+                              rtw_usb_write_port_tx_complete, txcb)) {

Can we use 'ret = rtw_usb_write_port(...);' style, and check by next line? Like the other one modified by this patch.

drivers/net/wireless/realtek/rtw88/usb.c static bool rtw_usb_tx_agg_skb(struct rtw_usb *rtwusb, struct sk_buff_head *list · line 461

Quoted 1 line
drivers/net/wireless/realtek/rtw88/usb.c +1 -0
+               /* URB submission failed, completion won't run, free the queue */

I think this comment is not necessary.

drivers/net/wireless/realtek/rtw88/usb.c static bool rtw_usb_tx_agg_skb(struct rtw_usb *rtwusb, struct sk_buff_head *list · line 462

Quoted 1 line
drivers/net/wireless/realtek/rtw88/usb.c +1 -0
+               skb_queue_purge(&txcb->tx_ack_queue);

Should use ieee80211_purge_tx_queue() since this is TX?

Quoted 20 lines
drivers/net/wireless/realtek/rtw88/usb.c +6 -2
+               kfree(txcb);
+               return false;
+       }
 
        return true;
 }

@@ -518,8 +524,10 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
 
        ret = rtw_usb_write_port(rtwdev, qsel, skb,
                                 rtw_usb_write_port_complete, skb);
-       if (unlikely(ret))
+       if (unlikely(ret)) {
                rtw_err(rtwdev, "failed to do USB write, ret=%d\n", ret);
+               dev_kfree_skb_any(skb);
+       }
 
        return ret;
 }
--

2.54.0