2022-02-25 13:53:52

by Hangyu Hua

[permalink] [raw]
Subject: [PATCH] can: usb: fix a possible memory leak in esd_usb2_start_xmit

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 <[email protected]>
---
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
--
2.25.1


2022-02-26 02:03:00

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH] can: usb: fix a possible memory leak in esd_usb2_start_xmit

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 <[email protected]>
> ---
> 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

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Attachments:
(No filename) (1.21 kB)
 signature.asc (499.00 B)
 Download all attachments

2022-02-28 08:14:03

by Hangyu Hua

[permalink] [raw]
Subject: Re: [PATCH] can: usb: fix a possible memory leak in esd_usb2_start_xmit

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 <[email protected]>
>> ---
>> 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
>

2022-02-28 10:58:33

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH] can: usb: fix a possible memory leak in esd_usb2_start_xmit

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

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Attachments:
(No filename) (900.00 B)
 signature.asc (499.00 B)
 Download all attachments

2022-02-28 11:00:23

by Hangyu Hua

[permalink] [raw]
Subject: Re: [PATCH] can: usb: fix a possible memory leak in esd_usb2_start_xmit

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
>