Re: [PATCH net] net: dsa: mv88e6xxx: override existent unicast portvec in port_fdb_add

[Tobias Waldekranz]

On Sun, Jan 31, 2021 at 09:13, DENG Qingfang <dqfext@xxxxxxxxx> wrote:
> On Sun, Jan 31, 2021 at 8:39 AM Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
>>
>> Tobias has a point in a way too, you should get used to adding the
>> 'master static' flags to your bridge fdb commands, otherwise weird
>> things like this could happen. The faulty code can only be triggered
>> when going through dsa_legacy_fdb_add, but it is still faulty
>> nonetheless.
>
> This bug is exposed when I try your patch series on kernel 5.4
> https://lore.kernel.org/netdev/20210106095136.224739-1-olteanv@xxxxxxxxx/
> https://lore.kernel.org/netdev/20210116012515.3152-1-tobias@xxxxxxxxxxxxxx/
>
> Without this patch, DSA will add a new port bit to the existing
> portvec when a client moves to the software part of a bridge. When it
> moves away, DSA will clear the port bit but the existing one will
> remain static. This results in connection issues when the client moves
> to a different port of the switch, and the kernel log below.
>
> mv88e6085 f1072004.mdio-mii:00: ATU member violation for
> xx:xx:xx:xx:xx:xx portvec dc00 spid 0

So the bug is really that an automatically learned address is promoted
to a static entry, right?

     br0
   /  |   \
swp0 swp1 eth0

1. A station starts out connected to swp0. An ATU entry is added by the
   switch via normal SA learning.

2. The station moves to eth0.

3. DSA reacts to the event on the foreign interface, reading back the
   entry from (1), adds the CPU port and _crucially_ modifies the state
   from MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_[1-7] to static.

4. If the station now moves to swp1, you get the ATU violation.

IMO, the condition should be changed to:

    /* User-configured entries take precedence over learned entries. */
    if (is_unicast_ether_addr(addr) &&
        (entry.state >= MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_1_OLDEST) &&
        (entry.state <= MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST))

This should solve the issue discussed here, and it makes sure to keep
the ATU in sync with the FDB config, no matter how crazy the setup is.