[RFC PATCH net-next 0/3] Offload learnt bridge addresses to DSA

[RFC PATCH net-next 0/3] Offload learnt bridge addresses to DSA

[Vladimir Oltean]

This small series tries to make DSA behave a bit more sanely when
bridged with "foreign" (non-DSA) interfaces. When a station A connected
to a DSA switch port needs to talk to another station B connected to a
non-DSA port through the Linux bridge, DSA must explicitly add a route
for station B towards its CPU port. It cannot rely on hardware address
learning for that.

Vladimir Oltean (3):
  net: dsa: don't use switchdev_notifier_fdb_info in
    dsa_switchdev_event_work
  net: dsa: move switchdev event implementation under the same
    switch/case statement
  net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign
    bridge neighbors

 net/dsa/dsa_priv.h |  12 ++++
 net/dsa/slave.c    | 160 +++++++++++++++++++++++++++------------------
 2 files changed, 110 insertions(+), 62 deletions(-)

-- 
2.25.1

[RFC PATCH net-next 1/3] net: dsa: don't use switchdev_notifier_fdb_info in dsa_switchdev_event_work

[Vladimir Oltean]

Currently DSA doesn't add FDB entries on the CPU port, because it only
does so through switchdev events added_by_user and associated with a
DSA net_device, and there are none of those for the CPU port.

But actually FDB entries towards the CPU port make sense for some use
cases where certain addresses need to be processed in software, and in
that case we need to call dsa_switchdev_event_work.

There is just one problem with the existing code: it passes a structure
in dsa_switchdev_event_work which was retrieved directly from switchdev,
so it contains a net_device. We need to generalize the contents to
something that covers the CPU port as well: the "ds, port" tuple is fine
for that.

Note that the new procedure for notifying the successful FDB offload is
inspired from the rocker model.

Also, nothing was being done if added_by_user was false. Let's check for
that a lot earlier, and don't actually bother to schedule the whole
workqueue for nothing.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/dsa_priv.h |  12 ++++++
 net/dsa/slave.c    | 100 ++++++++++++++++++++++-----------------------
 2 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 12998bf04e55..03671ed984a1 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -73,6 +73,18 @@ struct dsa_notifier_mtu_info {
 	int mtu;
 };
 
+struct dsa_switchdev_event_work {
+	struct dsa_switch *ds;
+	int port;
+	struct work_struct work;
+	unsigned long event;
+	/* Specific for SWITCHDEV_FDB_ADD_TO_DEVICE and
+	 * SWITCHDEV_FDB_DEL_TO_DEVICE
+	 */
+	unsigned char addr[ETH_ALEN];
+	u16 vid;
+};
+
 struct dsa_slave_priv {
 	/* Copy of CPU port xmit for faster access in slave transmit hot path */
 	struct sk_buff *	(*xmit)(struct sk_buff *skb,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 59c80052e950..30db8230e30b 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2062,72 +2062,62 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-struct dsa_switchdev_event_work {
-	struct work_struct work;
-	struct switchdev_notifier_fdb_info fdb_info;
-	struct net_device *dev;
-	unsigned long event;
-};
+static void
+dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work)
+{
+	struct dsa_switch *ds = switchdev_work->ds;
+	struct dsa_port *dp = dsa_to_port(ds, switchdev_work->port);
+	struct switchdev_notifier_fdb_info info;
+
+	if (!dsa_is_user_port(ds, dp->index))
+		return;
+
+	info.addr = switchdev_work->addr;
+	info.vid = switchdev_work->vid;
+	info.offloaded = true;
+	call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED,
+				 dp->slave, &info.info, NULL);
+}
 
 static void dsa_slave_switchdev_event_work(struct work_struct *work)
 {
 	struct dsa_switchdev_event_work *switchdev_work =
 		container_of(work, struct dsa_switchdev_event_work, work);
-	struct net_device *dev = switchdev_work->dev;
-	struct switchdev_notifier_fdb_info *fdb_info;
-	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = switchdev_work->ds;
+	struct dsa_port *dp;
 	int err;
 
+	dp = dsa_to_port(ds, switchdev_work->port);
+
 	rtnl_lock();
 	switch (switchdev_work->event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
-		fdb_info = &switchdev_work->fdb_info;
-		if (!fdb_info->added_by_user)
-			break;
-
-		err = dsa_port_fdb_add(dp, fdb_info->addr, fdb_info->vid);
+		err = dsa_port_fdb_add(dp, switchdev_work->addr,
+				       switchdev_work->vid);
 		if (err) {
-			netdev_dbg(dev, "fdb add failed err=%d\n", err);
+			dev_dbg(ds->dev, "port %d fdb add failed err=%d\n",
+				dp->index, err);
 			break;
 		}
-		fdb_info->offloaded = true;
-		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
-					 &fdb_info->info, NULL);
+		dsa_fdb_offload_notify(switchdev_work);
 		break;
 
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
-		fdb_info = &switchdev_work->fdb_info;
-		if (!fdb_info->added_by_user)
-			break;
-
-		err = dsa_port_fdb_del(dp, fdb_info->addr, fdb_info->vid);
+		err = dsa_port_fdb_del(dp, switchdev_work->addr,
+				       switchdev_work->vid);
 		if (err) {
-			netdev_dbg(dev, "fdb del failed err=%d\n", err);
-			dev_close(dev);
+			dev_dbg(ds->dev, "port %d fdb del failed err=%d\n",
+				dp->index, err);
+			if (dsa_is_user_port(ds, dp->index))
+				dev_close(dp->slave);
 		}
 		break;
 	}
 	rtnl_unlock();
 
-	kfree(switchdev_work->fdb_info.addr);
 	kfree(switchdev_work);
-	dev_put(dev);
-}
-
-static int
-dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work *
-				  switchdev_work,
-				  const struct switchdev_notifier_fdb_info *
-				  fdb_info)
-{
-	memcpy(&switchdev_work->fdb_info, fdb_info,
-	       sizeof(switchdev_work->fdb_info));
-	switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
-	if (!switchdev_work->fdb_info.addr)
-		return -ENOMEM;
-	ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
-			fdb_info->addr);
-	return 0;
+	if (dsa_is_user_port(ds, dp->index))
+		dev_put(dp->slave);
 }
 
 /* Called under rcu_read_lock() */
@@ -2135,7 +2125,9 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 				     unsigned long event, void *ptr)
 {
 	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
+	const struct switchdev_notifier_fdb_info *fdb_info;
 	struct dsa_switchdev_event_work *switchdev_work;
+	struct dsa_port *dp;
 	int err;
 
 	if (event == SWITCHDEV_PORT_ATTR_SET) {
@@ -2148,20 +2140,32 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 	if (!dsa_slave_dev_check(dev))
 		return NOTIFY_DONE;
 
+	dp = dsa_slave_to_port(dev);
+
 	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
 	if (!switchdev_work)
 		return NOTIFY_BAD;
 
 	INIT_WORK(&switchdev_work->work,
 		  dsa_slave_switchdev_event_work);
-	switchdev_work->dev = dev;
+	switchdev_work->ds = dp->ds;
+	switchdev_work->port = dp->index;
 	switchdev_work->event = event;
 
 	switch (event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
-		if (dsa_slave_switchdev_fdb_work_init(switchdev_work, ptr))
-			goto err_fdb_work_init;
+		fdb_info = ptr;
+
+		if (!fdb_info->added_by_user) {
+			kfree(switchdev_work);
+			return NOTIFY_OK;
+		}
+
+		ether_addr_copy(switchdev_work->addr,
+				fdb_info->addr);
+		switchdev_work->vid = fdb_info->vid;
+
 		dev_hold(dev);
 		break;
 	default:
@@ -2171,10 +2175,6 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 
 	dsa_schedule_work(&switchdev_work->work);
 	return NOTIFY_OK;
-
-err_fdb_work_init:
-	kfree(switchdev_work);
-	return NOTIFY_BAD;
 }
 
 static int dsa_slave_switchdev_blocking_event(struct notifier_block *unused,
-- 
2.25.1

Re: [RFC PATCH net-next 1/3] net: dsa: don't use switchdev_notifier_fdb_info in dsa_switchdev_event_work

[Vladimir Oltean]

On Sun, Nov 08, 2020 at 03:19:51PM +0200, Vladimir Oltean wrote:
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 59c80052e950..30db8230e30b 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2062,72 +2062,62 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> -struct dsa_switchdev_event_work {
> -	struct work_struct work;
> -	struct switchdev_notifier_fdb_info fdb_info;
> -	struct net_device *dev;
> -	unsigned long event;
> -};
> +static void
> +dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work)
> +{
> +	struct dsa_switch *ds = switchdev_work->ds;
> +	struct dsa_port *dp = dsa_to_port(ds, switchdev_work->port);
> +	struct switchdev_notifier_fdb_info info;
> +
> +	if (!dsa_is_user_port(ds, dp->index))
> +		return;
> +
> +	info.addr = switchdev_work->addr;
> +	info.vid = switchdev_work->vid;
> +	info.offloaded = true;
> +	call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED,
> +				 dp->slave, &info.info, NULL);
> +}
>  
>  static void dsa_slave_switchdev_event_work(struct work_struct *work)
>  {
>  	struct dsa_switchdev_event_work *switchdev_work =
>  		container_of(work, struct dsa_switchdev_event_work, work);
> -	struct net_device *dev = switchdev_work->dev;
> -	struct switchdev_notifier_fdb_info *fdb_info;
> -	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	struct dsa_switch *ds = switchdev_work->ds;
> +	struct dsa_port *dp;
>  	int err;
>  
> +	dp = dsa_to_port(ds, switchdev_work->port);
> +
>  	rtnl_lock();
>  	switch (switchdev_work->event) {
>  	case SWITCHDEV_FDB_ADD_TO_DEVICE:
> -		fdb_info = &switchdev_work->fdb_info;
> -		if (!fdb_info->added_by_user)
> -			break;
> -
> -		err = dsa_port_fdb_add(dp, fdb_info->addr, fdb_info->vid);
> +		err = dsa_port_fdb_add(dp, switchdev_work->addr,
> +				       switchdev_work->vid);
>  		if (err) {
> -			netdev_dbg(dev, "fdb add failed err=%d\n", err);
> +			dev_dbg(ds->dev, "port %d fdb add failed err=%d\n",
> +				dp->index, err);
>  			break;
>  		}
> -		fdb_info->offloaded = true;
> -		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
> -					 &fdb_info->info, NULL);
> +		dsa_fdb_offload_notify(switchdev_work);
>  		break;
>  
>  	case SWITCHDEV_FDB_DEL_TO_DEVICE:
> -		fdb_info = &switchdev_work->fdb_info;
> -		if (!fdb_info->added_by_user)
> -			break;
> -
> -		err = dsa_port_fdb_del(dp, fdb_info->addr, fdb_info->vid);
> +		err = dsa_port_fdb_del(dp, switchdev_work->addr,
> +				       switchdev_work->vid);
>  		if (err) {
> -			netdev_dbg(dev, "fdb del failed err=%d\n", err);
> -			dev_close(dev);
> +			dev_dbg(ds->dev, "port %d fdb del failed err=%d\n",
> +				dp->index, err);
> +			if (dsa_is_user_port(ds, dp->index))
> +				dev_close(dp->slave);

Not sure that this dev_close() serves any real purpose, it stands in the
way a little. It was introduced "to indicate inconsistent situation".

commit c9eb3e0f870105242a15a5e628ed202cf32afe0d
Author: Arkadi Sharshevsky <arkadis@mellanox.com>
Date:   Sun Aug 6 16:15:42 2017 +0300

    net: dsa: Add support for learning FDB through notification

    Add support for learning FDB through notification. The driver defers
    the hardware update via ordered work queue. In case of a successful
    FDB add a notification is sent back to bridge.

    In case of hw FDB del failure the static FDB will be deleted from
    the bridge, thus, the interface is moved to down state in order to
    indicate inconsistent situation.

I hope it's ok to only close a net device if it exists, I can't think of
anything smarter.

>  		}
>  		break;
>  	}
>  	rtnl_unlock();
>  
> -	kfree(switchdev_work->fdb_info.addr);
>  	kfree(switchdev_work);
> -	dev_put(dev);
> -}
> -
> -static int
> -dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work *
> -				  switchdev_work,
> -				  const struct switchdev_notifier_fdb_info *
> -				  fdb_info)
> -{
> -	memcpy(&switchdev_work->fdb_info, fdb_info,
> -	       sizeof(switchdev_work->fdb_info));
> -	switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
> -	if (!switchdev_work->fdb_info.addr)
> -		return -ENOMEM;
> -	ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
> -			fdb_info->addr);
> -	return 0;
> +	if (dsa_is_user_port(ds, dp->index))
> +		dev_put(dp->slave);
>  }

The reference counting is broken here. It doesn't line up with the
dev_hold(dev) done in the last patch, which is on a non-DSA interface.
Anyway I think a net_device refcount is way too much for what we need
here, which is to ensure that DSA doesn't get unbound. I think I'll just
simplify to get_device(ds->dev) and put_device(ds->dev).

[RFC PATCH net-next 2/3] net: dsa: move switchdev event implementation under the same switch/case statement

[Vladimir Oltean]

We'll need to start listening to SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE
events even for interfaces where dsa_slave_dev_check returns false, so
we need that check inside the switch-case statement for SWITCHDEV_FDB_*.

This movement also avoids two useless allocation / free paths for
switchdev_work, which were difficult to avoid before, due to the code's
structure:
- on the untreated "default event" case.
- on the case where fdb_info->added_by_user is false.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/slave.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 30db8230e30b..b34da39722c7 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2130,50 +2130,45 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 	struct dsa_port *dp;
 	int err;
 
-	if (event == SWITCHDEV_PORT_ATTR_SET) {
+	switch (event) {
+	case SWITCHDEV_PORT_ATTR_SET:
 		err = switchdev_handle_port_attr_set(dev, ptr,
 						     dsa_slave_dev_check,
 						     dsa_slave_port_attr_set);
 		return notifier_from_errno(err);
-	}
-
-	if (!dsa_slave_dev_check(dev))
-		return NOTIFY_DONE;
-
-	dp = dsa_slave_to_port(dev);
-
-	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
-	if (!switchdev_work)
-		return NOTIFY_BAD;
-
-	INIT_WORK(&switchdev_work->work,
-		  dsa_slave_switchdev_event_work);
-	switchdev_work->ds = dp->ds;
-	switchdev_work->port = dp->index;
-	switchdev_work->event = event;
-
-	switch (event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
 		fdb_info = ptr;
 
-		if (!fdb_info->added_by_user) {
-			kfree(switchdev_work);
+		if (!dsa_slave_dev_check(dev))
+			return NOTIFY_DONE;
+
+		if (!fdb_info->added_by_user)
 			return NOTIFY_OK;
-		}
+
+		dp = dsa_slave_to_port(dev);
+
+		switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
+		if (!switchdev_work)
+			return NOTIFY_BAD;
+
+		INIT_WORK(&switchdev_work->work,
+			  dsa_slave_switchdev_event_work);
+		switchdev_work->ds = dp->ds;
+		switchdev_work->port = dp->index;
+		switchdev_work->event = event;
 
 		ether_addr_copy(switchdev_work->addr,
 				fdb_info->addr);
 		switchdev_work->vid = fdb_info->vid;
 
 		dev_hold(dev);
+		dsa_schedule_work(&switchdev_work->work);
 		break;
 	default:
-		kfree(switchdev_work);
 		return NOTIFY_DONE;
 	}
 
-	dsa_schedule_work(&switchdev_work->work);
 	return NOTIFY_OK;
 }
 
-- 
2.25.1

Re: [RFC PATCH net-next 2/3] net: dsa: move switchdev event implementation under the same switch/case statement

[Florian Fainelli]

On 11/8/2020 5:19 AM, Vladimir Oltean wrote:
> We'll need to start listening to SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE
> events even for interfaces where dsa_slave_dev_check returns false, so
> we need that check inside the switch-case statement for SWITCHDEV_FDB_*.
> 
> This movement also avoids two useless allocation / free paths for
> switchdev_work, which were difficult to avoid before, due to the code's
> structure:
> - on the untreated "default event" case.
> - on the case where fdb_info->added_by_user is false.
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Vladimir Oltean]

Some DSA switches (and not only) cannot learn source MAC addresses from
packets injected from the CPU. They only perform hardware address
learning from inbound traffic.

This can be problematic when we have a bridge spanning some DSA switch
ports and some non-DSA ports (which we'll call "foreign interfaces" from
DSA's perspective).

There are 2 classes of problems created by the lack of learning on
CPU-injected traffic:
- excessive flooding, due to the fact that DSA treats those addresses as
  unknown
- the risk of stale routes, which can lead to temporary packet loss

To illustrate the second class, consider the following situation, which
is common in production equipment (wireless access points, where there
is a WLAN interface and an Ethernet switch, and these form a single
bridging domain).

 AP 1:
 +------------------------------------------------------------------------+
 |                                          br0                           |
 +------------------------------------------------------------------------+
 +------------+ +------------+ +------------+ +------------+ +------------+
 |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
 +------------+ +------------+ +------------+ +------------+ +------------+
       |                                                       ^        ^
       |                                                       |        |
       |                                                       |        |
       |                                                    Client A  Client B
       |
       |
       |
 +------------+ +------------+ +------------+ +------------+ +------------+
 |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
 +------------+ +------------+ +------------+ +------------+ +------------+
 +------------------------------------------------------------------------+
 |                                          br0                           |
 +------------------------------------------------------------------------+
 AP 2

- br0 of AP 1 will know that Clients A and B are reachable via wlan0
- the hardware fdb of a DSA switch driver today is not kept in sync with
  the software entries on other bridge ports, so it will not know that
  clients A and B are reachable via the CPU port UNLESS the hardware
  switch itself performs SA learning from traffic injected from the CPU.
  Nonetheless, a substantial number of switches don't.
- the hardware fdb of the DSA switch on AP 2 may autonomously learn that
  Client A and B are reachable through swp0. Therefore, the software br0
  of AP 2 also may or may not learn this. In the example we're
  illustrating, some Ethernet traffic has been going on, and br0 from AP
  2 has indeed learnt that it can reach Client B through swp0.

One of the wireless clients, say Client B, disconnects from AP 1 and
roams to AP 2. The topology now looks like this:

 AP 1:
 +------------------------------------------------------------------------+
 |                                          br0                           |
 +------------------------------------------------------------------------+
 +------------+ +------------+ +------------+ +------------+ +------------+
 |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
 +------------+ +------------+ +------------+ +------------+ +------------+
       |                                                            ^
       |                                                            |
       |                                                         Client A
       |
       |
       |                                                         Client B
       |                                                            |
       |                                                            v
 +------------+ +------------+ +------------+ +------------+ +------------+
 |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
 +------------+ +------------+ +------------+ +------------+ +------------+
 +------------------------------------------------------------------------+
 |                                          br0                           |
 +------------------------------------------------------------------------+
 AP 2

- br0 of AP 1 still knows that Client A is reachable via wlan0 (no change)
- br0 of AP 1 will (possibly) know that Client B has left wlan0. There
  are cases where it might never find out though. Either way, DSA today
  does not process that notification in any way.
- the hardware FDB of the DSA switch on AP 1 may learn autonomously that
  Client B can be reached via swp0, if it receives any packet with
  Client 1's source MAC address over Ethernet.
- the hardware FDB of the DSA switch on AP 2 still thinks that Client B
  can be reached via swp0. It does not know that it has roamed to wlan0,
  because it doesn't perform SA learning from the CPU port.

Now Client A contacts Client B.
AP 1 routes the packet fine towards swp0 and delivers it on the Ethernet
segment.
AP 2 sees a frame on swp0 and its fdb says that the destination is swp0.
Hairpinning is disabled => drop.

This problem comes from the fact that these switches have a 'blind spot'
for addresses coming from software bridging. The generic solution is not
to assume that hardware learning can be enabled somehow, but to listen
to more bridge learning events. It turns out that the bridge driver does
learn in software from all inbound frames, in __br_handle_local_finish.
A proper SWITCHDEV_FDB_ADD_TO_DEVICE notification is emitted for the
addresses serviced by the bridge on 'foreign' interfaces. The problem is
that DSA currently only cares about SWITCHDEV_FDB_ADD_TO_DEVICE events
received on its own interfaces, such as static FDB entries.

Luckily we can change that, and DSA can listen to all switchdev FDB
add/del events in the system and figure out if those events were emitted
by a bridge that spans at least one of DSA's own ports. In case that is
true, DSA will also offload that address towards its own CPU port, in
the eventuality that there might be bridge clients attached to the DSA
switch who want to talk to the station connected to the foreign
interface.

Reported-by: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/slave.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index b34da39722c7..5b3b07a39105 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2120,6 +2120,28 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
 		dev_put(dp->slave);
 }
 
+static int dsa_lower_dev_walk(struct net_device *lower_dev,
+			      struct netdev_nested_priv *priv)
+{
+	if (dsa_slave_dev_check(lower_dev)) {
+		priv->data = netdev_priv(lower_dev);
+		return 1;
+	}
+
+	return 0;
+}
+
+struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
+{
+	struct netdev_nested_priv priv = {
+		.data = NULL,
+	};
+
+	netdev_walk_all_lower_dev_rcu(dev, dsa_lower_dev_walk, &priv);
+
+	return priv.data;
+}
+
 /* Called under rcu_read_lock() */
 static int dsa_slave_switchdev_event(struct notifier_block *unused,
 				     unsigned long event, void *ptr)
@@ -2140,13 +2162,32 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
 		fdb_info = ptr;
 
-		if (!dsa_slave_dev_check(dev))
-			return NOTIFY_DONE;
+		if (dsa_slave_dev_check(dev)) {
+			if (!fdb_info->added_by_user)
+				return NOTIFY_OK;
+
+			dp = dsa_slave_to_port(dev);
+		} else {
+			/* Snoop addresses learnt on foreign interfaces
+			 * bridged with us, for switches that don't
+			 * automatically learn SA from CPU-injected traffic
+			 */
+			struct net_device *br_dev;
+			struct dsa_slave_priv *p;
 
-		if (!fdb_info->added_by_user)
-			return NOTIFY_OK;
+			br_dev = netdev_master_upper_dev_get_rcu(dev);
+			if (!br_dev)
+				return NOTIFY_DONE;
 
-		dp = dsa_slave_to_port(dev);
+			if (!netif_is_bridge_master(br_dev))
+				return NOTIFY_DONE;
+
+			p = dsa_slave_dev_lower_find(br_dev);
+			if (!p)
+				return NOTIFY_DONE;
+
+			dp = p->dp->cpu_dp;
+		}
 
 		switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
 		if (!switchdev_work)
-- 
2.25.1

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[DENG Qingfang]

On Sun, Nov 8, 2020 at 9:20 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Some DSA switches (and not only) cannot learn source MAC addresses from
> packets injected from the CPU. They only perform hardware address
> learning from inbound traffic.
>
> This can be problematic when we have a bridge spanning some DSA switch
> ports and some non-DSA ports (which we'll call "foreign interfaces" from
> DSA's perspective).
>
> There are 2 classes of problems created by the lack of learning on
> CPU-injected traffic:
> - excessive flooding, due to the fact that DSA treats those addresses as
>   unknown
> - the risk of stale routes, which can lead to temporary packet loss
>
> To illustrate the second class, consider the following situation, which
> is common in production equipment (wireless access points, where there
> is a WLAN interface and an Ethernet switch, and these form a single
> bridging domain).
>
>  AP 1:
>  +------------------------------------------------------------------------+
>  |                                          br0                           |
>  +------------------------------------------------------------------------+
>  +------------+ +------------+ +------------+ +------------+ +------------+
>  |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
>  +------------+ +------------+ +------------+ +------------+ +------------+
>        |                                                       ^        ^
>        |                                                       |        |
>        |                                                       |        |
>        |                                                    Client A  Client B
>        |
>        |
>        |
>  +------------+ +------------+ +------------+ +------------+ +------------+
>  |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
>  +------------+ +------------+ +------------+ +------------+ +------------+
>  +------------------------------------------------------------------------+
>  |                                          br0                           |
>  +------------------------------------------------------------------------+
>  AP 2
>
> - br0 of AP 1 will know that Clients A and B are reachable via wlan0
> - the hardware fdb of a DSA switch driver today is not kept in sync with
>   the software entries on other bridge ports, so it will not know that
>   clients A and B are reachable via the CPU port UNLESS the hardware
>   switch itself performs SA learning from traffic injected from the CPU.
>   Nonetheless, a substantial number of switches don't.
> - the hardware fdb of the DSA switch on AP 2 may autonomously learn that
>   Client A and B are reachable through swp0. Therefore, the software br0
>   of AP 2 also may or may not learn this. In the example we're
>   illustrating, some Ethernet traffic has been going on, and br0 from AP
>   2 has indeed learnt that it can reach Client B through swp0.
>
> One of the wireless clients, say Client B, disconnects from AP 1 and
> roams to AP 2. The topology now looks like this:
>
>  AP 1:
>  +------------------------------------------------------------------------+
>  |                                          br0                           |
>  +------------------------------------------------------------------------+
>  +------------+ +------------+ +------------+ +------------+ +------------+
>  |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
>  +------------+ +------------+ +------------+ +------------+ +------------+
>        |                                                            ^
>        |                                                            |
>        |                                                         Client A
>        |
>        |
>        |                                                         Client B
>        |                                                            |
>        |                                                            v
>  +------------+ +------------+ +------------+ +------------+ +------------+
>  |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
>  +------------+ +------------+ +------------+ +------------+ +------------+
>  +------------------------------------------------------------------------+
>  |                                          br0                           |
>  +------------------------------------------------------------------------+
>  AP 2
>
> - br0 of AP 1 still knows that Client A is reachable via wlan0 (no change)
> - br0 of AP 1 will (possibly) know that Client B has left wlan0. There
>   are cases where it might never find out though. Either way, DSA today
>   does not process that notification in any way.
> - the hardware FDB of the DSA switch on AP 1 may learn autonomously that
>   Client B can be reached via swp0, if it receives any packet with
>   Client 1's source MAC address over Ethernet.
> - the hardware FDB of the DSA switch on AP 2 still thinks that Client B
>   can be reached via swp0. It does not know that it has roamed to wlan0,
>   because it doesn't perform SA learning from the CPU port.
>
> Now Client A contacts Client B.
> AP 1 routes the packet fine towards swp0 and delivers it on the Ethernet
> segment.
> AP 2 sees a frame on swp0 and its fdb says that the destination is swp0.
> Hairpinning is disabled => drop.
>
> This problem comes from the fact that these switches have a 'blind spot'
> for addresses coming from software bridging. The generic solution is not
> to assume that hardware learning can be enabled somehow, but to listen
> to more bridge learning events. It turns out that the bridge driver does
> learn in software from all inbound frames, in __br_handle_local_finish.
> A proper SWITCHDEV_FDB_ADD_TO_DEVICE notification is emitted for the
> addresses serviced by the bridge on 'foreign' interfaces. The problem is
> that DSA currently only cares about SWITCHDEV_FDB_ADD_TO_DEVICE events
> received on its own interfaces, such as static FDB entries.
>
> Luckily we can change that, and DSA can listen to all switchdev FDB
> add/del events in the system and figure out if those events were emitted
> by a bridge that spans at least one of DSA's own ports. In case that is
> true, DSA will also offload that address towards its own CPU port, in
> the eventuality that there might be bridge clients attached to the DSA
> switch who want to talk to the station connected to the foreign
> interface.
>
> Reported-by: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  net/dsa/slave.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index b34da39722c7..5b3b07a39105 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2120,6 +2120,28 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
>                 dev_put(dp->slave);
>  }
>
> +static int dsa_lower_dev_walk(struct net_device *lower_dev,
> +                             struct netdev_nested_priv *priv)
> +{
> +       if (dsa_slave_dev_check(lower_dev)) {
> +               priv->data = netdev_priv(lower_dev);
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
> +struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
> +{
> +       struct netdev_nested_priv priv = {
> +               .data = NULL,
> +       };
> +
> +       netdev_walk_all_lower_dev_rcu(dev, dsa_lower_dev_walk, &priv);
> +
> +       return priv.data;
> +}
> +
>  /* Called under rcu_read_lock() */
>  static int dsa_slave_switchdev_event(struct notifier_block *unused,
>                                      unsigned long event, void *ptr)
> @@ -2140,13 +2162,32 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
>         case SWITCHDEV_FDB_DEL_TO_DEVICE:
>                 fdb_info = ptr;
>
> -               if (!dsa_slave_dev_check(dev))
> -                       return NOTIFY_DONE;
> +               if (dsa_slave_dev_check(dev)) {
> +                       if (!fdb_info->added_by_user)
> +                               return NOTIFY_OK;
> +
> +                       dp = dsa_slave_to_port(dev);
> +               } else {
> +                       /* Snoop addresses learnt on foreign interfaces
> +                        * bridged with us, for switches that don't
> +                        * automatically learn SA from CPU-injected traffic
> +                        */

Can it be turned off for switches that support SA learning from CPU?

> +                       struct net_device *br_dev;
> +                       struct dsa_slave_priv *p;
>
> -               if (!fdb_info->added_by_user)
> -                       return NOTIFY_OK;
> +                       br_dev = netdev_master_upper_dev_get_rcu(dev);
> +                       if (!br_dev)
> +                               return NOTIFY_DONE;
>
> -               dp = dsa_slave_to_port(dev);
> +                       if (!netif_is_bridge_master(br_dev))
> +                               return NOTIFY_DONE;
> +
> +                       p = dsa_slave_dev_lower_find(br_dev);
> +                       if (!p)
> +                               return NOTIFY_DONE;
> +
> +                       dp = p->dp->cpu_dp;
> +               }
>
>                 switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
>                 if (!switchdev_work)
> --
> 2.25.1
>

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Vladimir Oltean]

On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote:
> Can it be turned off for switches that support SA learning from CPU?

Is there a good reason I would add another property per switch and not
just do it unconditionally?

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Andrew Lunn]

On Sun, Nov 08, 2020 at 07:23:55PM +0200, Vladimir Oltean wrote:
> On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote:
> > Can it be turned off for switches that support SA learning from CPU?
> 
> Is there a good reason I would add another property per switch and not
> just do it unconditionally?

Just throwing out ideas, i've no idea if they are relevant. I wonder
if we can get into issues with fast ageing with a topology change?  We
don't have too much control over the hardware. I think some devices
just flush everything, or maybe just one port. So we have different
life times for CPU port database entries and user port database
entries?

We might also run into bugs with flushing removing static database
entries which should not be. But that would be a bug. Also, dumping
the database might run into bugs since we have not had entries for the
CPU port before.

We also need to make sure the static entries get removed correctly
when a host moves. The mv88e6xxx will not replace a static entry with
a dynamically learned one. It will probably rise an ATU violation
interrupt that frames have come in the wrong port.

What about switches which do not implement port_fdb_add? Do these
patches at least do something sensible?

	Andrew

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Vladimir Oltean]

On Mon, Nov 09, 2020 at 12:59:39AM +0100, Andrew Lunn wrote:
> On Sun, Nov 08, 2020 at 07:23:55PM +0200, Vladimir Oltean wrote:
> > On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote:
> > > Can it be turned off for switches that support SA learning from CPU?
> > 
> > Is there a good reason I would add another property per switch and not
> > just do it unconditionally?
> 
> Just throwing out ideas, i've no idea if they are relevant. I wonder
> if we can get into issues with fast ageing with a topology change?  We
> don't have too much control over the hardware. I think some devices
> just flush everything, or maybe just one port. So we have different
> life times for CPU port database entries and user port database
> entries?

A quick scan for "port_fast_age" did not find any implementers who do
not act upon the "port" argument.

> We might also run into bugs with flushing removing static database
> entries which should not be. But that would be a bug.

I can imagine that happening, when there are multiple bridges spanning a
DSA switch, each bridge also contains a "foreign" interface, and the 2
bridging domains service 2 stations that have the same MAC address. In
that case, since the fdb_add and fdb_del are not reference-counted on
the shared DSA CPU port, we would indeed trigger this bug.

I was on the fence on whether to include the reference counting patch I
have for host MDBs, and to make these addresses refcounted as well.
What do you think?

> Also, dumping the database might run into bugs since we have not had
> entries for the CPU port before.

I don't see what conditions can make this happen.

> We also need to make sure the static entries get removed correctly
> when a host moves. The mv88e6xxx will not replace a static entry with
> a dynamically learned one. It will probably rise an ATU violation
> interrupt that frames have come in the wrong port.

This is a good one. Currently every implementer of .port_fdb_add assumes
a static entry is what we want, but that is not the case here. We want
an entry that can expire or the switch can move it to a different port
when there is evidence that it's wrong. Should we add more arguments to
the API?

> What about switches which do not implement port_fdb_add? Do these
> patches at least do something sensible?

dsa_slave_switchdev_event
-> dsa_slave_switchdev_event_work
   -> dsa_port_fdb_add
      -> dsa_port_notify(DSA_NOTIFIER_FDB_ADD)
         -> dsa_switch_fdb_add
            -> if (!ds->ops->port_fdb_add) return -EOPNOTSUPP;
   -> an error is printed with dev_dbg, and
      dsa_fdb_offload_notify(switchdev_work) is not called.

On dsa_port_fdb_del error, there is also an attempt to call dev_close()
on error, but only on user ports, which the CPU port is not.

So, we do something almost sensible, but mostly by mistake it seems.

I think the simplest would be to simply avoid all this nonsense right
away in dsa_slave_switchdev_event:

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2188,6 +2188,9 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 			dp = p->dp->cpu_dp;
 		}
 
+		if (!dp->ds->ops->port_fdb_add || !dp->ds->ops->port_fdb_del)
+			return NOTIFY_DONE;
+
 		switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
 		if (!switchdev_work)
 			return NOTIFY_BAD;

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Andrew Lunn]

> > We also need to make sure the static entries get removed correctly
> > when a host moves. The mv88e6xxx will not replace a static entry with
> > a dynamically learned one. It will probably rise an ATU violation
> > interrupt that frames have come in the wrong port.
> 
> This is a good one. Currently every implementer of .port_fdb_add assumes
> a static entry is what we want, but that is not the case here. We want
> an entry that can expire or the switch can move it to a different port
> when there is evidence that it's wrong.

I doubt you will find any hardware that actually does this. I expect
there are static entries, and dynamic entries, and nothing
hybrid. After a move, we need to rely on a broadcast packet making its
way to the software bridge, which causes it to learn about the move,
and delete the static CPU entry from the hardware.

We can probably test this with having our wireless device move back
and forth a few times, so we can see the full cycle a few
times. Unfortunately, i don't have two boards with both a switch and
WiFi.

    Andrew

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Tobias Waldekranz]

On Mon, Nov 09, 2020 at 02:30, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Mon, Nov 09, 2020 at 12:59:39AM +0100, Andrew Lunn wrote:
>> We also need to make sure the static entries get removed correctly
>> when a host moves. The mv88e6xxx will not replace a static entry with
>> a dynamically learned one. It will probably rise an ATU violation
>> interrupt that frames have come in the wrong port.
>
> This is a good one. Currently every implementer of .port_fdb_add assumes
> a static entry is what we want, but that is not the case here. We want
> an entry that can expire or the switch can move it to a different port
> when there is evidence that it's wrong. Should we add more arguments to
> the API?

I don't think that would help. You would essentially be trading one
situation where station moves causes loss of traffic for another
one. But now you have also increased the background load of an already
choked resource, the MDIO bus.

At least on mv88e6xxx, your only option to allow the hardware to move
the station to another port autonomously is to add the entry as a
dynamically learnt one. However, since the switch does not perform any
SA learning on the CPU port in this world, the entry would have to be
refreshed by software, otherwise it would just age out.

Then you run in to this situation:

A and B are communicating.

       br0
  .----'|'----.
  |     |     |
swp0  swp1  wlan0
  |           |
  A           B

The switch's FDB:
A: swp0
B: cpu0 (due to this patchset)

Now B roams to an AP somewhere behind swp1 and continues to communicate
with A.

       br0
  .----'|'----.
  |     |     |
swp0  swp1  wlan0
  |     |
  A     B

The switch's FDB:
A: swp0
B: swp1

But br0 sees none of this, so at whatever interval we choose we will
refresh the FDB, moving the station back to the cpu:

A: swp0
B: cpu0

So now you have traded the issue of having to wait for the hardware to
age out its entry, to the issue of having to wait for br0 to age out its
entry. Right?

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Vladimir Oltean]

On Mon, Nov 09, 2020 at 09:09:37AM +0100, Tobias Waldekranz wrote:
> On Mon, Nov 09, 2020 at 02:30, Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Mon, Nov 09, 2020 at 12:59:39AM +0100, Andrew Lunn wrote:
> >> We also need to make sure the static entries get removed correctly
> >> when a host moves. The mv88e6xxx will not replace a static entry with
> >> a dynamically learned one. It will probably rise an ATU violation
> >> interrupt that frames have come in the wrong port.
> >
> > This is a good one. Currently every implementer of .port_fdb_add assumes
> > a static entry is what we want, but that is not the case here. We want
> > an entry that can expire or the switch can move it to a different port
> > when there is evidence that it's wrong. Should we add more arguments to
> > the API?
>
> I don't think that would help. You would essentially be trading one
> situation where station moves causes loss of traffic for another
> one. But now you have also increased the background load of an already
> choked resource, the MDIO bus.

In practice, DSA switches are already very demanding of their management
interface throughput, for PTP and things like that. I do expect that if
you spent any significant amount of time with DSA, you already know the
ins and outs of your MDIO/SPI/I2C controller and it would already be
optimized for efficiency. But ok, we can add this to the list of cons.

> At least on mv88e6xxx, your only option to allow the hardware to move
> the station to another port autonomously is to add the entry as a
> dynamically learnt one. However, since the switch does not perform any
> SA learning on the CPU port in this world, the entry would have to be
> refreshed by software, otherwise it would just age out.
>
> Then you run in to this situation:
>
> A and B are communicating.
>
>        br0
>   .----'|'----.
>   |     |     |
> swp0  swp1  wlan0
>   |           |
>   A           B
>
> The switch's FDB:
> A: swp0
> B: cpu0 (due to this patchset)
>
> Now B roams to an AP somewhere behind swp1 and continues to communicate
> with A.
>
>        br0
>   .----'|'----.
>   |     |     |
> swp0  swp1  wlan0
>   |     |
>   A     B
>
> The switch's FDB:
> A: swp0
> B: swp1
>
> But br0 sees none of this, so at whatever interval we choose we will
> refresh the FDB, moving the station back to the cpu:
>
> A: swp0
> B: cpu0

No, br0 should see some traffic from station B. Not the unicast traffic
towards station A, of course (because that has already been learnt to go
towards swp0), but some broadcast ARP, or some multicast ND. This is the
big assumption behind any solution: that the stations are not silent and
make their presence known somehow.

> So now you have traded the issue of having to wait for the hardware to
> age out its entry, to the issue of having to wait for br0 to age out its
> entry. Right?

That's the thing.
The software bridge will never expire its entry in br_fdb_update if
traffic is continuously coming in. fdb->updated will just keep getting
larger and larger after each incoming packet. But the hardware bridge is
not aware of this traffic. So:
- if the hardware bridge has a dynamic entry installed (one that's
  subject to ageing), that entry will eventually expire within 5 minutes
  when its software equivalent won't. Then no switchdev event will ever
  come back to update the hardware bridge, since from the software's
  perspective it was never supposed to expire. It's as if we _do_ want
  the entry to be static. But:
- if the hardware bridge has a static entry installed, then that entry
  might become wrong and cause connectivity loss until the software
  bridge figures it out.
It's what Andrew described as a 'hybrid' entry. We would want a 'static'
entry (one that doesn't age out based on a timer) that is 'weak' (can be
overridden when traffic comes in on a different port). I'm not sure
either that such thing exists.

So for now, static entries are the best we've got. Let's re-run the
simulation knowing that we're working with static addresses towards the
CPU, to see how bad things are.

 AP 1:
 +------------------------------------------------------------------------+
 |                                          br0                           |
 +------------------------------------------------------------------------+
 +------------+ +------------+ +------------+ +------------+ +------------+
 |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
 +------------+ +------------+ +------------+ +------------+ +------------+
       |                                                       ^        ^
       |                                                       |        |
       |                                                       |        |
       |                                                    Client A  Client B
       |
       |
       |
 +------------+ +------------+ +------------+ +------------+ +------------+
 |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
 +------------+ +------------+ +------------+ +------------+ +------------+
 +------------------------------------------------------------------------+
 |                                          br0                           |
 +------------------------------------------------------------------------+
 AP 2

- br0 of AP 1 will lean that Clients A and B are reachable via wlan0.
  The DSA switch will snoop these and add static entries towards the CPU
  port.
- the hardware fdb of the DSA switch, as well as br0 on AP 2, will learn
  that Clients A and B are reachable through swp0, because of our
  assumption of non-silent stations. There are no static entries
  involved on AP 2 for now.

Client B disconnects from AP 1 and roams to AP 2.

 AP 1:
 +------------------------------------------------------------------------+
 |                                          br0                           |
 +------------------------------------------------------------------------+
 +------------+ +------------+ +------------+ +------------+ +------------+
 |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
 +------------+ +------------+ +------------+ +------------+ +------------+
       |                                                            ^
       |                                                            |
       |                                                         Client A
       |
       |
       |                                                         Client B
       |                                                            |
       |                                                            v
 +------------+ +------------+ +------------+ +------------+ +------------+
 |    swp0    | |    swp1    | |    swp2    | |    swp3    | |    wlan0   |
 +------------+ +------------+ +------------+ +------------+ +------------+
 +------------------------------------------------------------------------+
 |                                          br0                           |
 +------------------------------------------------------------------------+
 AP 2

- br0 of AP 1 still knows that Client A is reachable via wlan0 (no change)
- In the general case, br0 of AP 1 will _NOT_ know that Client B has
  left wlan0. So there is still a static entry for Client B towards the
  CPU port.
- Right now, any attempt from Client A to directly address Client B via
  unicast would result, if the FDB were to be consulted, in packet
  drops, because the switch on AP 1 would say 'wait a minute, I'm
  receiving a packet for Client B from the CPU port, but Client B is
  reachable via the CPU port!'. Luckily for us, the switches that we're
  working with are not looking up the FDB for CPU injected traffic,
  remember? So I don't think this is a problem. So unicast packets would
  be delivered to anywhere that the software bridge wanted to. Right
  now, even the software bridge has a wrong impression of where Client B
  is.
- remember the assumption that Client B is not silent at startup. So
  some broadcast packets with Client B's source MAC address will reach
  the Ethernet segment. The hardware switch on AP 1 will have no problem
  accepting these packets, since they are broadcast/multicast. They will
  reach the software bridge. At this point, the software bridge finally
  learns the new destination for Client B, and it emits a new
  SWITCHDEV_FDB_ADD_TO_DEVICE event. Today we ignore that, because
  added_by_user will be false. That's what we do wrong/incomplete in
  this RFC patch set. We should keep track of static addresses installed
  on the CPU port, and if we ever receive a !added_by_user notification
  on a DSA switch port for one of those addresses, we should update the
  hardware FDB of the DSA switch on AP 1.

So there you have it, it's not that bad. More work needs to be done, but
IMO it's still workable.

But now maybe it makes more sense to treat the switches that perform
hardware SA learning on the CPU port separately, after I've digested
this a bit.

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Tobias Waldekranz]

On Mon, Nov 09, 2020 at 12:03, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Mon, Nov 09, 2020 at 09:09:37AM +0100, Tobias Waldekranz wrote:
>> one. But now you have also increased the background load of an already
>> choked resource, the MDIO bus.
>
> In practice, DSA switches are already very demanding of their management
> interface throughput, for PTP and things like that. I do expect that if
> you spent any significant amount of time with DSA, you already know the
> ins and outs of your MDIO/SPI/I2C controller and it would already be
> optimized for efficiency. But ok, we can add this to the list of cons.

You are arguing for my position though, no? Yes it is demanding; that is
why we must allocate it carefully.

> So there you have it, it's not that bad. More work needs to be done, but
> IMO it's still workable.

If you bypass learning on all frames sent from the CPU (as today), yes I
agree that you should be able to solve it with static entries. But I
think that you will have lots of weird problems with initial packet loss
as the FDB updates are not synchronous with the packet flow. I.e. the
bridge will tell DSA to update the entry, but the update in HW will
occur some time later when the workqueue actually performs the
operation.

> But now maybe it makes more sense to treat the switches that perform
> hardware SA learning on the CPU port separately, after I've digested
> this a bit.

Yes, please. Because it will be impossible to add tx forward offloading
otherwise.

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Vladimir Oltean]

On Mon, Nov 09, 2020 at 12:05:19PM +0100, Tobias Waldekranz wrote:
> On Mon, Nov 09, 2020 at 12:03, Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Mon, Nov 09, 2020 at 09:09:37AM +0100, Tobias Waldekranz wrote:
> >> one. But now you have also increased the background load of an already
> >> choked resource, the MDIO bus.
> >
> > In practice, DSA switches are already very demanding of their management
> > interface throughput, for PTP and things like that. I do expect that if
> > you spent any significant amount of time with DSA, you already know the
> > ins and outs of your MDIO/SPI/I2C controller and it would already be
> > optimized for efficiency. But ok, we can add this to the list of cons.
>
> You are arguing for my position though, no? Yes it is demanding; that is
> why we must allocate it carefully.

Yes, if the change brings additional load to the MDIO/SPI/I2C link and
doesn't bring any benefit, then it makes sense to skip it.

> > So there you have it, it's not that bad. More work needs to be done, but
> > IMO it's still workable.
>
> If you bypass learning on all frames sent from the CPU (as today), yes I
> agree that you should be able to solve it with static entries. But I
> think that you will have lots of weird problems with initial packet loss
> as the FDB updates are not synchronous with the packet flow. I.e. the
> bridge will tell DSA to update the entry, but the update in HW will
> occur some time later when the workqueue actually performs the
> operation.

I don't know how bad this is in practice. It's surely better than
waiting 5 minutes though.

> > But now maybe it makes more sense to treat the switches that perform
> > hardware SA learning on the CPU port separately, after I've digested
> > this a bit.
>
> Yes, please. Because it will be impossible to add tx forward offloading
> otherwise.

Ok, so this change, when applied to mv88e6xxx, would preclude you from
using FORWARD frames for your other application of that feature, unless
you explicitly turn off SA learning for FORWARD frames coming the CPU
port, case in which you would still be ok.

I need to sit on this for a while. How many DSA drivers do we have that
don't do SA learning in hardware for CPU-injected packets? ocelot/felix
and mv88e6xxx? Who else? Because if there aren't that many (or any at
all except for these two), then I could try to spend some time and see
how Felix behaves when I send FORWARD frames to it. Then we could go on
full blast with the other alternative, to force-enable address learning
from the CPU port, and declare this one as too complicated and not worth
the effort.

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Vladimir Oltean]

On Mon, Nov 09, 2020 at 02:31:11PM +0200, Vladimir Oltean wrote:
> I need to sit on this for a while. How many DSA drivers do we have that
> don't do SA learning in hardware for CPU-injected packets? ocelot/felix
> and mv88e6xxx? Who else? Because if there aren't that many (or any at
> all except for these two), then I could try to spend some time and see
> how Felix behaves when I send FORWARD frames to it. Then we could go on
> full blast with the other alternative, to force-enable address learning
> from the CPU port, and declare this one as too complicated and not worth
> the effort.

In fact I'm not sure that I should be expecting an answer to this
question. We can evaluate the other alternative in parallel. Would you
be so kind to send some sort of RFC for your TX-side offload_fwd_mark so
that I could test with the hardware I have, and get a better understanding
of the limitations there?

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Tobias Waldekranz]

On Mon Nov 9, 2020 at 3:38 PM CET, Vladimir Oltean wrote:
> On Mon, Nov 09, 2020 at 02:31:11PM +0200, Vladimir Oltean wrote:
> > I need to sit on this for a while. How many DSA drivers do we have that
> > don't do SA learning in hardware for CPU-injected packets? ocelot/felix
> > and mv88e6xxx? Who else? Because if there aren't that many (or any at
> > all except for these two), then I could try to spend some time and see
> > how Felix behaves when I send FORWARD frames to it. Then we could go on
> > full blast with the other alternative, to force-enable address learning
> > from the CPU port, and declare this one as too complicated and not worth
> > the effort.
>
> In fact I'm not sure that I should be expecting an answer to this
> question. We can evaluate the other alternative in parallel. Would you
> be so kind to send some sort of RFC for your TX-side offload_fwd_mark so
> that I could test with the hardware I have, and get a better
> understanding
> of the limitations there?

That is the plan. I have some stuff I need to get done before
though. The current implementation is on a 4.19 kernel, so it's going
to take some time to rebase it.

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Florian Fainelli]

On 11/9/2020 4:38 AM, Vladimir Oltean wrote:
> On Mon, Nov 09, 2020 at 02:31:11PM +0200, Vladimir Oltean wrote:
>> I need to sit on this for a while. How many DSA drivers do we have that
>> don't do SA learning in hardware for CPU-injected packets? ocelot/felix
>> and mv88e6xxx? Who else? Because if there aren't that many (or any at
>> all except for these two), then I could try to spend some time and see
>> how Felix behaves when I send FORWARD frames to it. Then we could go on
>> full blast with the other alternative, to force-enable address learning
>> from the CPU port, and declare this one as too complicated and not worth
>> the effort.
> 
> In fact I'm not sure that I should be expecting an answer to this
> question. We can evaluate the other alternative in parallel. Would you
> be so kind to send some sort of RFC for your TX-side offload_fwd_mark so
> that I could test with the hardware I have, and get a better understanding
> of the limitations there?

For Broadcom switches, ARL (Address Resolution Logic, where learning
happens) is bypassed when packets ingress the CPU port with opcode 1
which is what net/dsa/tag_brcm.c uses. When opcode 0 is used, address
learning occurs.

The reason why opcode 1 is used is because of the Advanced Congestion
Buffering (ACB) which requires us to steer packets towards a particular
switch port and egress queue number within that port. With opcode 0 we
would not be able to do that.

We could make the opcode dependent on the switch/DSA master since not
all combinations support ACB, but given we have 3 or 4 Ethernet switches
kind within DSA that do not do learning from the CPU port, I guess we
need a solution to that problem somehow.
-- 
Florian

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Alexandra Winter]

On 08.11.20 18:23, Vladimir Oltean wrote:
> On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote:
>> Can it be turned off for switches that support SA learning from CPU?
> 
> Is there a good reason I would add another property per switch and not
> just do it unconditionally?
> 
I have a similar concern for a future patch, where I want to turn on or off, whether the
device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface.
(Options will be: static MACs only, learning in the device or learning in bridge and notifications to device)
What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message?

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Vladimir Oltean]

Hi Alexandra,

On Wed, Nov 11, 2020 at 11:13:03AM +0100, Alexandra Winter wrote:
> On 08.11.20 18:23, Vladimir Oltean wrote:
> > On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote:
> >> Can it be turned off for switches that support SA learning from CPU?
> > 
> > Is there a good reason I would add another property per switch and not
> > just do it unconditionally?
> > 
> I have a similar concern for a future patch, where I want to turn on or off, whether the
> device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface.
> (Options will be: static MACs only, learning in the device or learning in bridge and notifications to device)
> What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message?

My understanding is that "learning_sync" is for pushing learnt addresses
from device to bridge, not from bridge to device.

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Alexandra Winter]

On 11.11.20 11:36, Vladimir Oltean wrote:
> Hi Alexandra,
> 
> On Wed, Nov 11, 2020 at 11:13:03AM +0100, Alexandra Winter wrote:
>> On 08.11.20 18:23, Vladimir Oltean wrote:
>>> On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote:
>>>> Can it be turned off for switches that support SA learning from CPU?
>>>
>>> Is there a good reason I would add another property per switch and not
>>> just do it unconditionally?
>>>
>> I have a similar concern for a future patch, where I want to turn on or off, whether the
>> device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface.
>> (Options will be: static MACs only, learning in the device or learning in bridge and notifications to device)
>> What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message?
> 
> My understanding is that "learning_sync" is for pushing learnt addresses
> from device to bridge, not from bridge to device.
> 
uh, sorry copy-paste error. I meant:
'bridge link set dev $netdev learning on self'

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Vladimir Oltean]

On Wed, Nov 11, 2020 at 03:14:26PM +0100, Alexandra Winter wrote:
> On 11.11.20 11:36, Vladimir Oltean wrote:
> > Hi Alexandra,
> > 
> > On Wed, Nov 11, 2020 at 11:13:03AM +0100, Alexandra Winter wrote:
> >> On 08.11.20 18:23, Vladimir Oltean wrote:
> >>> On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote:
> >>>> Can it be turned off for switches that support SA learning from CPU?
> >>>
> >>> Is there a good reason I would add another property per switch and not
> >>> just do it unconditionally?
> >>>
> >> I have a similar concern for a future patch, where I want to turn on or off, whether the
> >> device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface.
> >> (Options will be: static MACs only, learning in the device or learning in bridge and notifications to device)
> >> What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message?
> > 
> > My understanding is that "learning_sync" is for pushing learnt addresses
> > from device to bridge, not from bridge to device.
> > 
> uh, sorry copy-paste error. I meant:
> 'bridge link set dev $netdev learning on self'

Even with "learning" instead of "learning_sync", I don't understand what
the "self" modifier would mean and how it would help, sorry.

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[Alexandra Winter]

On 12.11.20 14:49, Vladimir Oltean wrote:
> On Wed, Nov 11, 2020 at 03:14:26PM +0100, Alexandra Winter wrote:
>> On 11.11.20 11:36, Vladimir Oltean wrote:
>>> Hi Alexandra,
>>>
>>> On Wed, Nov 11, 2020 at 11:13:03AM +0100, Alexandra Winter wrote:
>>>> On 08.11.20 18:23, Vladimir Oltean wrote:
>>>>> On Sun, Nov 08, 2020 at 10:09:25PM +0800, DENG Qingfang wrote:
>>>>>> Can it be turned off for switches that support SA learning from CPU?
>>>>>
>>>>> Is there a good reason I would add another property per switch and not
>>>>> just do it unconditionally?
>>>>>
>>>> I have a similar concern for a future patch, where I want to turn on or off, whether the
>>>> device driver listens to SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE for a certain interface.
>>>> (Options will be: static MACs only, learning in the device or learning in bridge and notifications to device)
>>>> What about 'bridge link set dev $netdev learning_sync on self' respectively the corresponding netlink message?
>>>
>>> My understanding is that "learning_sync" is for pushing learnt addresses
>>> from device to bridge, not from bridge to device.
>>>
>> uh, sorry copy-paste error. I meant:
>> 'bridge link set dev $netdev learning on self'
> 
> Even with "learning" instead of "learning_sync", I don't understand what
> the "self" modifier would mean and how it would help, sorry.
> 
With the self modifier, the command is not executed by the linux bridge but instead sent to the device driver of the
respective bridgeport. So AFAIU 'learning on self' would mean, that instead of only the bridge doing the learning on this
bridgeport, the device itself should do the learning. Which sounds to me like a good fit for SA learning from CPU.

It seems that the self modifier is not used on hardware switches today, but rather on virtualized network cards, where
it is e.g. used ot turn VEPA mode on or off per virtual interface. In drivers/s390/net/qeth we use 'learning_sync on self',
to control whether a virtualized interface should synchronize the card's fdb with an attached linux bridge.

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB, DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]

Hi Vladimir,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Offload-learnt-bridge-addresses-to-DSA/20201109-090521
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git c9448e828d113cd7eafe77c414127e877ca88b20
config: c6x-randconfig-r015-20201109 (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/307255d137629e3c55903902af882231d1b5cb56
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vladimir-Oltean/Offload-learnt-bridge-addresses-to-DSA/20201109-090521
        git checkout 307255d137629e3c55903902af882231d1b5cb56
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/dsa/slave.c:2134:24: warning: no previous prototype for 'dsa_slave_dev_lower_find' [-Wmissing-prototypes]
    2134 | struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
         |                        ^~~~~~~~~~~~~~~~~~~~~~~~

vim +/dsa_slave_dev_lower_find +2134 net/dsa/slave.c

  2133	
> 2134	struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
  2135	{
  2136		struct netdev_nested_priv priv = {
  2137			.data = NULL,
  2138		};
  2139	
  2140		netdev_walk_all_lower_dev_rcu(dev, dsa_lower_dev_walk, &priv);
  2141	
  2142		return priv.data;
  2143	}
  2144	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 21636 bytes --]

Re: [RFC PATCH net-next 3/3] net: dsa: listen for SWITCHDEV_{FDB, DEL}_ADD_TO_DEVICE on foreign bridge neighbors

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2425 bytes --]

Hi Vladimir,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Offload-learnt-bridge-addresses-to-DSA/20201109-090521
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git c9448e828d113cd7eafe77c414127e877ca88b20
config: x86_64-randconfig-a013-20201116 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c044709b8fbea2a9a375e4173a6bd735f6866c0c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/307255d137629e3c55903902af882231d1b5cb56
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vladimir-Oltean/Offload-learnt-bridge-addresses-to-DSA/20201109-090521
        git checkout 307255d137629e3c55903902af882231d1b5cb56
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/dsa/slave.c:2134:24: warning: no previous prototype for function 'dsa_slave_dev_lower_find' [-Wmissing-prototypes]
   struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
                          ^
   net/dsa/slave.c:2134:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
   ^
   static 
   1 warning generated.

vim +/dsa_slave_dev_lower_find +2134 net/dsa/slave.c

  2133	
> 2134	struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
  2135	{
  2136		struct netdev_nested_priv priv = {
  2137			.data = NULL,
  2138		};
  2139	
  2140		netdev_walk_all_lower_dev_rcu(dev, dsa_lower_dev_walk, &priv);
  2141	
  2142		return priv.data;
  2143	}
  2144	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 41126 bytes --]