1. Patchew
  2. linux
  3. View series

[PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()

Wang Hai posted 1 patch 1 month, 1 week ago
There is a newer version of this series
drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
1 file changed, 1 insertion(+)
[PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
Posted by Wang Hai 1 month, 1 week ago 
The bcm_sysport_xmit() returns NETDEV_TX_OK without freeing skb
in case of dma_map_single() fails, add dev_kfree_skb() to fix it.

Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index c9faa8540859..0a68b526e4a8 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1359,6 +1359,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 		netif_err(priv, tx_err, dev, "DMA map failed at %p (len=%d)\n",
 			  skb->data, skb_len);
 		ret = NETDEV_TX_OK;
+		dev_kfree_skb_any(skb);
 		goto out;
 	}
 
-- 
2.17.1
Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
Posted by Florian Fainelli 1 month, 1 week ago 
On 10/14/24 07:51, Wang Hai wrote:
> The bcm_sysport_xmit() returns NETDEV_TX_OK without freeing skb
> in case of dma_map_single() fails, add dev_kfree_skb() to fix it.
> 
> Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
> Signed-off-by: Wang Hai <wanghai38@huawei.com>

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian
Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
Posted by Florian Fainelli 1 month, 1 week ago 
On 10/14/24 07:51, Wang Hai wrote:
> The bcm_sysport_xmit() returns NETDEV_TX_OK without freeing skb
> in case of dma_map_single() fails, add dev_kfree_skb() to fix it.
> 
> Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
> Signed-off-by: Wang Hai <wanghai38@huawei.com>
 > --->   drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index c9faa8540859..0a68b526e4a8 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -1359,6 +1359,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>   		netif_err(priv, tx_err, dev, "DMA map failed at %p (len=%d)\n",
>   			  skb->data, skb_len);
>   		ret = NETDEV_TX_OK;
> +		dev_kfree_skb_any(skb);

Since we already have a private counter tracking DMA mapping errors, I 
would follow what the driver does elsewhere in the transmit path, 
especially what bcm_sysport_insert_tsb() does, and just use 
dev_consume_skb_any() here.
-- 
Florian
Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
Posted by Jakub Kicinski 1 month, 1 week ago 
On Mon, 14 Oct 2024 09:59:27 -0700 Florian Fainelli wrote:
> > diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> > index c9faa8540859..0a68b526e4a8 100644
> > --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> > +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> > @@ -1359,6 +1359,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
> >   		netif_err(priv, tx_err, dev, "DMA map failed at %p (len=%d)\n",
> >   			  skb->data, skb_len);
> >   		ret = NETDEV_TX_OK;
> > +		dev_kfree_skb_any(skb);  
> 
> Since we already have a private counter tracking DMA mapping errors, I 
> would follow what the driver does elsewhere in the transmit path, 
> especially what bcm_sysport_insert_tsb() does, and just use 
> dev_consume_skb_any() here.

Are you saying that if the packet drop is accounted is some statistics
we should not inform drop monitor about it? 🤔️ 
That wasn't my understanding of kfree_skb vs consume_skb..
Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
Posted by Florian Fainelli 1 month, 1 week ago 
On 10/15/24 11:01, Jakub Kicinski wrote:
> On Mon, 14 Oct 2024 09:59:27 -0700 Florian Fainelli wrote:
>>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>>> index c9faa8540859..0a68b526e4a8 100644
>>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>>> @@ -1359,6 +1359,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>>>    		netif_err(priv, tx_err, dev, "DMA map failed at %p (len=%d)\n",
>>>    			  skb->data, skb_len);
>>>    		ret = NETDEV_TX_OK;
>>> +		dev_kfree_skb_any(skb);
>>
>> Since we already have a private counter tracking DMA mapping errors, I
>> would follow what the driver does elsewhere in the transmit path,
>> especially what bcm_sysport_insert_tsb() does, and just use
>> dev_consume_skb_any() here.
> 
> Are you saying that if the packet drop is accounted is some statistics
> we should not inform drop monitor about it? 🤔️
> That wasn't my understanding of kfree_skb vs consume_skb..

Yes that's my reasoning here, now given that we have had packet drops on 
transmit that took forever to track down, maybe I better retract that 
statement and go with v1.
-- 
Florian
Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
Posted by Jakub Kicinski 1 month, 1 week ago 
On Tue, 15 Oct 2024 11:07:29 -0700 Florian Fainelli wrote:
> >> Since we already have a private counter tracking DMA mapping errors, I
> >> would follow what the driver does elsewhere in the transmit path,
> >> especially what bcm_sysport_insert_tsb() does, and just use
> >> dev_consume_skb_any() here.  
> > 
> > Are you saying that if the packet drop is accounted is some statistics
> > we should not inform drop monitor about it? 🤔️
> > That wasn't my understanding of kfree_skb vs consume_skb..  
> 
> Yes that's my reasoning here, now given that we have had packet drops on 
> transmit that took forever to track down, maybe I better retract that 
> statement and go with v1.

Sounds good, we can apply v1. Would you like to ack/review here?
Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
Posted by Florian Fainelli 1 month, 1 week ago 
On 10/15/24 12:54, Jakub Kicinski wrote:
> On Tue, 15 Oct 2024 11:07:29 -0700 Florian Fainelli wrote:
>>>> Since we already have a private counter tracking DMA mapping errors, I
>>>> would follow what the driver does elsewhere in the transmit path,
>>>> especially what bcm_sysport_insert_tsb() does, and just use
>>>> dev_consume_skb_any() here.
>>>
>>> Are you saying that if the packet drop is accounted is some statistics
>>> we should not inform drop monitor about it? 🤔️
>>> That wasn't my understanding of kfree_skb vs consume_skb..
>>
>> Yes that's my reasoning here, now given that we have had packet drops on
>> transmit that took forever to track down, maybe I better retract that
>> statement and go with v1.
> 
> Sounds good, we can apply v1. Would you like to ack/review here?
> 

Yes, now done, thanks!
-- 
Florian
Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
Posted by Jakub Kicinski 1 month, 1 week ago 
On Tue, 15 Oct 2024 16:55:59 -0700 Florian Fainelli wrote:
> >> Yes that's my reasoning here, now given that we have had packet drops on
> >> transmit that took forever to track down, maybe I better retract that
> >> statement and go with v1.  
> > 
> > Sounds good, we can apply v1. Would you like to ack/review here?
> 
> Yes, now done, thanks!

I'm having bad luck waiting for people today :)
Pushed exactly 6min before you responded..
Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
Posted by Wang Hai 1 month, 1 week ago 
On 2024/10/15 0:59, Florian Fainelli wrote:
> On 10/14/24 07:51, Wang Hai wrote:
>> The bcm_sysport_xmit() returns NETDEV_TX_OK without freeing skb
>> in case of dma_map_single() fails, add dev_kfree_skb() to fix it.
>>
>> Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT 
>> Ethernet MAC driver")
>> Signed-off-by: Wang Hai <wanghai38@huawei.com>
> > --->   drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
>> b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index c9faa8540859..0a68b526e4a8 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -1359,6 +1359,7 @@ static netdev_tx_t bcm_sysport_xmit(struct 
>> sk_buff *skb,
>>           netif_err(priv, tx_err, dev, "DMA map failed at %p 
>> (len=%d)\n",
>>                 skb->data, skb_len);
>>           ret = NETDEV_TX_OK;
>> +        dev_kfree_skb_any(skb);
>
> Since we already have a private counter tracking DMA mapping errors, I 
> would follow what the driver does elsewhere in the transmit path, 
> especially what bcm_sysport_insert_tsb() does, and just use 
> dev_consume_skb_any() here.

Hi, Florian.

Thanks for the suggestion, I've resent the v2 version of this one as well.

[PATCH v2 net] net: systemport: fix potential memory leak in 
bcm_sysport_xmit()

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.