mt76: mt7615: Fix memory leak in mt7615_mcu_wtbl_sta_add()

[PATCH net] mt76: mt7615: Fix memory leak in mt7615_mcu_wtbl_sta_add()

Posted by Zilin Guan 3 weeks, 3 days ago

In mt7615_mcu_wtbl_sta_add(), an skb sskb is allocated. If the
subsequent call to mt76_connac_mcu_alloc_wtbl_req() fails, the function
returns an error without freeing sskb, leading to a memory leak.

Fix this by calling dev_kfree_skb() on sskb in the error handling path
to ensure it is properly released.

Fixes: 99c457d902cf9 ("mt76: mt7615: move mt7615_mcu_set_bmc to mt7615_mcu_ops")
Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
 drivers/net/wireless/mediatek/mt76/mt7615/mcu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
index 4064e193d4de..08ee2e861c4e 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
@@ -874,8 +874,10 @@ mt7615_mcu_wtbl_sta_add(struct mt7615_phy *phy, struct ieee80211_vif *vif,
 	wtbl_hdr = mt76_connac_mcu_alloc_wtbl_req(&dev->mt76, &msta->wcid,
 						  WTBL_RESET_AND_SET, NULL,
 						  &wskb);
-	if (IS_ERR(wtbl_hdr))
+	if (IS_ERR(wtbl_hdr)) {
+		dev_kfree_skb(sskb);
 		return PTR_ERR(wtbl_hdr);
+	}
 
 	if (enable) {
 		mt76_connac_mcu_wtbl_generic_tlv(&dev->mt76, wskb, vif, sta,
-- 
2.34.1

Re: [PATCH net] mt76: mt7615: Fix memory leak in mt7615_mcu_wtbl_sta_add()

Posted by Lorenzo Bianconi 3 weeks, 3 days ago

> In mt7615_mcu_wtbl_sta_add(), an skb sskb is allocated. If the
> subsequent call to mt76_connac_mcu_alloc_wtbl_req() fails, the function
> returns an error without freeing sskb, leading to a memory leak.
> 
> Fix this by calling dev_kfree_skb() on sskb in the error handling path
> to ensure it is properly released.
> 
> Fixes: 99c457d902cf9 ("mt76: mt7615: move mt7615_mcu_set_bmc to mt7615_mcu_ops")
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
>  drivers/net/wireless/mediatek/mt76/mt7615/mcu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> index 4064e193d4de..08ee2e861c4e 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> @@ -874,8 +874,10 @@ mt7615_mcu_wtbl_sta_add(struct mt7615_phy *phy, struct ieee80211_vif *vif,
>  	wtbl_hdr = mt76_connac_mcu_alloc_wtbl_req(&dev->mt76, &msta->wcid,
>  						  WTBL_RESET_AND_SET, NULL,
>  						  &wskb);
> -	if (IS_ERR(wtbl_hdr))
> +	if (IS_ERR(wtbl_hdr)) {
> +		dev_kfree_skb(sskb);

Hi Zilin,

I can't see how this is useful since if mt76_connac_mcu_alloc_wtbl_req returns
an error, wskb will not be allocated.

Regards,
Lorenzo

>  		return PTR_ERR(wtbl_hdr);
> +	}
>  
>  	if (enable) {
>  		mt76_connac_mcu_wtbl_generic_tlv(&dev->mt76, wskb, vif, sta,
> -- 
> 2.34.1
>

Re: [PATCH net] mt76: mt7615: Fix memory leak in mt7615_mcu_wtbl_sta_add()

Posted by Zilin Guan 3 weeks, 3 days ago

On Thu, Nov 13, 2025 at 08:17:09AM +0100, Lorenzo Bianconi wrote:
> [-- Attachment #1: Type: text/plain, Size: 1475 bytes --]
> 
> > In mt7615_mcu_wtbl_sta_add(), an skb sskb is allocated. If the
> > subsequent call to mt76_connac_mcu_alloc_wtbl_req() fails, the function
> > returns an error without freeing sskb, leading to a memory leak.
> > 
> > Fix this by calling dev_kfree_skb() on sskb in the error handling path
> > to ensure it is properly released.
> > 
> > Fixes: 99c457d902cf9 ("mt76: mt7615: move mt7615_mcu_set_bmc to mt7615_mcu_ops")
> > Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> > ---
> >  drivers/net/wireless/mediatek/mt76/mt7615/mcu.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > index 4064e193d4de..08ee2e861c4e 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > @@ -874,8 +874,10 @@ mt7615_mcu_wtbl_sta_add(struct mt7615_phy *phy, struct ieee80211_vif *vif,
> >  	wtbl_hdr = mt76_connac_mcu_alloc_wtbl_req(&dev->mt76, &msta->wcid,
> >  						  WTBL_RESET_AND_SET, NULL,
> >  						  &wskb);
> > -	if (IS_ERR(wtbl_hdr))
> > +	if (IS_ERR(wtbl_hdr)) {
> > +		dev_kfree_skb(sskb);
> 
> Hi Zilin,
> 
> I can't see how this is useful since if mt76_connac_mcu_alloc_wtbl_req returns
> an error, wskb will not be allocated.
> 
> Regards,
> Lorenzo

Hi Lorenzo,

Thanks for your review.

You are correct that 'wskb' is not allocated in this error path. 
However, my patch is intended to free 'sskb', which was allocated 
earlier in the function. Without this change, 'sskb' is leaked if
mt76_connac_mcu_alloc_wtbl_req() fails.

This approach is similar to the error handling logic later in the
function, where a failure in sending one skb results in the other one
being freed.

Hope this clarifies.

> >  		return PTR_ERR(wtbl_hdr);
> > +	}
> >  
> >  	if (enable) {
> >  		mt76_connac_mcu_wtbl_generic_tlv(&dev->mt76, wskb, vif, sta,
> > -- 
> > 2.34.1
> > 

Best Regards,
Zilin Guan

Re: [PATCH net] mt76: mt7615: Fix memory leak in mt7615_mcu_wtbl_sta_add()

Posted by Lorenzo Bianconi 3 weeks, 3 days ago

> On Thu, Nov 13, 2025 at 08:17:09AM +0100, Lorenzo Bianconi wrote:
> > [-- Attachment #1: Type: text/plain, Size: 1475 bytes --]
> > 
> > > In mt7615_mcu_wtbl_sta_add(), an skb sskb is allocated. If the
> > > subsequent call to mt76_connac_mcu_alloc_wtbl_req() fails, the function
> > > returns an error without freeing sskb, leading to a memory leak.
> > > 
> > > Fix this by calling dev_kfree_skb() on sskb in the error handling path
> > > to ensure it is properly released.
> > > 
> > > Fixes: 99c457d902cf9 ("mt76: mt7615: move mt7615_mcu_set_bmc to mt7615_mcu_ops")
> > > Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> > > ---
> > >  drivers/net/wireless/mediatek/mt76/mt7615/mcu.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > > index 4064e193d4de..08ee2e861c4e 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > > @@ -874,8 +874,10 @@ mt7615_mcu_wtbl_sta_add(struct mt7615_phy *phy, struct ieee80211_vif *vif,
> > >  	wtbl_hdr = mt76_connac_mcu_alloc_wtbl_req(&dev->mt76, &msta->wcid,
> > >  						  WTBL_RESET_AND_SET, NULL,
> > >  						  &wskb);
> > > -	if (IS_ERR(wtbl_hdr))
> > > +	if (IS_ERR(wtbl_hdr)) {
> > > +		dev_kfree_skb(sskb);
> > 
> > Hi Zilin,
> > 
> > I can't see how this is useful since if mt76_connac_mcu_alloc_wtbl_req returns
> > an error, wskb will not be allocated.
> > 
> > Regards,
> > Lorenzo
> 
> Hi Lorenzo,
> 
> Thanks for your review.
> 
> You are correct that 'wskb' is not allocated in this error path. 
> However, my patch is intended to free 'sskb', which was allocated 
> earlier in the function. Without this change, 'sskb' is leaked if
> mt76_connac_mcu_alloc_wtbl_req() fails.
> 
> This approach is similar to the error handling logic later in the
> function, where a failure in sending one skb results in the other one
> being freed.
> 
> Hope this clarifies.

yes, right. I misread the code. I agree with the fix.

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> 
> > >  		return PTR_ERR(wtbl_hdr);
> > > +	}
> > >  
> > >  	if (enable) {
> > >  		mt76_connac_mcu_wtbl_generic_tlv(&dev->mt76, wskb, vif, sta,
> > > -- 
> > > 2.34.1
> > > 
> 
> Best Regards,
> Zilin Guan