| Message ID | 20220225060019.21220-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: usb: fix a possible memory leak in esd_usb2_start_xmit | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On 25.02.2022 14:00:19, Hangyu Hua wrote: > As in case of ems_usb_start_xmit, dev_kfree_skb needs to be called when > usb_submit_urb fails to avoid possible refcount leaks. Thanks for your patch. Have you tested that there is actually a mem leak? Please have a look at the can_free_echo_skb() function that is called a few lines earlier. > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > --- > drivers/net/can/usb/esd_usb2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c > index 286daaaea0b8..7b5e6c250d00 100644 > --- a/drivers/net/can/usb/esd_usb2.c > +++ b/drivers/net/can/usb/esd_usb2.c > @@ -810,7 +810,7 @@ static netdev_tx_t esd_usb2_start_xmit(struct sk_buff *skb, > usb_unanchor_urb(urb); > > stats->tx_dropped++; > - > + dev_kfree_skb(skb); > if (err == -ENODEV) > netif_device_detach(netdev); > else regards, Marc
Hi I get it. But this means ems_usb_start_xmit have a redundant dev_kfree_skb beacause can_put_echo_skb delete original skb and can_free_echo_skb delete the cloned skb. While this code is harmless do you think we need to delete it ? Thanks. On 2022/2/25 23:56, Marc Kleine-Budde wrote: > On 25.02.2022 14:00:19, Hangyu Hua wrote: >> As in case of ems_usb_start_xmit, dev_kfree_skb needs to be called when >> usb_submit_urb fails to avoid possible refcount leaks. > > Thanks for your patch. Have you tested that there is actually a mem > leak? Please have a look at the can_free_echo_skb() function that is > called a few lines earlier. > >> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >> --- >> drivers/net/can/usb/esd_usb2.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c >> index 286daaaea0b8..7b5e6c250d00 100644 >> --- a/drivers/net/can/usb/esd_usb2.c >> +++ b/drivers/net/can/usb/esd_usb2.c >> @@ -810,7 +810,7 @@ static netdev_tx_t esd_usb2_start_xmit(struct sk_buff *skb, >> usb_unanchor_urb(urb); >> >> stats->tx_dropped++; >> - >> + dev_kfree_skb(skb); >> if (err == -ENODEV) >> netif_device_detach(netdev); >> else > > regards, > Marc >
On 28.02.2022 10:05:03, Hangyu Hua wrote: > I get it. But this means ems_usb_start_xmit have a redundant > dev_kfree_skb beacause can_put_echo_skb delete original skb and > can_free_echo_skb delete the cloned skb. While this code is harmless > do you think we need to delete it ? ACK. This dev_kfree_skb() should be deleted: | err = usb_submit_urb(urb, GFP_ATOMIC); | if (unlikely(err)) { | can_free_echo_skb(netdev, context->echo_index, NULL); | | usb_unanchor_urb(urb); | usb_free_coherent(dev->udev, size, buf, urb->transfer_dma); | dev_kfree_skb(skb); Can you create a patch? regards, Marc
Yes. I will create a patch later. Thanks. On 2022/2/28 15:51, Marc Kleine-Budde wrote: > On 28.02.2022 10:05:03, Hangyu Hua wrote: >> I get it. But this means ems_usb_start_xmit have a redundant >> dev_kfree_skb beacause can_put_echo_skb delete original skb and >> can_free_echo_skb delete the cloned skb. While this code is harmless >> do you think we need to delete it ? > > ACK. This dev_kfree_skb() should be deleted: > > | err = usb_submit_urb(urb, GFP_ATOMIC); > | if (unlikely(err)) { > | can_free_echo_skb(netdev, context->echo_index, NULL); > | > | usb_unanchor_urb(urb); > | usb_free_coherent(dev->udev, size, buf, urb->transfer_dma); > | dev_kfree_skb(skb); > > Can you create a patch? > > regards, > Marc >
diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c index 286daaaea0b8..7b5e6c250d00 100644 --- a/drivers/net/can/usb/esd_usb2.c +++ b/drivers/net/can/usb/esd_usb2.c @@ -810,7 +810,7 @@ static netdev_tx_t esd_usb2_start_xmit(struct sk_buff *skb, usb_unanchor_urb(urb); stats->tx_dropped++; - + dev_kfree_skb(skb); if (err == -ENODEV) netif_device_detach(netdev); else
As in case of ems_usb_start_xmit, dev_kfree_skb needs to be called when usb_submit_urb fails to avoid possible refcount leaks. Signed-off-by: Hangyu Hua <hbh25y@gmail.com> --- drivers/net/can/usb/esd_usb2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)