Re: [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications

From: Vladimir Oltean <olteanv@gmail.com>
To: Tobias Waldekranz <tobias@waldekranz.com>
Cc: davem@davemloft.net, kuba@kernel.org, andrew@lunn.ch,
	vivien.didelot@gmail.com, f.fainelli@gmail.com, roopa@nvidia.com,
	nikolay@nvidia.com, netdev@vger.kernel.org, jiri@resnulli.us,
	idosch@idosch.org, stephen@networkplumber.org
Subject: Re: [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications
Date: Mon, 18 Jan 2021 23:03:40 +0200	[thread overview]
Message-ID: <20210118210340.5nlr4bq32nssvhvz@skbuf> (raw)
In-Reply-To: <87o8hmj8w0.fsf@waldekranz.com>

On Mon, Jan 18, 2021 at 09:19:11PM +0100, Tobias Waldekranz wrote:
> > My only hope is that we could just offload the entries pointing towards
> > br0, and ignore the local ones.
> 
> That was my initial approach. Unfortunately that breaks down when the
> bridge inherits its address from a port, i.e. the default case.
> 
> When the address is added to the bridge (fdb->dst == NULL), fdb_insert
> will find the previous local entry that is set on the port and bail out
> before sending a notification:
> 
> 	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);
> 	}
> 
> You could change this so that a notification always is sent out. Or you
> could give precedence to !fdb->dst and update the existing entry.

I'm afraid my competence ends here.
IMO the problem is really the struct net_bridge_port *source argument of
fdb_insert. The behavior we want is that all is_local FDB entries are
coming from br0, and none from the brports (aka source == NULL, so the
callers that had something non-NULL for source should be deleted).
"You can't always get what you want" though.

> > But for that I would need the bridge maintainers to clarify what is
> > the difference between then, as I asked in your other patch.
> 
> I am pretty sure they mean the same thing, I believe that !fdb->dst
> implies is_local. It is just that "bridge fdb add ADDR dev br0 self" is
> a new(er) thing, and before that there was "local" entries on ports.
> Maybe I should try to get rid of the local flag in the bridge first, and
> then come back to this problem once that is done? Either way, I agree
> that 5/7 is all we want to add to DSA to get this working.

Please expand on what you plan to do. The is_local bit is part of the
bridge UAPI, how do you plan to get rid of it?