• Vladimir Oltean's avatar
    net: dsa: felix: avoid early deletion of host FDB entries · 7e580490
    Vladimir Oltean authored
    The Felix driver declares FDB isolation but puts all standalone ports in
    VID 0. This is mostly problem-free as discussed with Alvin here:
    https://patchwork.kernel.org/project/netdevbpf/cover/20220302191417.1288145-1-vladimir.oltean@nxp.com/#24763870
    
    however there is one catch. DSA still thinks that FDB entries are
    installed on the CPU port as many times as there are user ports, and
    this is problematic when multiple user ports share the same MAC address.
    
    Consider the default case where all user ports inherit their MAC address
    from the DSA master, and then the user runs:
    
    ip link set swp0 address 00:01:02:03:04:05
    
    The above will make dsa_slave_set_mac_address() call
    dsa_port_standalone_host_fdb_add() for 00:01:02:03:04:05 in port 0's
    standalone database, and dsa_port_standalone_host_fdb_del() for the old
    address of swp0, again in swp0's standalone database.
    
    Both the ->port_fdb_add() and ->port_fdb_del() will be propagated down
    to the felix driver, which will end up deleting the old MAC address from
    the CPU port. But this is still in use by other user ports, so we end up
    breaking unicast termination for them.
    
    There isn't a problem in the fact that DSA keeps track of host
    standalone addresses in the individual database of each user port: some
    drivers like sja1105 need this. There also isn't a problem in the fact
    that some drivers choose the same VID/FID for all standalone ports.
    It is just that the deletion of these host addresses must be delayed
    until they are known to not be in use any longer, and only the driver
    has this knowledge. Since DSA keeps these addresses in &cpu_dp->fdbs and
    &cpu_db->mdbs, it is just a matter of walking over those lists and see
    whether the same MAC address is present on the CPU port in the port db
    of another user port.
    
    I have considered reusing the generic dsa_port_walk_fdbs() and
    dsa_port_walk_mdbs() schemes for this, but locking makes it difficult.
    In the ->port_fdb_add() method and co, &dp->addr_lists_lock is held, but
    dsa_port_walk_fdbs() also acquires that lock. Also, even assuming that
    we introduce an unlocked variant of the address iterator, we'd still
    need some relatively complex data structures, and a void *ctx in the
    dsa_fdb_walk_cb_t which we don't currently pass, such that drivers are
    able to figure out, after iterating, whether the same MAC address is or
    isn't present in the port db of another port.
    
    All the above, plus the fact that I expect other drivers to follow the
    same model as felix where all standalone ports use the same FID, made me
    conclude that a generic method provided by DSA is necessary:
    dsa_fdb_present_in_other_db() and the mdb equivalent. Felix calls this
    from the ->port_fdb_del() handler for the CPU port, when the database
    was classified to either a port db, or a LAG db.
    
    For symmetry, we also call this from ->port_fdb_add(), because if the
    address was installed once, then installing it a second time serves no
    purpose: it's already in hardware in VID 0 and it affects all standalone
    ports.
    
    This change moves dsa_db_equal() from switch.c to dsa.c, since it now
    has one more caller.
    
    Fixes: 54c31984 ("net: mscc: ocelot: enforce FDB isolation when VLAN-unaware")
    Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
    Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
    7e580490
switch.c 25.4 KB