Re: [PATCH RFC net 0/2] net: dsa: felix: fix VLAN-unaware reception

[Vladimir Oltean]

On Mon, Dec 16, 2024 at 11:10:05AM +0100, Robert Hodaszi wrote:
> On Sunday, 15.12.2024 at 18:09 +0100, Vladimir Oltean <vladimir.oltean@xxxxxxx> wrote:
> > 
> > Give me an example traffic pattern, Linux configuration and corruption,
> > please. I spent a lot of time trying to make sure I am not introducing
> > regressions, and I have no idea what you are seeing that is wrong.
> > Please don't try to make assumptions, just let me see what you see.
> 
> The config I'm using:
>  - Using the 2.5Gbps as CPU port in 'ocelot-8021q' mode, Linux interface name is 'eth0'
>  - Using 2 downstream ports as external Ethernet ports: 'eth1' and 'eth2'
>  - 'eth1' port of the device is directly connected with my PC (Ethernet interface #1, 192.168.1.1)
>  - 'eth2' port of the device is directly connected with my PC (Ethernet interface #2, 192.168.2.1)
> 
> DTS:
> 
>   &mscc_felix_port0 {
>     label = "eth1";
>     managed = "in-band-status";
>     phy-handle = <&qsgmii_phy0>;
>     phy-mode = "qsgmii";
>     status = "okay";
>   };
> 
>   &mscc_felix_port1 {
>     label = "eth2";
>     managed = "in-band-status";
>     phy-handle = <&qsgmii_phy1>;
>     phy-mode = "qsgmii";
>     status = "okay";
>   };
> 
>   &mscc_felix_port4 {
>     ethernet = <&enetc_port2>;
>     status = "okay";
>     dsa-tag-protocol = "ocelot-8021q";
>   };
> 
> LS1028 unit's Linux config:
> 
>   # Static IP to 'eth1'
>   $ ifconfig eth1 192.168.1.2 up
> 
>   # Create a VLAN-unaware bridge, and add 'eth2' to that
>   $ brctl addbr br0
>   $ brctl addif br0 eth2
> 
>   # Set static IP to the bridge
>   $ ifconfig br0 192.168.2.2 up
>   $ ifconfig eth2 up
> 
> Now at this point:
> 
>   1. I can ping perfectly fine the eth1 interface from my PC ("ping 192.168.1.2"), and vice-versa
>   2. Pinging 'br0' from my PC is not working ("ping 192.168.2.2"). I can see the ARP requests, but there are not ARP replies at all.
> 
> If I enable VLAN-filtering on 'br0', it starts working:
> 
>   $ echo 1 > /sys/class/net/br0/bridge/vlan_filtering
> 
> 
> So basically:
> 
>   1. Raw interface -> working
>   2. VLAN-aware bridge -> working
>   3. VLAN-unaware bridge -> NOT working
> 
> I traced what is happening. When VLAN-filtering is not enabled on the bridge, LS1028's switch is configured with 'push_inner_tag = OCELOT_NO_ES0_TAG'. But ds->untag_vlan_aware_bridge_pvid is always set to true at switch setup, in felix_tag_8021q_setup(). That makes dsa_switch_rcv() call dsa_software_vlan_untag() for each packets.
> 
> 
> Now in dsa_software_vlan_untag(), if the port is not part of the bridge (case #1), it returns with the skb early. That's OK.
> 
> 
>   static inline struct sk_buff *dsa_software_vlan_untag(struct sk_buff *skb)
>   {
>     struct dsa_port *dp = dsa_user_to_port(skb->dev);
>     struct net_device *br = dsa_port_bridge_dev_get(dp);
>     u16 vid;
> 
>     /* software untagging for standalone ports not yet necessary */
>     if (!br)
>       return skb;
> 
> 
> But if port is part of a bridge, no matter "push_inner_tag" is set as OCELOT_ES0_TAG or OCELOT_NO_ES0_TAG, it always untags it:
> 
>     /* Move VLAN tag from data to hwaccel */
>     if (!skb_vlan_tag_present(skb)) {
>       skb = skb_vlan_untag(skb);
>       if (!skb)
>         return NULL;
>     }
> 
> As the "untag_vlan_aware_bridge_pvid" is a switch-specific thing, not port-specific, I cannot change it to false/true depending on the port is added to a VLAN-unaware/aware bridge, as the other port may be added to another bridge (eth1 -> VLAN-aware (tags enabled), eth2 -> VLAN-unaware (tags disabled)).
> 
> Also, in the past this code part looked like this:
> 
>     /* Move VLAN tag from data to hwaccel */
>     if (!skb_vlan_tag_present(skb) && skb->protocol == htons(proto)) {
>       skb = skb_vlan_untag(skb);
>       if (!skb)
>         return NULL;
>     }
> 
> So we had a protocol check. This wouldn't work 100% neither, because what if a VLAN packet arrives from the outer world into a VLAN-unaware bridge? I assume, that shouldn't be untagged, still, it would do that.
> 
> 
> I'm not that happy with my patch though, as I had to add another flag for each ports. But that seems to be the "cleanest" solution. That's why as marked it as RFC.
> 
> Thanks,
> Robert

The memory is starting to come back :-|

Ok, so the good news is that you aren't seeing things, I can reproduce
with tools/testing/selftests/drivers/net/dsa/local_termination.sh.

Another good thing is that the fix is easier than your posted attempt.
You've correctly identified the previous VLAN stripping logic, and that
is what we should go forward with. I don't agree with your analysis that
it wouldn't work, because if you look at the implementation of
skb_vlan_untag(), it strips the VLAN header from the skb head, but still
keeps it in the hwaccel area, so packets are still VLAN-tagged.

This does not have a functional impact upon reception, it is just done
to have unified handling later on in the function:
skb_vlan_tag_present() and skb_vlan_tag_get_id(). This side effect is
also mentioned as a comment on dsa_software_vlan_untag().

The stripping itself will only take place in dsa_software_untag_vlan_unaware_bridge()
if the switch driver sets dp->ds->untag_bridge_pvid. The felix driver
does not set this.

What is not so good is that I'm seriously starting to doubt my sanity.
You'd think that I ran the selftests that I had posted together with the
patch introducing the bug, but somehow they fail :-| And not only that,
but thoughts about this problem itself have since passed through my head,
and I failed to correctly identify where the problem applies and where
it does not. I'm sorry for that.

I've just posted a fix to this bug, which I would like you to double-check
and respond with review and test tags, or let me know if it doesn't work.
https://lore.kernel.org/netdev/20241216135059.1258266-1-vladimir.oltean@xxxxxxx/
I posted it myself because I don't expect you to have the full context
(it's a bug that I introduced), and with yours there are still a lot of
unanswered "why"s, as well as not the simplest solution.