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(-)

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

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

-- 
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 |

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

-- 
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 |

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
>