| Message ID | 20241014145115.44977-1-wanghai38@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: systemport: fix potential memory leak in bcm_sysport_xmit() | expand |
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.
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()
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?
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?
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?
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>
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?
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..
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; }
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(+)