Re: [PATCH net-next 02/11] 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] After this patch, the comment in virtio_can_start_xmit()
  (drivers/net/can/virtio_can.c) still says "can_put_echo_skb() already…

Pre-existing issues:
- [Medium] At this commit, can_put_echo_skb() in
  drivers/net/can/dev/skb.c handles skb ownership differently depending…

--

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

This patch replaces kfree_skb() with dev_kfree_skb_any() on the drop
paths of can_put_echo_skb(), can_dropped_invalid_skb(),
can_dev_dropped_skb() and can_create_echo_skb(). After a successful clone,
can_create_echo_skb() now releases the original skb with
dev_consume_skb_any() instead of consume_skb(). With these changes the
helpers are safe to call with hardware interrupts disabled.

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

> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index bc0787535b524..46c151eece8f7 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -63,7 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	    (skb->protocol != htons(ETH_P_CAN) &&
>  	     skb->protocol != htons(ETH_P_CANFD) &&
>  	     skb->protocol != htons(ETH_P_CANXL))) {
> -		kfree_skb(skb);
> +		dev_kfree_skb_any(skb);
>  		return 0;
>  	}
>  
> @@ -91,7 +91,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	} else {
>  		/* locking problem with netif_stop_queue() ?? */
>  		netdev_err(dev, "%s: BUG! echo_skb %d is occupied!\n", __func__, idx);
> -		kfree_skb(skb);
> +		dev_kfree_skb_any(skb);
>  		return -EBUSY;
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. Since
these error paths in can_put_echo_skb() are being changed anyway, should
the out-of-bounds index path free the skb as well?

At this commit that path returns without freeing or storing the skb:

drivers/net/can/dev/skb.c:can_put_echo_skb() {
	if (idx >= priv->echo_skb_max) {
		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
			   __func__, idx, priv->echo_skb_max);
		return -EINVAL;
	}
    ...
}

The -EBUSY, -ENOMEM and IFF_ECHO/protocol drop paths all free the skb,
and callers depend on that. For example, virtio_can_start_xmit() does:

	err = can_put_echo_skb(skb, dev, can_tx_msg->putidx, 0);
	if (unlikely(err)) {
		/* skb was already freed by can_put_echo_skb() on error */
		...
		goto kick;
	}

Won't the skb leak if a driver passes idx >= priv->echo_skb_max?

The -EINVAL branch comes from commit 6411959c10fe. A later patch in this
series, "can: dev: can_put_echo_skb(): free skb on invalid echo index",
adds dev_kfree_skb_any(skb) before the return -EINVAL. By the end of the
series, every return path consumes the skb.

[ ... ]

> 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 the comment in virtio_can_start_xmit() still names
the old helper:

drivers/net/can/virtio_can.c:virtio_can_start_xmit() {
    ...
		/*
		 * 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.
		 */
    ...
}

What the comment says about ownership is still correct.
dev_consume_skb_any() gives up the skb whether it frees it directly or
defers the free. Only the helper name is out of date.

The commit message FAQ says this comment will be updated in a separate
series to make backporting easier, and it is still unchanged at the end of
this series. Should it say dev_consume_skb_any() instead?

>  	return nskb;
>  }

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