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

From: Florian Fainelli <f.fainelli@gmail.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>, netdev@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Andrew Lunn <andrew@lunn.ch>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 9/9] net: dsa: tag_ocelot: call only the relevant portion of __skb_vlan_pop() on TX
Date: Thu, 23 Mar 2023 09:34:00 -0700	[thread overview]
Message-ID: <1dab843b-268f-7c7c-6049-8f0f6d4c920a@gmail.com> (raw)
In-Reply-To: <20230322233823.1806736-10-vladimir.oltean@nxp.com>

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