Re: [PATCH net-next 9/9] net: dsa: tag_ocelot: call only the relevant portion of __skb_vlan_pop() on TX

[Florian Fainelli]

On 3/22/23 16:38, Vladimir Oltean wrote:
> ocelot_xmit_get_vlan_info() calls __skb_vlan_pop() as the most
> appropriate helper I could find which strips away a VLAN header.
> That's all I need it to do, but __skb_vlan_pop() has more logic, which
> will become incompatible with the future revert of commit 6d1ccff62780
> ("net: reset mac header in dev_start_xmit()").
>
> Namely, it performs a sanity check on skb_mac_header(), which will stop
> being set after the above revert, so it will return an error instead of
> removing the VLAN tag.
>
> ocelot_xmit_get_vlan_info() gets called in 2 circumstances:
>
> (1) the port is under a VLAN-aware bridge and the bridge sends
>      VLAN-tagged packets
>
> (2) the port is under a VLAN-aware bridge and somebody else (an 8021q
>      upper) sends VLAN-tagged packets (using a VID that isn't in the
>      bridge vlan tables)
>
> In case (1), there is actually no bug to defend against, because
> br_dev_xmit() calls skb_reset_mac_header() and things continue to work.
>
> However, in case (2), illustrated using the commands below, it can be
> seen that our intervention is needed, since __skb_vlan_pop() complains:
>
> $ ip link add br0 type bridge vlan_filtering 1 && ip link set br0 up
> $ ip link set $eth master br0 && ip link set $eth up
> $ ip link add link $eth name $eth.100 type vlan id 100 && ip link set $eth.100 up
> $ ip addr add 192.168.100.1/24 dev $eth.100
> $ # needed to work around an apparent DSA RX filtering bug
> $ ip link set $eth promisc on
>
> I could fend off the checks in __skb_vlan_pop() with some
> skb_mac_header_was_set() calls, but seeing how few callers of
> __skb_vlan_pop() there are from TX paths, that seems rather
> unproductive.
>
> As an alternative solution, extract the bare minimum logic to strip a
> VLAN header, and move it to a new helper named vlan_remove_tag(), close
> to the definition of vlan_insert_tag(). Document it appropriately and
> make ocelot_xmit_get_vlan_info() call this smaller helper instead.
>
> Seeing that it doesn't appear illegal to test skb->protocol in the TX
> path, I guess it would be a good for vlan_remove_tag() to also absorb
> the vlan_set_encap_proto() function call.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>

Reviewed-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
--
Florian