Re: [PATCH net-next 10/12] can: skb: make echo skb freeing safe in any IRQ context

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch changes can_create_echo_skb() in
  include/linux/can/skb.h to release the original skb with…

--

can: skb: make echo skb freeing safe in any IRQ context

The commit converts the drop paths in can_put_echo_skb(),
can_dropped_invalid_skb() and can_dev_dropped_skb() from kfree_skb() to
dev_kfree_skb_any(). It also makes can_create_echo_skb() release the
original skb with dev_consume_skb_any() instead of consume_skb(). This
makes the release safe when the caller has hardware interrupts disabled.

Link: https://patch.msgid.link/tencent_E84809CF236D0137885E7E4E4D58340B3208@qq.com

> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index 5d27843862fca..87ace09c0d8bd 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -78,12 +78,12 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
>  
>  	nskb = skb_clone(skb, GFP_ATOMIC);
>  	if (unlikely(!nskb)) {
> -		kfree_skb(skb);
> +		dev_kfree_skb_any(skb);
>  		return NULL;
>  	}
>  
>  	can_skb_set_owner(nskb, skb->sk);
> -	consume_skb(skb);
> +	dev_consume_skb_any(skb);

[Severity: Low]
This isn't a bug, but should the comment in virtio_can_start_xmit() in
drivers/net/can/virtio_can.c be updated as well? It still names
consume_skb():

drivers/net/can/virtio_can.c:virtio_can_start_xmit() {
    ...
	if (unlikely(err)) {
		/*
		 * can_put_echo_skb() already consumed skb via consume_skb(),
		 * so returning NETDEV_TX_BUSY would cause the stack to requeue
		 * a freed pointer. Drop the frame and return OK instead.
		 */
    ...
}

The ownership point in the comment still holds, because the original skb
is still released. After this change, though, the release goes through
dev_consume_skb_any(). That either defers the free through
dev_kfree_skb_irq_reason() or calls kfree_skb_reason(skb, SKB_CONSUMED).

>  	return nskb;
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018164417.1540449-2-mkl%40pengutronix.de