[RFC net-next 0/7] net: dsa: Sync local bridge FDB addresses to hardware

[RFC net-next 0/7] net: dsa: Sync local bridge FDB addresses to hardware

[Tobias Waldekranz]

This is an extension of previous work done by Vladimir Oltean:
https://lore.kernel.org/netdev/20210106095136.224739-1-olteanv@gmail.com/

With this series, local addresses belonging to bridge ports or to the
bridge itself are also synced down to the hardware FDB. As a result
the hardware can avoid flooding return traffic to the CPU which is not
only inefficient but also very confusing to users:

https://lore.kernel.org/netdev/CAGwvh_MAQWuKuhu5VuYjibmyN-FRxCXXhrQBRm34GShZPSN6Aw@mail.gmail.com/
https://lore.kernel.org/netdev/6106e3d5-31fc-388e-d4ac-c84ac0746a72@prevas.dk/

Patch 1 through 3 extends the switchdev fdb notifications to include
the local flag, and to handle the case when an entry is added to the
bridge itself.

Patches 4 through 6 enables DSA to make use of those extensions.

Finally, enable assisted learning on the CPU port for mv88e6xxx.

Tobias Waldekranz (7):
  net: bridge: switchdev: Refactor br_switchdev_fdb_notify
  net: bridge: switchdev: Include local flag in FDB notifications
  net: bridge: switchdev: Send FDB notifications for host addresses
  net: dsa: Include local addresses in assisted CPU port learning
  net: dsa: Include bridge addresses in assisted CPU port learning
  net: dsa: Sync static FDB entries on foreign interfaces to hardware
  net: dsa: mv88e6xxx: Request assisted learning on CPU port

 drivers/net/dsa/mv88e6xxx/chip.c |  1 +
 include/net/switchdev.h          |  1 +
 net/bridge/br_fdb.c              |  4 +--
 net/bridge/br_private.h          |  7 +++--
 net/bridge/br_switchdev.c        | 47 ++++++++++----------------------
 net/dsa/slave.c                  | 26 ++++++++++++------
 6 files changed, 40 insertions(+), 46 deletions(-)

-- 
2.17.1

[RFC net-next 1/7] net: bridge: switchdev: Refactor br_switchdev_fdb_notify

[Tobias Waldekranz]

Instead of having to add more and more arguments to
br_switchdev_fdb_call_notifiers, get rid of it and build the info
struct directly in br_switchdev_fdb_notify.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 net/bridge/br_switchdev.c | 41 +++++++++++----------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index a9c23ef83443..ff470add8e52 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -102,46 +102,27 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
 	return 0;
 }
 
-static void
-br_switchdev_fdb_call_notifiers(bool adding, const unsigned char *mac,
-				u16 vid, struct net_device *dev,
-				bool added_by_user, bool offloaded)
-{
-	struct switchdev_notifier_fdb_info info;
-	unsigned long notifier_type;
-
-	info.addr = mac;
-	info.vid = vid;
-	info.added_by_user = added_by_user;
-	info.offloaded = offloaded;
-	notifier_type = adding ? SWITCHDEV_FDB_ADD_TO_DEVICE : SWITCHDEV_FDB_DEL_TO_DEVICE;
-	call_switchdev_notifiers(notifier_type, dev, &info.info, NULL);
-}
-
 void
 br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
 {
+	struct switchdev_notifier_fdb_info info = {
+		.addr = fdb->key.addr.addr,
+		.vid = fdb->key.vlan_id,
+		.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags),
+		.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags),
+	};
+
 	if (!fdb->dst)
 		return;
 
 	switch (type) {
 	case RTM_DELNEIGH:
-		br_switchdev_fdb_call_notifiers(false, fdb->key.addr.addr,
-						fdb->key.vlan_id,
-						fdb->dst->dev,
-						test_bit(BR_FDB_ADDED_BY_USER,
-							 &fdb->flags),
-						test_bit(BR_FDB_OFFLOADED,
-							 &fdb->flags));
+		call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_DEVICE,
+					 fdb->dst->dev, &info.info, NULL);
 		break;
 	case RTM_NEWNEIGH:
-		br_switchdev_fdb_call_notifiers(true, fdb->key.addr.addr,
-						fdb->key.vlan_id,
-						fdb->dst->dev,
-						test_bit(BR_FDB_ADDED_BY_USER,
-							 &fdb->flags),
-						test_bit(BR_FDB_OFFLOADED,
-							 &fdb->flags));
+		call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_DEVICE,
+					 fdb->dst->dev, &info.info, NULL);
 		break;
 	}
 }
-- 
2.17.1

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

[Tobias Waldekranz]

Some switchdev drivers, notably DSA, ignore all dynamically learned
address notifications (!added_by_user) as these are autonomously added
by the switch. Previously, such a notification was indistinguishable
from a local address notification. Include a local bit in the
notification so that the two classes can be discriminated.

This allows DSA-like devices to add local addresses to the hardware
FDB (with the CPU as the destination), thereby avoiding flows towards
the CPU being flooded by the switch as unknown unicast.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 include/net/switchdev.h   | 1 +
 net/bridge/br_switchdev.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 88fcac140966..43e4469a17b1 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -213,6 +213,7 @@ struct switchdev_notifier_fdb_info {
 	const unsigned char *addr;
 	u16 vid;
 	u8 added_by_user:1,
+	   local:1,
 	   offloaded:1;
 };
 
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index ff470add8e52..1090bb3d4ee0 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -109,6 +109,7 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
 		.addr = fdb->key.addr.addr,
 		.vid = fdb->key.vlan_id,
 		.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags),
+		.local = test_bit(BR_FDB_LOCAL, &fdb->flags),
 		.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags),
 	};
 
-- 
2.17.1

[RFC net-next 3/7] net: bridge: switchdev: Send FDB notifications for host addresses

[Tobias Waldekranz]

Treat addresses added to the bridge itself in the same way as regular
ports and send out a notification so that drivers may sync it down to
the hardware FDB.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 net/bridge/br_fdb.c       |  4 ++--
 net/bridge/br_private.h   |  7 ++++---
 net/bridge/br_switchdev.c | 11 +++++------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index b7490237f3fc..1d54ae0f58fb 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -602,7 +602,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 			/* fastpath: update of existing entry */
 			if (unlikely(source != fdb->dst &&
 				     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
-				br_switchdev_fdb_notify(fdb, RTM_DELNEIGH);
+				br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
 				fdb->dst = source;
 				fdb_modified = true;
 				/* Take over HW learned entry */
@@ -735,7 +735,7 @@ static void fdb_notify(struct net_bridge *br,
 	int err = -ENOBUFS;
 
 	if (swdev_notify)
-		br_switchdev_fdb_notify(fdb, type);
+		br_switchdev_fdb_notify(br, fdb, type);
 
 	skb = nlmsg_new(fdb_nlmsg_size(), GFP_ATOMIC);
 	if (skb == NULL)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index d62c6e1af64a..a3e20b747eca 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1568,8 +1568,8 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
 int br_switchdev_set_port_flag(struct net_bridge_port *p,
 			       unsigned long flags,
 			       unsigned long mask);
-void br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb,
-			     int type);
+void br_switchdev_fdb_notify(struct net_bridge *br,
+			     const struct net_bridge_fdb_entry *fdb, int type);
 int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags,
 			       struct netlink_ext_ack *extack);
 int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid);
@@ -1615,7 +1615,8 @@ static inline int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid)
 }
 
 static inline void
-br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
+br_switchdev_fdb_notify(struct net_bridge *br,
+			const struct net_bridge_fdb_entry *fdb, int type)
 {
 }
 
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 1090bb3d4ee0..125637c34f14 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -103,7 +103,8 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
 }
 
 void
-br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
+br_switchdev_fdb_notify(struct net_bridge *br,
+			const struct net_bridge_fdb_entry *fdb, int type)
 {
 	struct switchdev_notifier_fdb_info info = {
 		.addr = fdb->key.addr.addr,
@@ -112,18 +113,16 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
 		.local = test_bit(BR_FDB_LOCAL, &fdb->flags),
 		.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags),
 	};
-
-	if (!fdb->dst)
-		return;
+	struct net_device *dev = fdb->dst ? fdb->dst->dev : br->dev;
 
 	switch (type) {
 	case RTM_DELNEIGH:
 		call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_DEVICE,
-					 fdb->dst->dev, &info.info, NULL);
+					 dev, &info.info, NULL);
 		break;
 	case RTM_NEWNEIGH:
 		call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_DEVICE,
-					 fdb->dst->dev, &info.info, NULL);
+					 dev, &info.info, NULL);
 		break;
 	}
 }
-- 
2.17.1

[RFC net-next 4/7] net: dsa: Include local addresses in assisted CPU port learning

[Tobias Waldekranz]

Add local addresses (i.e. the ports' MAC addresses) to the hardware
FDB when assisted CPU port learning is enabled.

NOTE: The bridge's own MAC address is also "local". If that address is
not shared with any port, the bridge's MAC is not be added by this
functionality - but the following commit takes care of that case.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 net/dsa/slave.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index c5c81cba8259..dca393e45547 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2174,10 +2174,12 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 		fdb_info = ptr;
 
 		if (dsa_slave_dev_check(dev)) {
-			if (!fdb_info->added_by_user)
-				return NOTIFY_OK;
-
 			dp = dsa_slave_to_port(dev);
+
+			if (fdb_info->local && dp->ds->assisted_learning_on_cpu_port)
+				dp = dp->cpu_dp;
+			else if (!fdb_info->added_by_user)
+				return NOTIFY_OK;
 		} else {
 			/* Snoop addresses learnt on foreign interfaces
 			 * bridged with us, for switches that don't
-- 
2.17.1

[RFC net-next 5/7] net: dsa: Include bridge addresses in assisted CPU port learning

[Tobias Waldekranz]

Now that notifications are sent out for addresses added to the bridge
itself, extend DSA to include those addresses in the hardware FDB when
assisted CPU port learning is enabled.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 net/dsa/slave.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index dca393e45547..1ac46ad4a846 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2188,7 +2188,11 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 			struct net_device *br_dev;
 			struct dsa_slave_priv *p;
 
-			br_dev = netdev_master_upper_dev_get_rcu(dev);
+			if (netif_is_bridge_master(dev))
+				br_dev = dev;
+			else
+				br_dev = netdev_master_upper_dev_get_rcu(dev);
+
 			if (!br_dev)
 				return NOTIFY_DONE;
 
-- 
2.17.1

[RFC net-next 6/7] net: dsa: Sync static FDB entries on foreign interfaces to hardware

[Tobias Waldekranz]

Reuse the "assisted_learning_on_cpu_port" functionality to always add
entries for user-configured entries on foreign interfaces, even if
assisted_learning_on_cpu_port is not enabled. E.g. in this situation:

   br0
   / \
swp0 dummy0

$ bridge fdb add 02:00:de:ad:00:01 dev dummy0 vlan 1 master

Results in DSA adding an entry in the hardware FDB, pointing this
address towards the CPU port.

The same is true for entries added to the bridge itself, e.g:

$ bridge fdb add 02:00:de:ad:00:01 dev br0 vlan 1 self

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 net/dsa/slave.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 1ac46ad4a846..f89b5eb4d2d6 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2181,9 +2181,12 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 			else if (!fdb_info->added_by_user)
 				return NOTIFY_OK;
 		} else {
-			/* Snoop addresses learnt on foreign interfaces
-			 * bridged with us, for switches that don't
-			 * automatically learn SA from CPU-injected traffic
+			/* Snoop addresses added to foreign interfaces
+			 * bridged with us, or the bridge
+			 * itself. Dynamically learned addresses can
+			 * also be added for switches that don't
+			 * automatically learn SA from CPU-injected
+			 * traffic.
 			 */
 			struct net_device *br_dev;
 			struct dsa_slave_priv *p;
@@ -2205,7 +2208,8 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 
 			dp = p->dp->cpu_dp;
 
-			if (!dp->ds->assisted_learning_on_cpu_port)
+			if (!fdb_info->added_by_user &&
+			    !dp->ds->assisted_learning_on_cpu_port)
 				return NOTIFY_DONE;
 		}
 
-- 
2.17.1

[RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port

[Tobias Waldekranz]

While the hardware is capable of performing learning on the CPU port,
it requires alot of additions to the bridge's forwarding path in order
to handle multi-destination traffic correctly.

Until that is in place, opt for the next best thing and let DSA sync
the relevant addresses down to the hardware FDB.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 91286d7b12c7..398f3cc8d2f3 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5726,6 +5726,7 @@ static int mv88e6xxx_register_switch(struct mv88e6xxx_chip *chip)
 	ds->ops = &mv88e6xxx_switch_ops;
 	ds->ageing_time_min = chip->info->age_time_coeff;
 	ds->ageing_time_max = chip->info->age_time_coeff * U8_MAX;
+	ds->assisted_learning_on_cpu_port = true;
 
 	/* Some chips support up to 32, but that requires enabling the
 	 * 5-bit port mode, which we do not support. 640k^W16 ought to
-- 
2.17.1

Re: [RFC net-next 1/7] net: bridge: switchdev: Refactor br_switchdev_fdb_notify

[Vladimir Oltean]

On Sat, Jan 16, 2021 at 02:25:09AM +0100, Tobias Waldekranz wrote:
> Instead of having to add more and more arguments to
> br_switchdev_fdb_call_notifiers, get rid of it and build the info
> struct directly in br_switchdev_fdb_notify.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

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

[Vladimir Oltean]

Hi Tobias,

On Sat, Jan 16, 2021 at 02:25:10AM +0100, Tobias Waldekranz wrote:
> Some switchdev drivers, notably DSA, ignore all dynamically learned
> address notifications (!added_by_user) as these are autonomously added
> by the switch. Previously, such a notification was indistinguishable
> from a local address notification. Include a local bit in the
> notification so that the two classes can be discriminated.
>
> This allows DSA-like devices to add local addresses to the hardware
> FDB (with the CPU as the destination), thereby avoiding flows towards
> the CPU being flooded by the switch as unknown unicast.
>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

In an ideal world, the BR_FDB_LOCAL bit of an FDB entry is what you
would probably want to use as an indication that the packet must be
delivered upstream by the hardware, considering that this is what the
software data path does:

br_handle_frame_finish:
		if (test_bit(BR_FDB_LOCAL, &dst->flags))
			return br_pass_frame_up(skb);

However, we are not in an ideal world, but in a cacophony of nonsensical
flags that must be passed to the 'bridge fdb add' command. For example,
I noticed this usage pattern in your patch 6/7:

|    br0
|    / \
| swp0 dummy0
|
| $ bridge fdb add 02:00:de:ad:00:01 dev dummy0 vlan 1 master

Do you know that this command doesn't do what you think it does
(assuming that 02:00:de:ad:00:01 is not the MAC address of dummy0)?

The command you wrote will add a _local_ FDB entry on dummy0.
I tried it on a DSA switch interface (swp0):

$ bridge fdb add 02:00:de:ad:00:01 dev swp0 vlan 1 master
[ 3162.165561] rtnl_fdb_add: addr 02:00:de:ad:00:01 vid 1 ndm_state NUD_NOARP|NUD_PERMANENT
[ 3162.172487] fdb_add_entry: fdb 02:00:de:ad:00:01 state NUD_NOARP|NUD_PERMANENT, fdb_to_nud NUD_REACHABLE, flags 0x0
[ 3162.180515] mscc_felix 0000:00:00.5 swp0: local fdb: 02:00:de:ad:00:01 vid 1

So, after your patches, this command syntax will stop adding a
front-facing FDB entry on swp0. It will add a CPU-facing FDB entry
instead.

You know why the bridge neighbour state is NUD_NOARP|NUD_PERMANENT in
rtnl_fdb_add? Well, because iproute2 set it that way:

	/* Assume permanent */
	if (!(req.ndm.ndm_state&(NUD_PERMANENT|NUD_REACHABLE)))
		req.ndm.ndm_state |= NUD_PERMANENT;

See iproute2 commit 0849e60a10d0 ("bridge: manage VXLAN based forwarding
tables"). I know so little about VXLAN's use of the bridge command, that
I cannot tell why it was decided to "assume permanent" (which seems to
have changed the default behavior for everybody).

Otherwise said, even if not mentioned in the man page, the default FDB
entry type is NUD_PERMANENT (which, in short, means a "local" entry, see
a more detailed explanation at the end).

The man page just says:

   bridge fdb add - add a new fdb entry
       This command creates a new fdb entry.

       LLADDR the Ethernet MAC address.

       dev DEV
              the interface to which this address is associated.

              local - is a local permanent fdb entry

              static - is a static (no arp) fdb entry

which is utterly misleading and useless. It does not say:
(a) what a local FDB entry is
(b) that if neither "local" or "static"|"dynamic" is specified,
    "local" is default

This already creates pretty bad premises. You would have needed to
explicitly add "static" to your command. Not only you, but in fact also
thousands of other people who already have switchdev deployments using
the 'bridge fdb' command.

So, in short, if everybody with switchdev hardware used the 'bridge fdb'
command correctly so far, your series would have been great. But in
fact, nobody did (me included). So we need to be more creative.

For example, there's that annoying "self" flag.
As far as I understand, there is zero reason for a DSA driver to use the
"self" flag, since that means "bypass the bridge FDB and just call the
.ndo_fdb_add of the device driver, which in the case of DSA is
dsa_legacy_fdb_add". Instead you would just use the "master" flag, which
makes the operation be propagated through br_fdb_add and the software
FDB has a chance to be updated.

Only that there's no one preventing you from using the 'self' and
'master' flags together. Which means that the FDB would be offloaded to
the device twice: once through the SWITCHDEV_FDB_ADD_TO_DEVICE
notification emitted by br_fdb_add, and once through dsa_legacy_fdb_add.
Contradict me if I'm wrong, but I'm thinking that you may have missed
this detail that bridge fdb addresses are implicitly 'local' because you
also used some 'self master' commands, and the "to-CPU" address
installed through switchdev was immediately overwritten by the correct
one installed through dsa_legacy_fdb_add.

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.

What I'm saying is that some of the is_local addresses should have been
rejected by DSA from day one, like this one:

bridge fdb add dev swp0 00:01:02:03:04:05 master

but nonetheless DSA is happy to accept it anyway, because switchdev
doesn't tell it it's local. Yay.

It looks like Mellanox have been telling their users to explicitly use
the "static" keyword when they mean a static FDB entry:
https://github.com/Mellanox/mlxsw/wiki/Bridge#forwarding-database-configuration
which, I mean, is great for them, but pretty much sucks for everybody
else, because Documentation/networking/switchdev.rst just says:

The switchdev driver should implement ndo_fdb_add, ndo_fdb_del and ndo_fdb_dump
to support static FDB entries installed to the device.  Static bridge FDB
entries are installed, for example, using iproute2 bridge cmd::

	bridge fdb add ADDR dev DEV [vlan VID] [self]

which of course is completely bogus anyway (it indicates 'self', it
doesn't indicate 'master' at all, it doesn't say anything about 'static').

Look, I'd be more than happy to accept that I'm the only idiot who
misread the existing documentation on 'bridge fdb', but I fear that this
is far from true. If we want to make progress with this patch, some user
space breakage will be necessary - and I think I'm in favour of doing
that.


Appendix:

The bridge FDB netlink UAPI is modeled as a neighbouring protocol for
PF_BRIDGE, similar to what ARP is for IPv4 and ND is for IPv6. There is
this forced correlation between generic neighbour states (NUD ==
"Network Unreachability Detection") and types of FDB entries:
- NUD_NOARP:
Normally this state is used to denote a neighbour that doesn't need any
protocol to resolve the mapping between L3 address and L2 address.
It seems to be mostly used when the neigh->dev does not implement
header_ops (net device has no L2 header), and therefore no neighbor
resolution is needed, because there is no L2 addressing on that device.
For PF_BRIDGE, all FDB entries are NUD_NOARP, except for 'dynamic' ones
(see below).

- NUD_PERMANENT:
Normally this state means that the L2 address of the neighbor has been
statically configured by the user and therefore there is no need for a
neighbour resolution.
For PF_BRIDGE though, it means that an FDB entry is 'local', i.e. the L2
address belongs to a local interface.

- NUD_REACHABLE:
Normally this state means that the L2 address has been resolved by the
neighbouring protocol.
For PF_BRIDGE, this flag must be interpreted together with NUD_NOARP for
full meaning.
NUD_REACHABLE=true, NUD_NOARP=true: static FDB entry
NUD_REACHABLE=true, NUD_NOARP=false: dynamic FDB entry

Re: [RFC net-next 3/7] net: bridge: switchdev: Send FDB notifications for host addresses

[Vladimir Oltean]

On Sat, Jan 16, 2021 at 02:25:11AM +0100, Tobias Waldekranz wrote:
> Treat addresses added to the bridge itself in the same way as regular
> ports and send out a notification so that drivers may sync it down to
> the hardware FDB.
>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

What is the functional difference between a bridge FDB entry which has
the BR_FDB_LOCAL flag, and a bridge FDB entry with a NULL fdb->dst?

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

[Tobias Waldekranz]

On Sun, Jan 17, 2021 at 21:30, Vladimir Oltean <olteanv@gmail.com> wrote:
> Hi Tobias,
>
> On Sat, Jan 16, 2021 at 02:25:10AM +0100, Tobias Waldekranz wrote:
>> Some switchdev drivers, notably DSA, ignore all dynamically learned
>> address notifications (!added_by_user) as these are autonomously added
>> by the switch. Previously, such a notification was indistinguishable
>> from a local address notification. Include a local bit in the
>> notification so that the two classes can be discriminated.
>>
>> This allows DSA-like devices to add local addresses to the hardware
>> FDB (with the CPU as the destination), thereby avoiding flows towards
>> the CPU being flooded by the switch as unknown unicast.
>>
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> ---
>
> In an ideal world, the BR_FDB_LOCAL bit of an FDB entry is what you
> would probably want to use as an indication that the packet must be
> delivered upstream by the hardware, considering that this is what the
> software data path does:
>
> br_handle_frame_finish:
> 		if (test_bit(BR_FDB_LOCAL, &dst->flags))
> 			return br_pass_frame_up(skb);

That was my thinking, yes.

> However, we are not in an ideal world, but in a cacophony of nonsensical
> flags that must be passed to the 'bridge fdb add' command. For example,
> I noticed this usage pattern in your patch 6/7:
>
> |    br0
> |    / \
> | swp0 dummy0
> |
> | $ bridge fdb add 02:00:de:ad:00:01 dev dummy0 vlan 1 master
>
> Do you know that this command doesn't do what you think it does
> (assuming that 02:00:de:ad:00:01 is not the MAC address of dummy0)?
>
> The command you wrote will add a _local_ FDB entry on dummy0.
> I tried it on a DSA switch interface (swp0):
>
> $ bridge fdb add 02:00:de:ad:00:01 dev swp0 vlan 1 master
> [ 3162.165561] rtnl_fdb_add: addr 02:00:de:ad:00:01 vid 1 ndm_state NUD_NOARP|NUD_PERMANENT
> [ 3162.172487] fdb_add_entry: fdb 02:00:de:ad:00:01 state NUD_NOARP|NUD_PERMANENT, fdb_to_nud NUD_REACHABLE, flags 0x0
> [ 3162.180515] mscc_felix 0000:00:00.5 swp0: local fdb: 02:00:de:ad:00:01 vid 1
>
> So, after your patches, this command syntax will stop adding a
> front-facing FDB entry on swp0. It will add a CPU-facing FDB entry
> instead.

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".

> You know why the bridge neighbour state is NUD_NOARP|NUD_PERMANENT in
> rtnl_fdb_add? Well, because iproute2 set it that way:
>
> 	/* Assume permanent */
> 	if (!(req.ndm.ndm_state&(NUD_PERMANENT|NUD_REACHABLE)))
> 		req.ndm.ndm_state |= NUD_PERMANENT;
>
> See iproute2 commit 0849e60a10d0 ("bridge: manage VXLAN based forwarding
> tables"). I know so little about VXLAN's use of the bridge command, that
> I cannot tell why it was decided to "assume permanent" (which seems to
> have changed the default behavior for everybody).
>
> Otherwise said, even if not mentioned in the man page, the default FDB
> entry type is NUD_PERMANENT (which, in short, means a "local" entry, see
> a more detailed explanation at the end).
>
> The man page just says:
>
>    bridge fdb add - add a new fdb entry
>        This command creates a new fdb entry.
>
>        LLADDR the Ethernet MAC address.
>
>        dev DEV
>               the interface to which this address is associated.
>
>               local - is a local permanent fdb entry
>
>               static - is a static (no arp) fdb entry
>
> which is utterly misleading and useless. It does not say:
> (a) what a local FDB entry is
> (b) that if neither "local" or "static"|"dynamic" is specified,
>     "local" is default
>
> This already creates pretty bad premises. You would have needed to
> explicitly add "static" to your command. Not only you, but in fact also
> thousands of other people who already have switchdev deployments using
> the 'bridge fdb' command.
>
> So, in short, if everybody with switchdev hardware used the 'bridge fdb'
> command correctly so far, your series would have been great. But in
> fact, nobody did (me included). So we need to be more creative.
>
> For example, there's that annoying "self" flag.
> As far as I understand, there is zero reason for a DSA driver to use the
> "self" flag, since that means "bypass the bridge FDB and just call the
> .ndo_fdb_add of the device driver, which in the case of DSA is
> dsa_legacy_fdb_add". Instead you would just use the "master" flag, which
> makes the operation be propagated through br_fdb_add and the software
> FDB has a chance to be updated.
>
> Only that there's no one preventing you from using the 'self' and
> 'master' flags together. Which means that the FDB would be offloaded to
> the device twice: once through the SWITCHDEV_FDB_ADD_TO_DEVICE
> notification emitted by br_fdb_add, and once through dsa_legacy_fdb_add.
> Contradict me if I'm wrong, but I'm thinking that you may have missed
> this detail that bridge fdb addresses are implicitly 'local' because you
> also used some 'self master' commands, and the "to-CPU" address
> installed through switchdev was immediately overwritten by the correct
> one installed through dsa_legacy_fdb_add.
>
> 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?

> What I'm saying is that some of the is_local addresses should have been
> rejected by DSA from day one, like this one:
>
> bridge fdb add dev swp0 00:01:02:03:04:05 master
>
> but nonetheless DSA is happy to accept it anyway, because switchdev
> doesn't tell it it's local. Yay.

Yes that is a real problem, for sure.

> It looks like Mellanox have been telling their users to explicitly use
> the "static" keyword when they mean a static FDB entry:
> https://github.com/Mellanox/mlxsw/wiki/Bridge#forwarding-database-configuration
> which, I mean, is great for them, but pretty much sucks for everybody
> else, because Documentation/networking/switchdev.rst just says:
>
> The switchdev driver should implement ndo_fdb_add, ndo_fdb_del and ndo_fdb_dump
> to support static FDB entries installed to the device.  Static bridge FDB
> entries are installed, for example, using iproute2 bridge cmd::
>
> 	bridge fdb add ADDR dev DEV [vlan VID] [self]
>
> which of course is completely bogus anyway (it indicates 'self', it
> doesn't indicate 'master' at all, it doesn't say anything about 'static').
>
> Look, I'd be more than happy to accept that I'm the only idiot who
> misread the existing documentation on 'bridge fdb', but I fear that this
> is far from true. If we want to make progress with this patch, some user
> space breakage will be necessary - and I think I'm in favour of doing
> that.

Trust me, you are not. There is a running joke about being able to
describe what "master" and "self" really means here at the office :)

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.

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.

Correct?

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

[Vladimir Oltean]

On Mon, Jan 18, 2021 at 07:58:59PM +0100, Tobias Waldekranz wrote:
> 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".

> > 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.

> 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.

> 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
maintainers to clarify what is the difference between then, as I asked
in your other patch.

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

[Ido Schimmel]

On Sun, Jan 17, 2021 at 09:30:09PM +0200, Vladimir Oltean wrote:
> Hi Tobias,
> 
> On Sat, Jan 16, 2021 at 02:25:10AM +0100, Tobias Waldekranz wrote:
> > Some switchdev drivers, notably DSA, ignore all dynamically learned
> > address notifications (!added_by_user) as these are autonomously added
> > by the switch. Previously, such a notification was indistinguishable
> > from a local address notification. Include a local bit in the
> > notification so that the two classes can be discriminated.
> >
> > This allows DSA-like devices to add local addresses to the hardware
> > FDB (with the CPU as the destination), thereby avoiding flows towards
> > the CPU being flooded by the switch as unknown unicast.
> >
> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> > ---
> 
> In an ideal world, the BR_FDB_LOCAL bit of an FDB entry is what you
> would probably want to use as an indication that the packet must be
> delivered upstream by the hardware, considering that this is what the
> software data path does:
> 
> br_handle_frame_finish:
> 		if (test_bit(BR_FDB_LOCAL, &dst->flags))
> 			return br_pass_frame_up(skb);
> 
> However, we are not in an ideal world, but in a cacophony of nonsensical
> flags that must be passed to the 'bridge fdb add' command. For example,
> I noticed this usage pattern in your patch 6/7:

Thanks for adding me. Reflecting FDB flags is a very much needed change.
I will take a look tomorrow or the day after.

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

[Tobias Waldekranz]

On Mon, Jan 18, 2021 at 21:27, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Mon, Jan 18, 2021 at 07:58:59PM +0100, Tobias Waldekranz wrote:
>> 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.

>> > 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.

>> 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.

>> 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.

> 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.

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

[Vladimir Oltean]

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?

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

[Nikolay Aleksandrov]

On 18/01/2021 22:19, Tobias Waldekranz wrote:
> On Mon, Jan 18, 2021 at 21:27, Vladimir Oltean <olteanv@gmail.com> wrote:
>> On Mon, Jan 18, 2021 at 07:58:59PM +0100, Tobias Waldekranz wrote:
>>> 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.
> 
>>>> 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.
> 
>>> 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.
> 
>>> 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.
> 
>> 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.

Cheers,
 Nik

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

[Nikolay Aleksandrov]

On 18/01/2021 23:17, Nikolay Aleksandrov wrote:
> On 18/01/2021 22:19, Tobias Waldekranz wrote:
>> On Mon, Jan 18, 2021 at 21:27, Vladimir Oltean <olteanv@gmail.com> wrote:
>>> On Mon, Jan 18, 2021 at 07:58:59PM +0100, Tobias Waldekranz wrote:
>>>> 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.
>>
>>>>> 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.
>>
>>>> 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.
>>
>>>> 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.
>>
>>> 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.

> Cheers,
>  Nik
> 

 

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

[Nikolay Aleksandrov]

On 18/01/2021 23:22, Nikolay Aleksandrov wrote:
> On 18/01/2021 23:17, Nikolay Aleksandrov wrote:
>> On 18/01/2021 22:19, Tobias Waldekranz wrote:
>>> On Mon, Jan 18, 2021 at 21:27, Vladimir Oltean <olteanv@gmail.com> wrote:
>>>> On Mon, Jan 18, 2021 at 07:58:59PM +0100, Tobias Waldekranz wrote:
>>>>> 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.
>>>
>>>>>> 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.
>>>
>>>>> 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.
>>>
>>>>> 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.
>>>
>>>> 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).

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

[Vladimir Oltean]

On Mon, Jan 18, 2021 at 11:39:27PM +0200, Nikolay Aleksandrov wrote:
> 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).

Can you give me an example of why it would matter that fdb->dst in this
case is set to ens16?

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

[Nikolay Aleksandrov]

On 18/01/2021 23:50, Vladimir Oltean wrote:
> On Mon, Jan 18, 2021 at 11:39:27PM +0200, Nikolay Aleksandrov wrote:
>> 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).
> 
> Can you give me an example of why it would matter that fdb->dst in this
> case is set to ens16?
> 

Can you dump it as "dev ens16" without it? :) 
Or alternatively can you send a notification with "dev ens16" without it?

I'm in favor of removing it, but it is risky since some script somewhere might
be searching for it, or some user-space daemon might expect to see a notification
for such entries and react on it.

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

[Vladimir Oltean]

On Mon, Jan 18, 2021 at 11:53:18PM +0200, Nikolay Aleksandrov wrote:
> On 18/01/2021 23:50, Vladimir Oltean wrote:
> > On Mon, Jan 18, 2021 at 11:39:27PM +0200, Nikolay Aleksandrov wrote:
> >> 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).
> >
> > Can you give me an example of why it would matter that fdb->dst in this
> > case is set to ens16?
> >
>
> Can you dump it as "dev ens16" without it? :)
> Or alternatively can you send a notification with "dev ens16" without it?
>
> I'm in favor of removing it, but it is risky since some script somewhere might
> be searching for it, or some user-space daemon might expect to see a notification
> for such entries and react on it.

If "dev ens16" makes no difference to the forwarding and/or termination
path of the bridge, just to user space reporting, then keeping
appearances is a bit pointless.

For example, DSA switch interfaces inherit by default the MAC address of
the host interface. Having multiple net devices with the same MAC
address works because either they are in different L2 domains (case in
which the MAC addresses should still be unique per domain), or they are
in the same L2 domain, under the same bridge (case in which it is the
bridge who should do IP neighbour resolution etc).
Having that said, let there be these commands:

$ ip link add br0 type bridge
$ ip link set swp0 master br0
$ ip link set swp1 master br0
$ ip link set swp2 master br0
$ ip link set swp3 master br0
$ bridge fdb | grep permanent
00:04:9f:05:de:0a dev swp0 vlan 1 master br0 permanent
00:04:9f:05:de:0a dev swp0 master br0 permanent

And these:

$ ip link add br0 type bridge
$ ip link set swp3 master br0
$ ip link set swp2 master br0
$ ip link set swp1 master br0
$ ip link set swp0 master br0
$ bridge fdb | grep permanent
00:04:9f:05:de:0a dev swp0 vlan 1 master br0 permanent
00:04:9f:05:de:0a dev swp0 master br0 permanent
00:04:9f:05:de:0a dev swp3 vlan 1 master br0 permanent
00:04:9f:05:de:0a dev swp3 master br0 permanent

Preserving the reporting for permanent/local FDB entries added by user
is one thing. But do we need to also preserve this behavior (i.e. report
the first unique MAC address of an interface that joins the bridge as a
permanent/local address on that brport, but not on the others, and not
on br0)? If yes, then I'm afraid there's nothing we can do.

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

[Vladimir Oltean]

On Tue, Jan 19, 2021 at 12:06:16AM +0200, Vladimir Oltean wrote:
> And these:
> 
> $ ip link add br0 type bridge
> $ ip link set swp3 master br0
> $ ip link set swp2 master br0
> $ ip link set swp1 master br0
> $ ip link set swp0 master br0
> $ bridge fdb | grep permanent
> 00:04:9f:05:de:0a dev swp0 vlan 1 master br0 permanent
> 00:04:9f:05:de:0a dev swp0 master br0 permanent
> 00:04:9f:05:de:0a dev swp3 vlan 1 master br0 permanent
> 00:04:9f:05:de:0a dev swp3 master br0 permanent

Ugh, I messed it up. The entries with swp0 are stray here. The permanent
entries get reported only for swp3 in the second case.

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

[Nikolay Aleksandrov]

On 19/01/2021 00:06, Vladimir Oltean wrote:
> On Mon, Jan 18, 2021 at 11:53:18PM +0200, Nikolay Aleksandrov wrote:
>> On 18/01/2021 23:50, Vladimir Oltean wrote:
>>> On Mon, Jan 18, 2021 at 11:39:27PM +0200, Nikolay Aleksandrov wrote:
>>>> 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).
>>>
>>> Can you give me an example of why it would matter that fdb->dst in this
>>> case is set to ens16?
>>>
>>
>> Can you dump it as "dev ens16" without it? :)
>> Or alternatively can you send a notification with "dev ens16" without it?
>>
>> I'm in favor of removing it, but it is risky since some script somewhere might
>> be searching for it, or some user-space daemon might expect to see a notification
>> for such entries and react on it.
> 
> If "dev ens16" makes no difference to the forwarding and/or termination
> path of the bridge, just to user space reporting, then keeping
> appearances is a bit pointless.
> 
> For example, DSA switch interfaces inherit by default the MAC address of
> the host interface. Having multiple net devices with the same MAC
> address works because either they are in different L2 domains (case in
> which the MAC addresses should still be unique per domain), or they are
> in the same L2 domain, under the same bridge (case in which it is the
> bridge who should do IP neighbour resolution etc).
> Having that said, let there be these commands:
> 
> $ ip link add br0 type bridge
> $ ip link set swp0 master br0
> $ ip link set swp1 master br0
> $ ip link set swp2 master br0
> $ ip link set swp3 master br0
> $ bridge fdb | grep permanent
> 00:04:9f:05:de:0a dev swp0 vlan 1 master br0 permanent
> 00:04:9f:05:de:0a dev swp0 master br0 permanent
> 
> And these:
> 
> $ ip link add br0 type bridge
> $ ip link set swp3 master br0
> $ ip link set swp2 master br0
> $ ip link set swp1 master br0
> $ ip link set swp0 master br0
> $ bridge fdb | grep permanent
> 00:04:9f:05:de:0a dev swp0 vlan 1 master br0 permanent
> 00:04:9f:05:de:0a dev swp0 master br0 permanent
> 00:04:9f:05:de:0a dev swp3 vlan 1 master br0 permanent
> 00:04:9f:05:de:0a dev swp3 master br0 permanent
> 
> Preserving the reporting for permanent/local FDB entries added by user
> is one thing. But do we need to also preserve this behavior (i.e. report
> the first unique MAC address of an interface that joins the bridge as a
> permanent/local address on that brport, but not on the others, and not
> on br0)? If yes, then I'm afraid there's nothing we can do.
> 

No, it shouldn't be a problem to change that. We should be careful about the
way it's changed though because reporting it for all ports might become a scale
issue with 4k vlans, and also today you can't add the same mac for multiple ports.
Perhaps the best way is to report it for the bridge itself, while still allowing
such entries to be added/deleted by user-space.

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

[Vladimir Oltean]

On Tue, Jan 19, 2021 at 12:42:04AM +0200, Nikolay Aleksandrov wrote:
> No, it shouldn't be a problem to change that. We should be careful about the
> way it's changed though because reporting it for all ports might become a scale
> issue with 4k vlans, and also today you can't add the same mac for multiple ports.
> Perhaps the best way is to report it for the bridge itself, while still allowing
> such entries to be added/deleted by user-space.

I think what Tobias is trying to achieve is:
(a) offload the locally terminated FDB addresses through switchdev, in a
    way that is not "poisoned", i.e. the driver should not be forced to
    recognize these entries based on the is_local flag. This includes
    the ports MAC addresses which are currently notified as is_local and
    with fdb->dst = source brport (not NULL).
(b) remain compatible with the mistakes of the past, i.e. DSA and
    probably other switchdev users will have to remain oblivious of the
    is_local flag. So we will still have to accept "bridge fdb add
    00:01:02:03:04:05 dev swp0 master local", and it will have to keep
    incorrectly installing a front-facing static FDB entry on swp0
    instead of a local/permanent one.

In terms of implementation, this would mean that for added_by_user
entries, we keep the existing notifications broken as they are.
Whereas for !added_by_user, we replace them as much as possible with
"fdb->dst == NULL" entries (i.e. for br0).

I haven't looked closely at the code, and I hope that this will not
happen, but maybe some of these addresses will inevitably have to be
duplicated with is_local addresses that were previously notified. In
that case I'm thinking there must be some hackery to always offload the
addresses in this order: first the is_local address, then the br0
address, to allow the bad entry to be overwritten with the good one.

Finally, we should modify the bridge manpage to say "we know that the
local|permanent flag is added by default, but it's deprecated so pls
don't use it anymore, just use fdb on br0".

How does this sound?

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

[Nikolay Aleksandrov]

On 19/01/2021 02:42, Vladimir Oltean wrote:
> On Tue, Jan 19, 2021 at 12:42:04AM +0200, Nikolay Aleksandrov wrote:
>> No, it shouldn't be a problem to change that. We should be careful about the
>> way it's changed though because reporting it for all ports might become a scale
>> issue with 4k vlans, and also today you can't add the same mac for multiple ports.
>> Perhaps the best way is to report it for the bridge itself, while still allowing
>> such entries to be added/deleted by user-space.
> 
> I think what Tobias is trying to achieve is:
> (a) offload the locally terminated FDB addresses through switchdev, in a
>     way that is not "poisoned", i.e. the driver should not be forced to
>     recognize these entries based on the is_local flag. This includes
>     the ports MAC addresses which are currently notified as is_local and
>     with fdb->dst = source brport (not NULL).
> (b) remain compatible with the mistakes of the past, i.e. DSA and
>     probably other switchdev users will have to remain oblivious of the
>     is_local flag. So we will still have to accept "bridge fdb add
>     00:01:02:03:04:05 dev swp0 master local", and it will have to keep
>     incorrectly installing a front-facing static FDB entry on swp0
>     instead of a local/permanent one.
> 
> In terms of implementation, this would mean that for added_by_user
> entries, we keep the existing notifications broken as they are.
> Whereas for !added_by_user, we replace them as much as possible with
> "fdb->dst == NULL" entries (i.e. for br0).
> 
> I haven't looked closely at the code, and I hope that this will not
> happen, but maybe some of these addresses will inevitably have to be
> duplicated with is_local addresses that were previously notified. In
> that case I'm thinking there must be some hackery to always offload the
> addresses in this order: first the is_local address, then the br0
> address, to allow the bad entry to be overwritten with the good one.
> 
> Finally, we should modify the bridge manpage to say "we know that the
> local|permanent flag is added by default, but it's deprecated so pls
> don't use it anymore, just use fdb on br0".
> 
> How does this sound?
> 

We'll be supporting it forever, I don't see how it's being deprecated. :)
Either way I'm ok with the above, but I'll be able to comment further when
I see how exactly the code would change. We should be very careful not to break
someone who uses these entries in a way we can't think of, for example we use
permanent user-space added entries combined with ext_learn for mlag purposes
which is a stranger use case, granted it won't be broken by the above.
Perhaps we should consider making the new behaviour optional instead, then
we can completely switch between the two modes and drop compatibility.

Re: [RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port

[DENG Qingfang]

Hi Tobias,

I've tested your patch series on kernel 5.4 and found that it only works
when VLAN filtering is enabled.
After some debugging, I noticed DSA will add static entries to ATU 0 if
VLAN filtering is disabled, regardless of default_pvid of the bridge,
which is also the ATU# used by the bridge.

Currently I use the hack below to rewrite ATU# to 1, but it obviously
does not solve the root cause.

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b99f27b8c084..9c897c03896f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2106,6 +2106,7 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
+	vid = vid ? : 1;
 	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
 					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
@@ -2120,6 +2121,7 @@ static int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
+	vid = vid ? : 1;
 	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid, 0);
 	mv88e6xxx_reg_unlock(chip);
-- 

Any ideas?

Re: [RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port

[Tobias Waldekranz]

On Mon, Feb 01, 2021 at 14:24, DENG Qingfang <dqfext@gmail.com> wrote:
> Hi Tobias,
>
> I've tested your patch series on kernel 5.4 and found that it only works
> when VLAN filtering is enabled.
> After some debugging, I noticed DSA will add static entries to ATU 0 if
> VLAN filtering is disabled, regardless of default_pvid of the bridge,
> which is also the ATU# used by the bridge.

Good catch, thanks! This seems to stem from a different policy regarding
VLAN assignment in the bridge vs. how a Marvell switch works.

By default, a bridge will use a default PVID of 1, even when VLAN
filtering is disabled (nbp_vlan_init). Yet it will assign all packets to
VLAN 0 on ingress (br_handle_frame_finish->br_allowed_ingress).

The switch OTOH, will use the PVID of the port for all packets when
802.1Q is disabled, thus assigning all packets to VLAN 1 when VLAN
filtering is disabled.

Andrew, Vladimir: Should mv88e6xxx always set the PVID to 0 when VLAN
filtering is disabled?

> Currently I use the hack below to rewrite ATU# to 1, but it obviously
> does not solve the root cause.

Alternatively, as a userspace workaround, you can change the default
PVID to 0:

ip link set dev $BR type bridge vlan_default_pvid 0

Re: [RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port

[Vladimir Oltean]

On Wed, Feb 03, 2021 at 10:27:02AM +0100, Tobias Waldekranz wrote:
> On Mon, Feb 01, 2021 at 14:24, DENG Qingfang <dqfext@gmail.com> wrote:
> > I've tested your patch series on kernel 5.4 and found that it only works
> > when VLAN filtering is enabled.
> > After some debugging, I noticed DSA will add static entries to ATU 0 if
> > VLAN filtering is disabled, regardless of default_pvid of the bridge,
> > which is also the ATU# used by the bridge.
> By default, a bridge will use a default PVID of 1, even when VLAN
> filtering is disabled (nbp_vlan_init). Yet it will assign all packets to
> VLAN 0 on ingress (br_handle_frame_finish->br_allowed_ingress).
>
> The switch OTOH, will use the PVID of the port for all packets when
> 802.1Q is disabled, thus assigning all packets to VLAN 1 when VLAN
> filtering is disabled.
>
> Andrew, Vladimir: Should mv88e6xxx always set the PVID to 0 when VLAN
> filtering is disabled?

For Ocelot/Felix, after trying to fight with some other fallout caused
by a mismatch between hardware pvid and bridge pvid when vlan_filtering=0:
https://patchwork.ozlabs.org/project/netdev/patch/20201015173355.564934-1-vladimir.oltean@nxp.com/
I did this and lived happily ever after:

  commit 75e5a554c87fd48f67d1674cfd34e47e3b454fb3
  Author: Vladimir Oltean <vladimir.oltean@nxp.com>
  Date:   Sat Oct 31 12:29:10 2020 +0200

      net: mscc: ocelot: use the pvid of zero when bridged with vlan_filtering=0

      Currently, mscc_ocelot ports configure pvid=0 in standalone mode, and
      inherit the pvid from the bridge when one is present.

      When the bridge has vlan_filtering=0, the software semantics are that
      packets should be received regardless of whether there's a pvid
      configured on the ingress port or not. However, ocelot does not observe
      those semantics today.

      Moreover, changing the PVID is also a problem with vlan_filtering=0.
      We are privately remapping the VID of FDB, MDB entries to the port's
      PVID when those are VLAN-unaware (i.e. when the VID of these entries
      comes to us as 0). But we have no logic of adjusting that remapping when
      the user changes the pvid and vlan_filtering is 0. So stale entries
      would be left behind, and untagged traffic will stop matching on them.

      And even if we were to solve that, there's an even bigger problem. If
      swp0 has pvid 1, and swp1 has pvid 2, and both are under a vlan_filtering=0
      bridge, they should be able to forward traffic between one another.
      However, with ocelot they wouldn't do that.

      The simplest way of fixing this is to never configure the pvid based on
      what the bridge is asking for, when vlan_filtering is 0. Only if there
      was a VLAN that the bridge couldn't mangle, that we could use as pvid....
      So, turns out, there's 0 just for that. And for a reason: IEEE
      802.1Q-2018, page 247, Table 9-2-Reserved VID values says:

              The null VID. Indicates that the tag header contains only
              priority information; no VID is present in the frame.
              This VID value shall not be configured as a PVID or a member
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              of a VID Set, or configured in any FDB entry, or used in any
              Management operation.

      So, aren't we doing exactly what 802.1Q says not to? Well, in a way, but
      what we're doing here is just driver-level bookkeeping, all for the
      better. The fact that we're using a pvid of 0 is not observable behavior
      from the outside world: the network stack does not see the classified
      VLAN that the switch uses, in vlan_filtering=0 mode. And we're also more
      consistent with the standalone mode now.

      And now that we use the pvid of 0 in this mode, there's another advantage:
      we don't need to perform any VID remapping for FDB and MDB entries either,
      we can just use the VID of 0 that the bridge is passing to us.

      The only gotcha is that every time we change the vlan_filtering setting,
      we need to reapply the pvid (either to 0, or to the value from the bridge).
      A small side-effect visible in the patch is that ocelot_port_set_pvid
      needs to be moved above ocelot_port_vlan_filtering, so that it can be
      called from there without forward-declarations.

      Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
      Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Re: [RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port

[Tobias Waldekranz]

On Wed, Feb 03, 2021 at 12:14, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Feb 03, 2021 at 10:27:02AM +0100, Tobias Waldekranz wrote:
>> On Mon, Feb 01, 2021 at 14:24, DENG Qingfang <dqfext@gmail.com> wrote:
>> > I've tested your patch series on kernel 5.4 and found that it only works
>> > when VLAN filtering is enabled.
>> > After some debugging, I noticed DSA will add static entries to ATU 0 if
>> > VLAN filtering is disabled, regardless of default_pvid of the bridge,
>> > which is also the ATU# used by the bridge.
>> By default, a bridge will use a default PVID of 1, even when VLAN
>> filtering is disabled (nbp_vlan_init). Yet it will assign all packets to
>> VLAN 0 on ingress (br_handle_frame_finish->br_allowed_ingress).
>>
>> The switch OTOH, will use the PVID of the port for all packets when
>> 802.1Q is disabled, thus assigning all packets to VLAN 1 when VLAN
>> filtering is disabled.
>>
>> Andrew, Vladimir: Should mv88e6xxx always set the PVID to 0 when VLAN
>> filtering is disabled?
>
> For Ocelot/Felix, after trying to fight with some other fallout caused
> by a mismatch between hardware pvid and bridge pvid when vlan_filtering=0:
> https://patchwork.ozlabs.org/project/netdev/patch/20201015173355.564934-1-vladimir.oltean@nxp.com/
> I did this and lived happily ever after:

OK great, so there is precedent. I will add it to my TODO.

Thank you!