Re: What to do when a bridge port gets its pvid deleted?

[Ido Schimmel <idosch@idosch.org>]

On Mon, Aug 19, 2019 at 08:15:03PM +0300, Vladimir Oltean wrote:
> On 6/28/19 7:45 PM, Florian Fainelli wrote:
> > On 6/28/19 5:37 AM, Vladimir Oltean wrote:
> > > On Fri, 28 Jun 2019 at 15:30, Ido Schimmel <idosch@idosch.org> wrote:
> > > > 
> > > > On Tue, Jun 25, 2019 at 11:49:29PM +0300, Vladimir Oltean wrote:
> > > > > A number of DSA drivers (BCM53XX, Microchip KSZ94XX, Mediatek MT7530
> > > > > at the very least), as well as Mellanox Spectrum (I didn't look at all
> > > > > the pure switchdev drivers) try to restore the pvid to a default value
> > > > > on .port_vlan_del.
> > > > 
> > > > I don't know about DSA drivers, but that's not what mlxsw is doing. If
> > > > the VLAN that is configured as PVID is deleted from the bridge port, the
> > > > driver instructs the port to discard untagged and prio-tagged packets.
> > > > This is consistent with the bridge driver's behavior.
> > > > 
> > > > We do have a flow the "restores" the PVID, but that's when a port is
> > > > unlinked from its bridge master. The PVID we set is 4095 which cannot be
> > > > configured by the 8021q / bridge driver. This is due to the way the
> > > > underlying hardware works. Even if a port is not bridged and used purely
> > > > for routing, packets still do L2 lookup first which sends them directly
> > > > to the router block. If PVID is not configured, untagged packets could
> > > > not be routed. Obviously, at egress we strip this VLAN.
> > > > 
> > > > > Sure, the port stops receiving traffic when its pvid is a VLAN ID that
> > > > > is not installed in its hw filter, but as far as the bridge core is
> > > > > concerned, this is to be expected:
> > > > > 
> > > > > # bridge vlan add dev swp2 vid 100 pvid untagged
> > > > > # bridge vlan
> > > > > port    vlan ids
> > > > > swp5     1 PVID Egress Untagged
> > > > > 
> > > > > swp2     1 Egress Untagged
> > > > >           100 PVID Egress Untagged
> > > > > 
> > > > > swp3     1 PVID Egress Untagged
> > > > > 
> > > > > swp4     1 PVID Egress Untagged
> > > > > 
> > > > > br0      1 PVID Egress Untagged
> > > > > # ping 10.0.0.1
> > > > > PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.
> > > > > 64 bytes from 10.0.0.1: icmp_seq=1 ttl=64 time=0.682 ms
> > > > > 64 bytes from 10.0.0.1: icmp_seq=2 ttl=64 time=0.299 ms
> > > > > 64 bytes from 10.0.0.1: icmp_seq=3 ttl=64 time=0.251 ms
> > > > > 64 bytes from 10.0.0.1: icmp_seq=4 ttl=64 time=0.324 ms
> > > > > 64 bytes from 10.0.0.1: icmp_seq=5 ttl=64 time=0.257 ms
> > > > > ^C
> > > > > --- 10.0.0.1 ping statistics ---
> > > > > 5 packets transmitted, 5 received, 0% packet loss, time 4188ms
> > > > > rtt min/avg/max/mdev = 0.251/0.362/0.682/0.163 ms
> > > > > # bridge vlan del dev swp2 vid 100
> > > > > # bridge vlan
> > > > > port    vlan ids
> > > > > swp5     1 PVID Egress Untagged
> > > > > 
> > > > > swp2     1 Egress Untagged
> > > > > 
> > > > > swp3     1 PVID Egress Untagged
> > > > > 
> > > > > swp4     1 PVID Egress Untagged
> > > > > 
> > > > > br0      1 PVID Egress Untagged
> > > > > 
> > > > > # ping 10.0.0.1
> > > > > PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.
> > > > > ^C
> > > > > --- 10.0.0.1 ping statistics ---
> > > > > 8 packets transmitted, 0 received, 100% packet loss, time 7267ms
> > > > > 
> > > > > What is the consensus here? Is there a reason why the bridge driver
> > > > > doesn't take care of this?
> > > > 
> > > > Take care of what? :) Always maintaining a PVID on the bridge port? It's
> > > > completely OK not to have a PVID.
> > > > 
> > > 
> > > Yes, I didn't think it through during the first email. I came to the
> > > same conclusion in the second one.
> > > 
> > > > > Do switchdev drivers have to restore the pvid to always be
> > > > > operational, even if their state becomes inconsistent with the upper
> > > > > dev? Is it just 'nice to have'? What if VID 1 isn't in the hw filter
> > > > > either (perfectly legal)?
> > > > 
> > > > Are you saying that DSA drivers always maintain a PVID on the bridge
> > > > port and allow untagged traffic to ingress regardless of the bridge
> > > > driver's configuration? If so, I think this needs to be fixed.
> > > 
> > > Well, not at the DSA core level.
> > > But for Microchip:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/dsa/microchip/ksz9477.c#n576
> > > For Broadcom:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/dsa/b53/b53_common.c#n1376
> > > For Mediatek:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/dsa/mt7530.c#n1196
> > > 
> > > There might be others as well.
> > 
> > That sounds bogus indeed, and I bet that the two other drivers just
> > followed the b53 driver there. I will have to test this again and come
> > up with a patch eventually.
> > 
> > When the port leaves the bridge we do bring it back into a default PVID
> > (which is different than the bridge's default PVID) so that part should
> > be okay.
> > 
> 
> Adding a few more networking people.
> So my flow is something like this:
> - Boot a board with a DSA switch
> - Bring all interfaces up
> - Enslave all interfaces to br0
> - Enable vlan_filtering on br0
> 
> What VIDs should be installed into the ports' hw filters, and what should
> the pvid be at this point?
> Should the switch ports pass any traffic?
> At this point, 'bridge vlan' shows a confusing:
> port    vlan ids
> eth0     1 PVID Egress Untagged
> 
> swp5     1 PVID Egress Untagged
> 
> swp2     1 PVID Egress Untagged
> 
> swp3     1 PVID Egress Untagged
> 
> swp4     1 PVID Egress Untagged
> 
> br0      1 PVID Egress Untagged
> for all ports, but the .port_vlan_add callback is nowhere to be found.

The bridge adds a PVID on the port when it is enslaved to the bridge.
The configuration only takes effect when VLAN filtering is enabled. I'm
looking at dsa_port_vlan_add() and it seems that it does not propagate
the VLAN call when VLAN filtering is disabled. This explains why you
never see the callback.

I assume that if you configure the bridge with VLAN filtering enabled
and then enslave a port, then everything works fine.

mlxsw avoids the situation by forbidding the toggling of VLAN filtering
on the bridge when its ports are enslaved.

> 
> Whose responsibility is it for the switch to pass traffic without any
> further 'bridge vlan' command? What is the mechanism through which this
> should work?
> 
> What if I do:
> sudo bridge vlan add vid 100 dev swp2 pvid untagged
> echo 0 | sudo tee /sys/class/net/br0/bridge/vlan_filtering
> echo 1 | sudo tee /sys/class/net/br0/bridge/vlan_filtering
> What pvid should there be on swp2 now?
> 'bridge vlan' shows:
> port    vlan ids
> eth0     1 PVID Egress Untagged
> 
> swp5     1 PVID Egress Untagged
> 
> swp2     1 Egress Untagged
>          100 PVID Egress Untagged
> 
> swp3     1 PVID Egress Untagged
> 
> swp4     1 PVID Egress Untagged
> 
> br0      1 PVID Egress Untagged
> If the 'bridge vlan' output is correct, whose responsibility is it to
> restore this pvid?

I suggest to follow mlxsw and avoid this mess. You can support both VLAN
filtering enable / disable without supporting dynamically toggling the
option.

> 
> More context: the sja1105 driver is somewhat similar to the mlxsw in that
> VLAN awareness cannot be truly disabled. Arid details aside, in both cases,
> achieving "VLAN-unaware"-like behavior involves manipulating the pvid in
> both cases. But it appears that the bridge core does expect:
> (1) that the driver performs a default VLAN initialization which matches its
> own, without them ever communicating. But because switchdev/DSA drivers
> start off in standalone mode, vlan_filtering=0 comes first, hence the
> non-standard pvid. Through what mechanism is the bridge-expected pvid
> supposed to get restored upon flipping vlan_filtering?
> (2) that toggling VLAN filtering off and on has no other state upon the
> underlying driver than enabling and disabling VLAN awareness. The VLAN hw
> filter table is assumed to be unchanged. Is this a correct assumption?
> 
> Thanks,
> -Vladimir