Re: [PATCH] can: dev: can_put_echo_skb(): free skb on invalid echo index

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:

Pre-existing issues:
- [High] Context-unsafe SKB free in can_put_echo_skb()
--

--- Patch [1]: [PATCH] can: dev: can_put_echo_skb(): free skb on invalid ec=
ho index ---
commit ab12134887377b0b69b9ab5b16cf5eb0ddf3eb9e
Author: Cunhao Lu <[email protected]>

can: dev: can_put_echo_skb(): free skb on invalid echo index

This commit fixes a memory leak in can_put_echo_skb() by ensuring the skb is
freed when the echo index is out of bounds. It adds a kfree_skb() call to
the -EINVAL error path.

Cc: [email protected]

> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 95fcdc1026f80..44ebeba998370 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -54,6 +54,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_de=
vice *dev,
>  	if (idx >=3D priv->echo_skb_max) {
>  		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of b=
ounds (%u/max %u)\n",
>  			   __func__, idx, priv->echo_skb_max);
> +		kfree_skb(skb);

[Severity: High]
This is a pre-existing issue in the surrounding code, but does adding anoth=
er
kfree_skb() here introduce an unsafe free context?

Since can_put_echo_skb() is commonly called from the ndo_start_xmit path of
CAN drivers while holding an irqsave spinlock (such as in
peak_canfd_start_xmit()), hardware interrupts may be disabled.

Calling kfree_skb() with interrupts disabled can be unsafe because SKB
destructors like sock_wfree() might assume they are in process or softirq
context, potentially leading to lockups or warnings.

Would it be safer to use dev_kfree_skb_any() here, and possibly update the
other pre-existing kfree_skb() and consume_skb() calls in this function as
well?

>  		return -EINVAL;
>  	}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/tencent_944DADCC4B4=
[email protected]?part=3D1