On 18/01/2021 23:22, Nikolay Aleksandrov wrote:

On 18/01/2021 23:17, Nikolay Aleksandrov wrote:

quoted

On 18/01/2021 22:19, Tobias Waldekranz wrote:

quoted

On Mon, Jan 18, 2021 at 21:27, Vladimir Oltean [off-list ref] wrote:

quoted

On Mon, Jan 18, 2021 at 07:58:59PM +0100, Tobias Waldekranz wrote:

quoted

Ah I see, no I was not aware of that. I just saw that the entry towards
the CPU was added to the ATU, which it would in both cases. I.e. from
the switch's POV, in this setup:

   br0
   / \ (A)
swp0 dummy0
       (B)

A "local" entry like (A), or a "static" entry like (B) means the same
thing to the switch: "it is somewhere behind my CPU-port".

Yes, except that if dummy0 was a real and non-switchdev interface, then
the "local" entry would probably break your traffic if what you meant
was "static".

Agreed.

quoted

quoted

quoted

So I think there is a very real issue in that the FDB entries with the
is_local bit was never specified to switchdev thus far, and now suddenly
is. I'm sorry, but what you're saying in the commit message, that
"!added_by_user has so far been indistinguishable from is_local" is
simply false.

Alright, so how do you do it? Here is the struct:

    struct switchdev_notifier_fdb_info {
	struct switchdev_notifier_info info; /* must be first */
	const unsigned char *addr;
	u16 vid;
	u8 added_by_user:1,
	   offloaded:1;
    };

Which field separates a local address on swp0 from a dynamically learned
address on swp0?

None, that's the problem. Local addresses are already presented to
switchdev without saying that they're local. Which is the entire reason
that users are misled into thinking that the addresses are not local.

I may have misread what you said, but to me, "!added_by_user has so far
been indistinguishable from is_local" means that:
- every struct switchdev_notifier_fdb_info with added_by_user == true
  also had an implicit is_local == false
- every struct switchdev_notifier_fdb_info with added_by_user == false
  also had an implicit is_local == true
It is _this_ that I deemed as clearly untrue.

The is_local flag is not indistinguishable from !added_by_user, it is
indistinguishable full stop. Which makes it hard to work with in a
backwards-compatible way.

This was probably a semantic mistake on my part, we meant the same
thing.

quoted

quoted

Ok, so just to see if I understand this correctly:

The situation today it that `bridge fdb add ADDR dev DEV master` results
in flows towards ADDR being sent to:

1. DEV if DEV belongs to a DSA switch.
2. To the host if DEV was a non-offloaded interface.

Not quite. In the bridge software FDB, the entry is marked as is_local
in both cases, doesn't matter if the interface is offloaded or not.
Just that switchdev does not propagate the is_local flag, which makes
the switchdev listeners think it is not local. The interpretation of
that will probably vary among switchdev drivers.

The subtlety is that for a non-offloading interface, the
misconfiguration (when you mean static but use local) is easy to catch.
Since only the entry from the software FDB will be hit, this means that
the frame will never be forwarded, so traffic will break.
But in the case of a switchdev offloading interface, the frames will hit
the hardware FDB entry more often than the software FDB entry. So
everything will work just fine and dandy even though it shouldn't.

Quite right.

quoted

quoted

With this series applied both would result in (2) which, while
idiosyncratic, is as intended. But this of course runs the risk of
breaking existing scripts which rely on the current behavior.

Yes.

My only hope is that we could just offload the entries pointing towards
br0, and ignore the local ones. But for that I would need the bridge

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.

quoted

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.

BR_FDB_LOCAL and !fdb->dst are not the same thing, check fdb_add_entry().
You cannot get rid of it, !fdb->dst implies BR_FDB_LOCAL, but it's not
symmetrical.

Scratch that, I spoke too soon. You can get rid of it internally, just need
to be careful not to break user-visible behaviour as Vladimir mentioned.

DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1;
	t=1611005977; bh=bdfYBvMa8LNyHkRyEtaHStOZr794nuxZw02BF6Zfg5c=;
	h=ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:
	 Authentication-Results:Subject:From:To:CC:References:Message-ID:
	 Date:User-Agent:In-Reply-To:Content-Type:Content-Language:
	 Content-Transfer-Encoding:X-Originating-IP:X-ClientProxiedBy:
	 MIME-Version:X-MS-Exchange-MessageSentRepresentingType:
	 X-MS-PublicTrafficType:X-MS-Office365-Filtering-Correlation-Id:
	 X-MS-TrafficTypeDiagnostic:X-MS-Exchange-Transport-Forked:
	 X-Microsoft-Antispam-PRVS:X-MS-Oob-TLC-OOBClassifiers:
	 X-MS-Exchange-SenderADCheck:X-Microsoft-Antispam:
	 X-Microsoft-Antispam-Message-Info:X-Forefront-Antispam-Report:
	 X-MS-Exchange-AntiSpam-MessageData:
	 X-MS-Exchange-CrossTenant-Network-Message-Id:
	 X-MS-Exchange-CrossTenant-AuthSource:
	 X-MS-Exchange-CrossTenant-AuthAs:
	 X-MS-Exchange-CrossTenant-OriginalArrivalTime:
	 X-MS-Exchange-CrossTenant-FromEntityHeader:
	 X-MS-Exchange-CrossTenant-Id:X-MS-Exchange-CrossTenant-MailboxType:
	 X-MS-Exchange-CrossTenant-UserPrincipalName:
	 X-MS-Exchange-Transport-CrossTenantHeadersStamped:X-OriginatorOrg;
	b=lOH2ClNGfg5hhcLMJSlOtsM9K7JoVYkrv1v5FBtgFxxiiWBcOGYir2uqdIfXvMTD/
	 LUTYZ9tVzvZJ/tKymoPhlR+V27URN1nOwkEdz3k1u46QcB+3eQa1blAz8c8bU/HMAP
	 joO4AKJ9BIuG/sc3ZTAX7jdOE3JUSOmCdfhqTCNc6sGmaVFBAwPrrhGPVth3niCkc7
	 JwfTXir+8JtBC0XV3Vw2DiYs8RCX22S/48evhzu6O3PNsmLTFaOZaDb0Ep76MquFPu
	 rjCjXH2ZfG9J/D9YchY/hybtGMRK4aruos1La9mEVi6WzUeW+PhR0/FiXzfW/6fef7
	 cswqoYtddosLw==

Apologies for the multiple emails, but wanted to leave an example:

00:11:22:33:44:55 dev ens16 master bridge permanent

This must always exist and user-space must be able to create it, which
might be against what you want to achieve (no BR_FDB_LOCAL entries with
fdb->dst != NULL).