Re: [RFC PATCH v2 net-next 09/16] net: dsa: replay port and local fdb entries when joining the bridge

[Vladimir Oltean]

On Mon, Mar 22, 2021 at 06:07:51PM +0100, Tobias Waldekranz wrote:
> On Mon, Mar 22, 2021 at 18:19, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> > On Mon, Mar 22, 2021 at 04:44:41PM +0100, Tobias Waldekranz wrote:
> >> I do not know if it is a problem or not, more of an observation: This is
> >> not guaranteed to be an exact replay of the events that the bridge port
> >> (i.e. bond0 or whatever) has received since, in fdb_insert, we exit
> >> early when adding local entries if that address is already in the
> >> database.
> >> 
> >> Do we have to guard against this somehow? Or maybe we should consider
> >> the current behavior a bug and make sure to always send the event in the
> >> first place?
> >
> > I don't really understand what you're saying.
> > fdb_insert has:
> >
> > 	fdb = br_fdb_find(br, addr, vid);
> > 	if (fdb) {
> > 		/* it is okay to have multiple ports with same
> > 		 * address, just use the first one.
> > 		 */
> > 		if (test_bit(BR_FDB_LOCAL, &fdb->flags))
> > 			return 0;
> > 		br_warn(br, "adding interface %s with same address as a received packet (addr:%pM, vlan:%u)\n",
> > 		       source ? source->dev->name : br->dev->name, addr, vid);
> > 		fdb_delete(br, fdb, true);
> > 	}
> >
> > 	fdb = fdb_create(br, source, addr, vid,
> > 			 BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
> >
> > Basically, if the {addr, vid} pair already exists in the fdb, and it
> > points to a local entry, fdb_create is bypassed.
> >
> > Whereas my br_fdb_replay() function iterates over br->fdb_list, which is
> > exactly where fdb_create() also lays its eggs. That is to say, unless
> > I'm missing something, that duplicate local FDB entries that skipped the
> > fdb_create() call in fdb_insert() because they were for already-existing
> > local FDB entries will also be skipped by br_fdb_replay(), because it
> > iterates over a br->fdb_list which contains unique local addresses.
> > Where am I wrong?
> 
> No you are right. I was thinking back to my attempt of offloading local
> addresses and I distinctly remembered that local addresses could be
> added without a notification being sent.
> 
> But that is not what is happening. It is just already inserted on
> another port. So the notification would reach DSA, or not, depending on
> ordering the of events. But there will be no discrepancy between that
> and the replay.

I'm not saying that the bridge isn't broken, because it is, but for
different reasons, as explained here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210224114350.2791260-9-olteanv@xxxxxxxxx/

What I can do is I can make br_switchdev_fdb_notify() skip fdb entries
with the BR_FDB_LOCAL bit set, and target that patch against "net", with
a Fixes: tag of 6b26b51b1d13 ("net: bridge: Add support for notifying
devices about FDB add/del").
Then I can also skip the entries with BR_FDB_LOCAL from br_fdb_replay.
Then, when I return to the "RX filtering for DSA" series, I can add the
"is_local" bit to switchdev FDB objects, and make all drivers reject
"is_local" entries (which is what the linked patch does) unless more
specific treatment is applied to those (trap to CPU).
Nikolay?