Re: [PATCH net] net: limit priority VLAN untag depth

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The commit message promises a per-packet bound, but the patch
  only bounds one pass through __netif_receive_skb_core()…

--

net: limit priority VLAN untag depth

This patch adds a vlan_depth counter to the VID 0 retry branch in
__netif_receive_skb_core(). Once VLAN_MAX_DEPTH tags have been consumed,
the packet is dropped. The aim is to stop repeated skb_vlan_untag() calls
from wrapping the 16-bit network and transport header offsets.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360e..b40efea27dc29 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6198,6 +6198,8 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
>  	}
>  
>  	if (unlikely(skb_vlan_tag_present(skb)) && !netdev_uses_dsa(skb->dev)) {
> +		unsigned int vlan_depth = 1;
> +
>  check_vlan_id:
>  		if (skb_vlan_tag_get_id(skb)) {
>  			/* Vlan id is non 0 and vlan_do_receive() above couldn't
> @@ -6209,6 +6211,10 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
>  			 * 802.1Q or 802.1AD and vlan_do_receive() above could
>  			 * not find vlan dev for vlan id 0.
>  			 */
> +			if (unlikely(vlan_depth >= VLAN_MAX_DEPTH))
> +				goto drop;
> +
> +			vlan_depth++;
>  			__vlan_hwaccel_clear_tag(skb);
>  			skb = skb_vlan_untag(skb);
>  			if (unlikely(!skb))

[Severity: High]

Does this limit the number of tags removed per packet, or only per pass
through __netif_receive_skb_core()?

vlan_depth is declared inside the if block, so it goes back to 1 every
time processing jumps to another_round. The VID 0 branch itself leaves
through another_round when the inner tag matches a VLAN device:

			if (vlan_do_receive(&skb))
				/* After stripping off 802.1P header with vlan 0
				 * vlan dev is found for inner header.
				 */
				goto another_round;

A TC ingress peer redirect also restarts the round on the same skb, and
skb->data does not change:

net/core/dev.c:sch_handle_ingress() {
    ...
	__skb_push(skb, skb->mac_len);
	if (skb_do_redirect(skb) == -EAGAIN) {
		__skb_pull(skb, skb->mac_len);
		*another = true;
		break;
	}
    ...
}

With BPF_F_PEER, skb_do_redirect() calls skb_scrub_packet(), sets skb->dev
to the peer in the other netns, and returns -EAGAIN. skb_scrub_packet()
leaves vlan_all alone, and nothing in this cycle counts iterations.

Here is one example setup. A veth pair is split across two netns. Each end
has a VID 100 VLAN device and no VID 0 device. Each VLAN device has an
ingress BPF program that calls bpf_redirect_peer() on its lower veth. A
frame carrying repeated [VID 0, VID 100] tag pairs could then cycle like
this:

  vethA: VID 0 branch untags the next VID 100 tag (vlan_depth == 2)
  vlan_do_receive() finds vlan100A -> goto another_round
  top-of-round skb_vlan_untag() strips the next VID 0 tag
  TC on vlan100A peer-redirects to vethB -> another_round
  vethB: VID 0 branch starts over with vlan_depth == 1
  ...

The same loop also seems possible with VID 100 tags alone. That version
goes through the earlier vlan_do_receive() -> goto another_round before
rx_handler and never reaches the patched branch.

Each round, skb_vlan_untag() calls skb_reset_network_header():

include/linux/skbuff.h:skb_reset_network_header() {
	long offset = skb->data - skb->head;

	DEBUG_NET_WARN_ON_ONCE(offset != (typeof(skb->network_header))offset);
	skb->network_header = offset;
}

After about 16K tags the 16-bit offset wraps. That is the out-of-bounds
condition the commit message describes.

Would it work better to put the bound inside skb_vlan_untag() itself, for
example on the header offset or the headroom? Another option is a depth
count that survives another_round.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023003750.1133904-1-user148783812897%40gmail.com