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

From: Tobias Waldekranz <tobias@waldekranz.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Roopa Prabhu <roopa@nvidia.com>,
	Nikolay Aleksandrov <nikolay@nvidia.com>,
	Jiri Pirko <jiri@resnulli.us>, Ido Schimmel <idosch@idosch.org>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	UNGLinuxDriver@microchip.com, Vadym Kochan <vkochan@marvell.com>,
	Taras Chornyi <tchornyi@marvell.com>,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Ivan Vecera <ivecera@redhat.com>,
	linux-omap@vger.kernel.org,
	Vladimir Oltean <vladimir.oltean@nxp.com>
Subject: Re: [RFC PATCH v2 net-next 09/16] net: dsa: replay port and local fdb entries when joining the bridge
Date: Mon, 22 Mar 2021 18:07:51 +0100	[thread overview]
Message-ID: <87o8fbm80o.fsf@waldekranz.com> (raw)
In-Reply-To: <20210322161955.c3slrmbtofswrqiz@skbuf>

On Mon, Mar 22, 2021 at 18:19, Vladimir Oltean <olteanv@gmail.com> 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.