Re: [PATCH net] net: parse the eighth nested VLAN tag

[netdev-bot+sashiko]

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 patch fixes the counter only when mac_len is 0 or covers the
  first VLAN header.

--

net: parse the eighth nested VLAN tag

This commit changes the parse_depth check in __vlan_get_protocol_offset()
from a pre-decrement to a post-decrement. The goal is for a packet with
eight nested VLAN tags to return the encapsulated EtherType instead of 0,
while a ninth tag is still rejected.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 48f6764117243..b10ce1d46674b 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -7703,7 +7703,7 @@ struct vlan_type_depth __vlan_get_protocol_offset(const struct sk_buff *skb,
>  
>  		vh = skb_header_pointer(skb, mac_offset + vlan_depth,
>  					sizeof(vhdr), &vhdr);
> -		if (unlikely(!vh || !--parse_depth))
> +		if (unlikely(!vh || !parse_depth--))
>  			return (struct vlan_type_depth) { 0 };

[Severity: High]
Does this allow eight nested tags for every value of skb->mac_len?

parse_depth goes down once per loop pass, not once per VLAN header that is
actually parsed. What the first pass reads depends on where the loop
starts:

net/core/skbuff.c:__vlan_get_protocol_offset() {
    ...
	if (vlan_depth) {
		if (WARN_ON_ONCE(vlan_depth < VLAN_HLEN))
			return (struct vlan_type_depth) { 0 };
		vlan_depth -= VLAN_HLEN;
	} else {
		vlan_depth = ETH_HLEN;
	}
    ...
}

With mac_len == 0 or mac_len == ETH_HLEN + VLAN_HLEN, the first pass
parses the first tag, and eight tags now work.

With mac_len == ETH_HLEN and the first tag still in-band, vlan_depth
starts at 10. The first pass reads the last two bytes of the source MAC as
the TCI and the outer TPID as h_vlan_encapsulated_proto. That only re-reads
the type that was passed in.

Eight in-band tags then take nine passes. The ninth pass sees
parse_depth == 0 and returns { 0 }, which is the result the commit message
says is fixed.

The egress path on a device without VLAN TX offload seems to produce this
state:

validate_xmit_vlan()
  __vlan_hwaccel_push_inside()
    vlan_insert_tag_set_proto()
      __vlan_insert_inner_tag()

__vlan_insert_inner_tag() pushes the hwaccel tag inline and moves
skb->mac_header, but it does not update skb->mac_len:

	skb_push(skb, VLAN_HLEN);
	...
	if (skb_mac_header_was_set(skb))
		skb->mac_header -= VLAN_HLEN;

A forwarded skb that arrived with mac_len == ETH_HLEN keeps that value. A
later vlan_get_protocol() call, such as one in a driver's xmit checksum
path, would then still get 0 for a frame with eight tags.

On the plain RX path (skb_vlan_untag() followed by skb_reset_mac_len()),
the extra pass happens to line up with the stripped hwaccel tag, so eight
tags still work there.

Could parse_depth be decremented only for passes that parse a real VLAN
header? Alternatively, the start offset could be chosen so that every pass
parses one.

If the current behaviour is kept, should the commit message and the
VLAN_MAX_DEPTH comment mention that the result depends on mac_len? The two
texts in question are "allowing eight nested tags while continuing to
reject a ninth" and "Max. number of nested VLAN tags parsed".

>  
>  		type = vh->h_vlan_encapsulated_proto;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261031135507.3970551-2-thnkslprpt%40gmail.com