* [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
@ 2024-10-14 14:51 Wang Hai
2024-10-14 16:59 ` Florian Fainelli
2024-10-15 23:53 ` Florian Fainelli
0 siblings, 2 replies; 9+ messages in thread
From: Wang Hai @ 2024-10-14 14:51 UTC (permalink / raw)
To: florian.fainelli, bcm-kernel-feedback-list, davem, edumazet, kuba,
pabeni, zhangxiaoxu5
Cc: netdev, linux-kernel, wanghai38
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
2024-10-14 14:51 [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit() Wang Hai
@ 2024-10-14 16:59 ` Florian Fainelli
2024-10-15 14:54 ` Wang Hai
2024-10-15 18:01 ` Jakub Kicinski
2024-10-15 23:53 ` Florian Fainelli
1 sibling, 2 replies; 9+ messages in thread
From: Florian Fainelli @ 2024-10-14 16:59 UTC (permalink / raw)
To: Wang Hai, bcm-kernel-feedback-list, davem, edumazet, kuba, pabeni,
zhangxiaoxu5
Cc: netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
2024-10-14 16:59 ` Florian Fainelli
@ 2024-10-15 14:54 ` Wang Hai
2024-10-15 18:01 ` Jakub Kicinski
1 sibling, 0 replies; 9+ messages in thread
From: Wang Hai @ 2024-10-15 14:54 UTC (permalink / raw)
To: Florian Fainelli, bcm-kernel-feedback-list, davem, edumazet, kuba,
pabeni, zhangxiaoxu5
Cc: netdev, linux-kernel
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()
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
2024-10-14 16:59 ` Florian Fainelli
2024-10-15 14:54 ` Wang Hai
@ 2024-10-15 18:01 ` Jakub Kicinski
2024-10-15 18:07 ` Florian Fainelli
1 sibling, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2024-10-15 18:01 UTC (permalink / raw)
To: Florian Fainelli
Cc: Wang Hai, bcm-kernel-feedback-list, davem, edumazet, pabeni,
zhangxiaoxu5, netdev, linux-kernel
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..
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
2024-10-15 18:01 ` Jakub Kicinski
@ 2024-10-15 18:07 ` Florian Fainelli
2024-10-15 19:54 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2024-10-15 18:07 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Wang Hai, bcm-kernel-feedback-list, davem, edumazet, pabeni,
zhangxiaoxu5, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
2024-10-15 18:07 ` Florian Fainelli
@ 2024-10-15 19:54 ` Jakub Kicinski
2024-10-15 23:55 ` Florian Fainelli
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2024-10-15 19:54 UTC (permalink / raw)
To: Florian Fainelli
Cc: Wang Hai, bcm-kernel-feedback-list, davem, edumazet, pabeni,
zhangxiaoxu5, netdev, linux-kernel
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
2024-10-15 19:54 ` Jakub Kicinski
@ 2024-10-15 23:55 ` Florian Fainelli
2024-10-16 0:00 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2024-10-15 23:55 UTC (permalink / raw)
To: Jakub Kicinski, Florian Fainelli
Cc: Wang Hai, bcm-kernel-feedback-list, davem, edumazet, pabeni,
zhangxiaoxu5, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
2024-10-15 23:55 ` Florian Fainelli
@ 2024-10-16 0:00 ` Jakub Kicinski
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2024-10-16 0:00 UTC (permalink / raw)
To: Florian Fainelli
Cc: Wang Hai, bcm-kernel-feedback-list, davem, edumazet, pabeni,
zhangxiaoxu5, netdev, linux-kernel
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..
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit()
2024-10-14 14:51 [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit() Wang Hai
2024-10-14 16:59 ` Florian Fainelli
@ 2024-10-15 23:53 ` Florian Fainelli
1 sibling, 0 replies; 9+ messages in thread
From: Florian Fainelli @ 2024-10-15 23:53 UTC (permalink / raw)
To: Wang Hai, florian.fainelli, bcm-kernel-feedback-list, davem,
edumazet, kuba, pabeni, zhangxiaoxu5
Cc: netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-16 0:00 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-14 14:51 [PATCH net] net: systemport: fix potential memory leak in bcm_sysport_xmit() Wang Hai
2024-10-14 16:59 ` Florian Fainelli
2024-10-15 14:54 ` Wang Hai
2024-10-15 18:01 ` Jakub Kicinski
2024-10-15 18:07 ` Florian Fainelli
2024-10-15 19:54 ` Jakub Kicinski
2024-10-15 23:55 ` Florian Fainelli
2024-10-16 0:00 ` Jakub Kicinski
2024-10-15 23:53 ` Florian Fainelli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).