Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Tobias Waldekranz <tobias@waldekranz.com>
To:     DENG Qingfang <dqfext@gmail.com>, Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] net: dsa: mv88e6xxx: override existent unicast portvec in port_fdb_add
In-Reply-To: <20210130134334.10243-1-dqfext@gmail.com>
References: <20210130134334.10243-1-dqfext@gmail.com>
Date:   Sat, 30 Jan 2021 21:47:02 +0100
Message-ID: <87eei25f09.fsf@waldekranz.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Sat, Jan 30, 2021 at 21:43, DENG Qingfang <dqfext@gmail.com> wrote:
> Having multiple destination ports for a unicast address does not make
> sense.
> Make port_db_load_purge override existent unicast portvec instead of
> adding a new port bit.

Is this the layer we want to solve this problem at? What are the
contents of the software FDB at this stage?

Here is a quick example I tried on one of my systems:

root@envoy:~# bridge fdb add 02:00:de:ad:00:01 dev eth1 static vlan 1
root@envoy:~# bridge fdb add 02:00:de:ad:00:01 dev eth2 static vlan 1
root@envoy:~# bridge fdb | grep de:ad
02:00:de:ad:00:01 dev eth2 vlan 1 self static
02:00:de:ad:00:01 dev eth1 vlan 1 self static

Why does the second add operation succeed? Am I missing some magic flag?
Presumably the bridge will only ever forward packets to which ever entry
ends up being first in the relevant hash list. Is that not the real
problem here?

As it stands today, those commands will result in the following ATU
config (eth1/2 being mapped to port 10/9):

root@envoy:~# mvls atu
ADDRESS             FID  STATE      Q  F  0  1  2  3  4  5  6  7  8  9  a
ff:ff:ff:ff:ff:ff     0  static     -  -  0  1  2  3  4  5  6  7  8  9  a
02:00:de:ad:00:01     1  static     -  -  .  .  .  .  .  .  .  .  .  9  a
ff:ff:ff:ff:ff:ff     1  static     -  -  0  1  2  3  4  5  6  7  8  9  a

One might argue that this is no more wrong than what would have been set
up with this patch applied. The problem is that the bridge allows this
configuration in the first place.