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

[Florian Fainelli <f.fainelli@gmail.com>]

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.
-- 
Florian