Date:   Mon, 18 Nov 2019 13:01:32 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH net-next] net: dsa: tag_8021q: Allow DSA tags and VLAN
 filtering simultaneously

On Mon, 18 Nov 2019 at 06:30, Florian Fainelli <f.fainelli@...il.com> wrote:
>
>
>
> On 11/17/2019 1:14 PM, Vladimir Oltean wrote:
> > There are very good reasons to want this (see documentation reference to
> > br_if.c), and there are also very good reasons for not enabling it by
> > default. So a devlink param named best_effort_vlan_filtering, currently
> > driver-specific and exported only by sja1105, is used to configure this.
> >
> > In practice, this is perhaps the way that most users are going to use
> > the switch in. Best-effort untagged traffic can be bridged with any net
> > device in the system or terminated locally, and VLAN-tagged streams are
> > forwarded autonomously in a time-sensitive manner according to their
> > PCP (they need not transit the CPU). For those cases where the CPU needs
> > to terminate some VLAN-tagged traffic, using the DSA master is still an
> > option.
> >
> > A complication while implementing this was the fact that
> > __netif_receive_skb_core calls __vlan_hwaccel_clear_tag right before
> > passing the skb to the DSA packet_type handler. This means that the
> > tagger does not see the VLAN tag in the skb, nor in the skb meta data.
> > The patch that starting zeroing the skb VLAN tag is:
> >
> >   commit d4b812dea4a236f729526facf97df1a9d18e191c
> >   Author: Eric Dumazet <edumazet@...gle.com>
> >   Date:   Thu Jul 18 07:19:26 2013 -0700
> >
> >       vlan: mask vlan prio bits
> >
> >       In commit 48cc32d38a52d0b68f91a171a8d00531edc6a46e
> >       ("vlan: don't deliver frames for unknown vlans to protocols")
> >       Florian made sure we set pkt_type to PACKET_OTHERHOST
> >       if the vlan id is set and we could find a vlan device for this
> >       particular id.
> >
> >       But we also have a problem if prio bits are set.
> >
> >       Steinar reported an issue on a router receiving IPv6 frames with a
> >       vlan tag of 4000 (id 0, prio 2), and tunneled into a sit device,
> >       because skb->vlan_tci is set.
> >
> >       Forwarded frame is completely corrupted : We can see (8100:4000)
> >       being inserted in the middle of IPv6 source address :
> >
> >       16:48:00.780413 IP6 2001:16d8:8100:4000:ee1c:0:9d9:bc87 >
> >       9f94:4d95:2001:67c:29f4::: ICMP6, unknown icmp6 type (0), length 64
> >              0x0000:  0000 0029 8000 c7c3 7103 0001 a0ae e651
> >              0x0010:  0000 0000 ccce 0b00 0000 0000 1011 1213
> >              0x0020:  1415 1617 1819 1a1b 1c1d 1e1f 2021 2223
> >              0x0030:  2425 2627 2829 2a2b 2c2d 2e2f 3031 3233
> >
> >       It seems we are not really ready to properly cope with this right now.
> >
> >       We can probably do better in future kernels :
> >       vlan_get_ingress_priority() should be a netdev property instead of
> >       a per vlan_dev one.
> >
> >       For stable kernels, lets clear vlan_tci to fix the bugs.
> >
> >       Reported-by: Steinar H. Gunderson <sesse@...gle.com>
> >       Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> >       Signed-off-by: David S. Miller <davem@...emloft.net>
> >
> > The patch doesn't say why "we are not really ready to properly cope with
> > this right now", and hence why the best solution is to remove the VLAN
> > tag from skb's that don't have a local VLAN sub-interface interested in
> > them. And I have no idea either.
> >
> > But the above patch has a loophole: if the VLAN tag is not
> > hw-accelerated, it isn't removed from the skb if there is no VLAN
> > sub-interface interested in it (our case). So we are hooking into the
> > .ndo_fix_features callback of the DSA master and clearing the rxvlan
> > offload feature, so the DSA tagger will always see the VLAN as part of
> > the skb data. This is symmetrical with the ETH_P_DSA_8021Q case and does
> > not need special treatment in the tagger. But perhaps the workaround is
> > brittle and might break if not understood well enough.
> >
> > The disabling of the rxvlan feature of the DSA master is unconditional.
> > The reasoning is that at first sight, no DSA master with regular frame
> > parsing abilities could be able to locate the VLAN tag with any of the
> > existing taggers anyway, and therefore, adding a property in dsa_switch
> > to control the rxvlan feature of the master would seem like useless
> > boilerplate.
> >
> > Signed-off-by: Vladimir Oltean <olteanv@...il.com>
> > ---
>
> [snip]
>
> > +best_effort_vlan_filtering
> > +                     [DEVICE, DRIVER-SPECIFIC]
> > +                     Allow plain ETH_P_8021Q headers to be used as DSA tags.
> > +                     Benefits:
> > +                     - Can terminate untagged traffic over switch net
> > +                       devices even when enslaved to a bridge with
> > +                       vlan_filtering=1.
> > +                     - Can do QoS based on VLAN PCP for autonomously
> > +                       forwarded frames.
> > +                     Drawbacks:
> > +                     - User cannot change pvid via 'bridge' commands. This
> > +                       would break source port identification on RX for
> > +                       untagged traffic.
> > +                     - User cannot use VLANs in range 1024-3071. If the
> > +                       switch receives frames with such VIDs, it will
> > +                       misinterpret them as DSA tags.
> > +                     - Cannot terminate VLAN-tagged traffic on local device.
> > +                       There is no way to deduce the source port from these.
> > +                       One could still use the DSA master though.
>
> Could we use QinQ to possibly solve these problems and would that work
> for your switch? I do not really mind being restricted to not being able
> to change the default_pvid or have a reduced VLAN range, but being able
> to test VLAN tags terminated on DSA slave network devices is a valuable
> thing to do.
> --
> Florian

Hi Florian,

Unfortunately the sja1105 supports only detecting double-tagged
frames. It doesn't have the concept of pushing/popping an S-tag on top
of frames detected as having a C-tag. A workaround would be to lie to
it about the C-tag EtherType, so it will push an S-tag unconditionally
(thinking VLAN-tagged frames are untagged, therefore pushing the
802.1ad pvid), but then you lose VLAN filtering based on the C-tag. To
be fair I don't completely understand the Linux abstraction of a
bridge with vlan_protocol 802.1ad either. It seems to me like such a
bridge would ignore the C-tag and treat those frames as untagged,
basically operating only at the S-tag level very similar to the
workaround I would have to do in sja1105. But a correct implementation
would have to do VLAN filtering based on the C-tag, and _in addition_
push an 802.1ad unique pvid (C-tag).
But I digress. The sja1105 doesn't support stacked VLANs so that is
part of the reason why I made dsa_8021q more flexible (just functions
that can be called from other drivers/taggers) and less tied to
sja1105. I would be happy to see this tagger getting used with stacked
VLANs as long as the hardware permits it.

Hope this helps,
-Vladimir

Powered by blists - more mailing lists