diff mbox series

[v2] rtw88: Fix some memory leaks

Message ID 20210620194110.7520-1-Larry.Finger@lwfinger.net
State New
Headers show
Series [v2] rtw88: Fix some memory leaks | expand

Commit Message

Larry Finger June 20, 2021, 7:41 p.m. UTC 
There are memory leaks of skb's from routines rtw_fw_c2h_cmd_rx_irqsafe()
and rtw_coex_info_response(), both arising from C2H operations. There are
no leaks from the buffers sent to mac80211.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Stable <stable@vger.kernel.org>
---
v2 - add the missinf changelog.

---
 drivers/net/wireless/realtek/rtw88/coex.c | 4 +++-
 drivers/net/wireless/realtek/rtw88/fw.c   | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Ping-Ke Shih June 21, 2021, 10:40 a.m. UTC | #1 
> -----Original Message-----

> From: Larry Finger [mailto:larry.finger@gmail.com] On Behalf Of Larry Finger

> Sent: Monday, June 21, 2021 3:41 AM

> To: kvalo@codeaurora.org

> Cc: linux-wireless@vger.kernel.org; Larry Finger; Stable

> Subject: [PATCH v2] rtw88: Fix some memory leaks

> 

> There are memory leaks of skb's from routines rtw_fw_c2h_cmd_rx_irqsafe()

> and rtw_coex_info_response(), both arising from C2H operations. There are

> no leaks from the buffers sent to mac80211.

> 

> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>

> Cc: Stable <stable@vger.kernel.org>

> ---

> v2 - add the missinf changelog.

> 

> ---

>  drivers/net/wireless/realtek/rtw88/coex.c | 4 +++-

>  drivers/net/wireless/realtek/rtw88/fw.c   | 2 ++

>  2 files changed, 5 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/net/wireless/realtek/rtw88/coex.c b/drivers/net/wireless/realtek/rtw88/coex.c

> index cedbf3825848..e81bf5070183 100644

> --- a/drivers/net/wireless/realtek/rtw88/coex.c

> +++ b/drivers/net/wireless/realtek/rtw88/coex.c

> @@ -591,8 +591,10 @@ void rtw_coex_info_response(struct rtw_dev *rtwdev, struct sk_buff *skb)

>  	struct rtw_coex *coex = &rtwdev->coex;

>  	u8 *payload = get_payload_from_coex_resp(skb);

> 

> -	if (payload[0] != COEX_RESP_ACK_BY_WL_FW)

> +	if (payload[0] != COEX_RESP_ACK_BY_WL_FW) {

> +		dev_kfree_skb_any(skb);

>  		return;

> +	}

> 

>  	skb_queue_tail(&coex->queue, skb);

>  	wake_up(&coex->wait);

> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c

> index 797b08b2a494..43525ad8543f 100644

> --- a/drivers/net/wireless/realtek/rtw88/fw.c

> +++ b/drivers/net/wireless/realtek/rtw88/fw.c

> @@ -231,9 +231,11 @@ void rtw_fw_c2h_cmd_rx_irqsafe(struct rtw_dev *rtwdev, u32 pkt_offset,

>  	switch (c2h->id) {

>  	case C2H_BT_MP_INFO:

>  		rtw_coex_info_response(rtwdev, skb);

> +		dev_kfree_skb_any(skb);


The rtw_coex_info_response() puts skb into a skb_queue, so we can't free it here.
Instead, we should free it after we dequeue and do thing. 
So, we send another patch: 
https://lore.kernel.org/linux-wireless/20210621103023.41928-1-pkshih@realtek.com/T/#u

I hope this isn't confusing you.


>  		break;

>  	case C2H_WLAN_RFON:

>  		complete(&rtwdev->lps_leave_check);

> +		dev_kfree_skb_any(skb);

>  		break;

>  	default:

>  		/* pass offset for further operation */

> --

> 2.32.0


--
Ping-Ke
Larry Finger June 21, 2021, 3:52 p.m. UTC | #2 
On 6/21/21 5:40 AM, Pkshih wrote:
> 

> 

>> -----Original Message-----

>> From: Larry Finger [mailto:larry.finger@gmail.com] On Behalf Of Larry Finger

>> Sent: Monday, June 21, 2021 3:41 AM

>> To: kvalo@codeaurora.org

>> Cc: linux-wireless@vger.kernel.org; Larry Finger; Stable

>> Subject: [PATCH v2] rtw88: Fix some memory leaks

>>

>> There are memory leaks of skb's from routines rtw_fw_c2h_cmd_rx_irqsafe()

>> and rtw_coex_info_response(), both arising from C2H operations. There are

>> no leaks from the buffers sent to mac80211.

>>

>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>

>> Cc: Stable <stable@vger.kernel.org>

>> ---

>> v2 - add the missinf changelog.

>>

>> ---

>>   drivers/net/wireless/realtek/rtw88/coex.c | 4 +++-

>>   drivers/net/wireless/realtek/rtw88/fw.c   | 2 ++

>>   2 files changed, 5 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/net/wireless/realtek/rtw88/coex.c b/drivers/net/wireless/realtek/rtw88/coex.c

>> index cedbf3825848..e81bf5070183 100644

>> --- a/drivers/net/wireless/realtek/rtw88/coex.c

>> +++ b/drivers/net/wireless/realtek/rtw88/coex.c

>> @@ -591,8 +591,10 @@ void rtw_coex_info_response(struct rtw_dev *rtwdev, struct sk_buff *skb)

>>   	struct rtw_coex *coex = &rtwdev->coex;

>>   	u8 *payload = get_payload_from_coex_resp(skb);

>>

>> -	if (payload[0] != COEX_RESP_ACK_BY_WL_FW)

>> +	if (payload[0] != COEX_RESP_ACK_BY_WL_FW) {

>> +		dev_kfree_skb_any(skb);

>>   		return;

>> +	}

>>

>>   	skb_queue_tail(&coex->queue, skb);

>>   	wake_up(&coex->wait);

>> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c

>> index 797b08b2a494..43525ad8543f 100644

>> --- a/drivers/net/wireless/realtek/rtw88/fw.c

>> +++ b/drivers/net/wireless/realtek/rtw88/fw.c

>> @@ -231,9 +231,11 @@ void rtw_fw_c2h_cmd_rx_irqsafe(struct rtw_dev *rtwdev, u32 pkt_offset,

>>   	switch (c2h->id) {

>>   	case C2H_BT_MP_INFO:

>>   		rtw_coex_info_response(rtwdev, skb);

>> +		dev_kfree_skb_any(skb);

> 

> The rtw_coex_info_response() puts skb into a skb_queue, so we can't free it here.

> Instead, we should free it after we dequeue and do thing.

> So, we send another patch:

> https://lore.kernel.org/linux-wireless/20210621103023.41928-1-pkshih@realtek.com/T/#u

> 

> I hope this isn't confusing you.


No, it is not confusing. T=You fixed some leaks the I did not know existed.

Kalle: Take P-K's patch and discard mine.

Thanks,

Larry
Kalle Valo June 22, 2021, 6:26 a.m. UTC | #3 
Larry Finger <Larry.Finger@lwfinger.net> writes:

> On 6/21/21 5:40 AM, Pkshih wrote:

>>

>>

>>> -----Original Message-----

>>> From: Larry Finger [mailto:larry.finger@gmail.com] On Behalf Of Larry Finger

>>> Sent: Monday, June 21, 2021 3:41 AM

>>> To: kvalo@codeaurora.org

>>> Cc: linux-wireless@vger.kernel.org; Larry Finger; Stable

>>> Subject: [PATCH v2] rtw88: Fix some memory leaks

>>>

>>> There are memory leaks of skb's from routines rtw_fw_c2h_cmd_rx_irqsafe()

>>> and rtw_coex_info_response(), both arising from C2H operations. There are

>>> no leaks from the buffers sent to mac80211.

>>>

>>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>

>>> Cc: Stable <stable@vger.kernel.org>

>>> ---

>>> v2 - add the missinf changelog.

>>>

>>> ---

>>>   drivers/net/wireless/realtek/rtw88/coex.c | 4 +++-

>>>   drivers/net/wireless/realtek/rtw88/fw.c   | 2 ++

>>>   2 files changed, 5 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/drivers/net/wireless/realtek/rtw88/coex.c b/drivers/net/wireless/realtek/rtw88/coex.c

>>> index cedbf3825848..e81bf5070183 100644

>>> --- a/drivers/net/wireless/realtek/rtw88/coex.c

>>> +++ b/drivers/net/wireless/realtek/rtw88/coex.c

>>> @@ -591,8 +591,10 @@ void rtw_coex_info_response(struct rtw_dev *rtwdev, struct sk_buff *skb)

>>>   	struct rtw_coex *coex = &rtwdev->coex;

>>>   	u8 *payload = get_payload_from_coex_resp(skb);

>>>

>>> -	if (payload[0] != COEX_RESP_ACK_BY_WL_FW)

>>> +	if (payload[0] != COEX_RESP_ACK_BY_WL_FW) {

>>> +		dev_kfree_skb_any(skb);

>>>   		return;

>>> +	}

>>>

>>>   	skb_queue_tail(&coex->queue, skb);

>>>   	wake_up(&coex->wait);

>>> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c

>>> index 797b08b2a494..43525ad8543f 100644

>>> --- a/drivers/net/wireless/realtek/rtw88/fw.c

>>> +++ b/drivers/net/wireless/realtek/rtw88/fw.c

>>> @@ -231,9 +231,11 @@ void rtw_fw_c2h_cmd_rx_irqsafe(struct rtw_dev *rtwdev, u32 pkt_offset,

>>>   	switch (c2h->id) {

>>>   	case C2H_BT_MP_INFO:

>>>   		rtw_coex_info_response(rtwdev, skb);

>>> +		dev_kfree_skb_any(skb);

>>

>> The rtw_coex_info_response() puts skb into a skb_queue, so we can't free it here.

>> Instead, we should free it after we dequeue and do thing.

>> So, we send another patch:

>> https://lore.kernel.org/linux-wireless/20210621103023.41928-1-pkshih@realtek.com/T/#u

>>

>> I hope this isn't confusing you.

>

> No, it is not confusing. T=You fixed some leaks the I did not know existed.

>

> Kalle: Take P-K's patch and discard mine.


Will do, thank you for clear instructions :)

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtw88/coex.c b/drivers/net/wireless/realtek/rtw88/coex.c
index cedbf3825848..e81bf5070183 100644
--- a/drivers/net/wireless/realtek/rtw88/coex.c
+++ b/drivers/net/wireless/realtek/rtw88/coex.c
@@ -591,8 +591,10 @@  void rtw_coex_info_response(struct rtw_dev *rtwdev, struct sk_buff *skb)
 	struct rtw_coex *coex = &rtwdev->coex;
 	u8 *payload = get_payload_from_coex_resp(skb);
 
-	if (payload[0] != COEX_RESP_ACK_BY_WL_FW)
+	if (payload[0] != COEX_RESP_ACK_BY_WL_FW) {
+		dev_kfree_skb_any(skb);
 		return;
+	}
 
 	skb_queue_tail(&coex->queue, skb);
 	wake_up(&coex->wait);
diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
index 797b08b2a494..43525ad8543f 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -231,9 +231,11 @@  void rtw_fw_c2h_cmd_rx_irqsafe(struct rtw_dev *rtwdev, u32 pkt_offset,
 	switch (c2h->id) {
 	case C2H_BT_MP_INFO:
 		rtw_coex_info_response(rtwdev, skb);
+		dev_kfree_skb_any(skb);
 		break;
 	case C2H_WLAN_RFON:
 		complete(&rtwdev->lps_leave_check);
+		dev_kfree_skb_any(skb);
 		break;
 	default:
 		/* pass offset for further operation */