Messages in this thread | | | Date | Thu, 23 Mar 2023 09:34:00 -0700 | Subject | Re: [PATCH net-next 9/9] net: dsa: tag_ocelot: call only the relevant portion of __skb_vlan_pop() on TX | From | 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@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
| |