[RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[DENG Qingfang]

When a client roams from a DSA user port to a soft-bridged port (such as WiFi
interface), the left-over MAC entry in the switch HW is not deleted, causing
inconsistency between Linux fdb and the switch MAC table. As a result, the
client cannot talk to other hosts which are on that DSA user port until the
MAC entry expires.

Solve this by notifying switchdev fdb to delete the leftover entry when an
entry is updated. Remove the added_by_user check in DSA

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
I tried this on mt7530 and mv88e6xxx, but only mt7530 works.
In previous discussion[1], Andrew Lunn said "try playing with auto learning
for the CPU port" but it didn't work on mv88e6xxx either

I think commit 7e26bf45e4cb ("net: bridge: allow SW learn to take over HW fdb
entries") already tried to fix this issue..

[1] https://lore.kernel.org/netdev/20200405150915.GD161768@lunn.ch/

 net/bridge/br_fdb.c | 3 +++
 net/dsa/slave.c     | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 4877a0db16c6..46003e78f2ac 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -579,6 +579,9 @@ 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))) {
+				/* Remove the entry in HW */
+				br_switchdev_fdb_notify(fdb, RTM_DELNEIGH);
+
 				fdb->dst = source;
 				fdb_modified = true;
 				/* Take over HW learned entry */
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index e94eb1aac602..6133a1be1a74 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1932,8 +1932,6 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
 
 	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);
 		if (err) {
-- 
2.26.0

[Bridge] [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[DENG Qingfang]

When a client roams from a DSA user port to a soft-bridged port (such as WiFi
interface), the left-over MAC entry in the switch HW is not deleted, causing
inconsistency between Linux fdb and the switch MAC table. As a result, the
client cannot talk to other hosts which are on that DSA user port until the
MAC entry expires.

Solve this by notifying switchdev fdb to delete the leftover entry when an
entry is updated. Remove the added_by_user check in DSA

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
I tried this on mt7530 and mv88e6xxx, but only mt7530 works.
In previous discussion[1], Andrew Lunn said "try playing with auto learning
for the CPU port" but it didn't work on mv88e6xxx either

I think commit 7e26bf45e4cb ("net: bridge: allow SW learn to take over HW fdb
entries") already tried to fix this issue..

[1] https://lore.kernel.org/netdev/20200405150915.GD161768@lunn.ch/

 net/bridge/br_fdb.c | 3 +++
 net/dsa/slave.c     | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 4877a0db16c6..46003e78f2ac 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -579,6 +579,9 @@ 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))) {
+				/* Remove the entry in HW */
+				br_switchdev_fdb_notify(fdb, RTM_DELNEIGH);
+
 				fdb->dst = source;
 				fdb_modified = true;
 				/* Take over HW learned entry */
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index e94eb1aac602..6133a1be1a74 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1932,8 +1932,6 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
 
 	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);
 		if (err) {
-- 
2.26.0

Re: [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[Andrew Lunn]

On Mon, Apr 20, 2020 at 12:19:46AM +0800, DENG Qingfang wrote:
> When a client roams from a DSA user port to a soft-bridged port (such as WiFi
> interface), the left-over MAC entry in the switch HW is not deleted, causing
> inconsistency between Linux fdb and the switch MAC table. As a result, the
> client cannot talk to other hosts which are on that DSA user port until the
> MAC entry expires.
> 
> Solve this by notifying switchdev fdb to delete the leftover entry when an
> entry is updated. Remove the added_by_user check in DSA
> 
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---
> I tried this on mt7530 and mv88e6xxx, but only mt7530 works.
> In previous discussion[1], Andrew Lunn said "try playing with auto learning
> for the CPU port" but it didn't work on mv88e6xxx either

Hi Deng

We should probably first define how we expect moving MAC to work. Then
we can make any core fixes, and driver fixes.

For DSA, we have assumed that the software bridge and the hardware
bridge are independent, each performs its own learning. Only static
entries are kept in sync.

How should this separate learning work for a MAC address which moves?

    Andrew

Re: [Bridge] [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[Andrew Lunn]

On Mon, Apr 20, 2020 at 12:19:46AM +0800, DENG Qingfang wrote:
> When a client roams from a DSA user port to a soft-bridged port (such as WiFi
> interface), the left-over MAC entry in the switch HW is not deleted, causing
> inconsistency between Linux fdb and the switch MAC table. As a result, the
> client cannot talk to other hosts which are on that DSA user port until the
> MAC entry expires.
> 
> Solve this by notifying switchdev fdb to delete the leftover entry when an
> entry is updated. Remove the added_by_user check in DSA
> 
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---
> I tried this on mt7530 and mv88e6xxx, but only mt7530 works.
> In previous discussion[1], Andrew Lunn said "try playing with auto learning
> for the CPU port" but it didn't work on mv88e6xxx either

Hi Deng

We should probably first define how we expect moving MAC to work. Then
we can make any core fixes, and driver fixes.

For DSA, we have assumed that the software bridge and the hardware
bridge are independent, each performs its own learning. Only static
entries are kept in sync.

How should this separate learning work for a MAC address which moves?

    Andrew

Re: [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[DENG Qingfang]

On Mon, Apr 20, 2020 at 12:42 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Apr 20, 2020 at 12:19:46AM +0800, DENG Qingfang wrote:
> > When a client roams from a DSA user port to a soft-bridged port (such as WiFi
> > interface), the left-over MAC entry in the switch HW is not deleted, causing
> > inconsistency between Linux fdb and the switch MAC table. As a result, the
> > client cannot talk to other hosts which are on that DSA user port until the
> > MAC entry expires.
> >
> > Solve this by notifying switchdev fdb to delete the leftover entry when an
> > entry is updated. Remove the added_by_user check in DSA
> >
> > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > ---
> > I tried this on mt7530 and mv88e6xxx, but only mt7530 works.
> > In previous discussion[1], Andrew Lunn said "try playing with auto learning
> > for the CPU port" but it didn't work on mv88e6xxx either
>
> Hi Deng
>
> We should probably first define how we expect moving MAC to work. Then
> we can make any core fixes, and driver fixes.
>
> For DSA, we have assumed that the software bridge and the hardware
> bridge are independent, each performs its own learning. Only static
> entries are kept in sync.
>
> How should this separate learning work for a MAC address which moves?

When a client moves from a hardware port (e.g. sw0p1) to a software port (wlan0)
or another hardware port that belongs to a different switch (sw1p1),
that MAC entry
in sw0's MAC table should be deleted, or replaced with the CPU port as
destination,
by DSA. Otherwise the client is unable to talk to other hosts on sw0 because sw0
still thinks the client is on sw0p1.

Discussion in OpenWrt GitHub:
https://github.com/openwrt/openwrt/pull/2798#issuecomment-602221971


>
>     Andrew

Re: [Bridge] [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[DENG Qingfang]

On Mon, Apr 20, 2020 at 12:42 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Apr 20, 2020 at 12:19:46AM +0800, DENG Qingfang wrote:
> > When a client roams from a DSA user port to a soft-bridged port (such as WiFi
> > interface), the left-over MAC entry in the switch HW is not deleted, causing
> > inconsistency between Linux fdb and the switch MAC table. As a result, the
> > client cannot talk to other hosts which are on that DSA user port until the
> > MAC entry expires.
> >
> > Solve this by notifying switchdev fdb to delete the leftover entry when an
> > entry is updated. Remove the added_by_user check in DSA
> >
> > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > ---
> > I tried this on mt7530 and mv88e6xxx, but only mt7530 works.
> > In previous discussion[1], Andrew Lunn said "try playing with auto learning
> > for the CPU port" but it didn't work on mv88e6xxx either
>
> Hi Deng
>
> We should probably first define how we expect moving MAC to work. Then
> we can make any core fixes, and driver fixes.
>
> For DSA, we have assumed that the software bridge and the hardware
> bridge are independent, each performs its own learning. Only static
> entries are kept in sync.
>
> How should this separate learning work for a MAC address which moves?

When a client moves from a hardware port (e.g. sw0p1) to a software port (wlan0)
or another hardware port that belongs to a different switch (sw1p1),
that MAC entry
in sw0's MAC table should be deleted, or replaced with the CPU port as
destination,
by DSA. Otherwise the client is unable to talk to other hosts on sw0 because sw0
still thinks the client is on sw0p1.

Discussion in OpenWrt GitHub:
https://github.com/openwrt/openwrt/pull/2798#issuecomment-602221971


>
>     Andrew

Re: [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[Andrew Lunn]

> When a client moves from a hardware port (e.g. sw0p1) to a software port (wlan0)
> or another hardware port that belongs to a different switch (sw1p1),
> that MAC entry
> in sw0's MAC table should be deleted, or replaced with the CPU port as
> destination,
> by DSA. Otherwise the client is unable to talk to other hosts on sw0 because sw0
> still thinks the client is on sw0p1.

The MAC address needs to move, no argument there. But what are the
mechanisms which cause this. Is learning sufficient, or does DSA need
to take an active role?

Forget about DSA for the moment. How does this work for two normal
bridges? Is the flow of unicast packets sufficient? Does it requires a
broadcast packet from the device after it moves?

	  Andrew

Re: [Bridge] [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[Andrew Lunn]

> When a client moves from a hardware port (e.g. sw0p1) to a software port (wlan0)
> or another hardware port that belongs to a different switch (sw1p1),
> that MAC entry
> in sw0's MAC table should be deleted, or replaced with the CPU port as
> destination,
> by DSA. Otherwise the client is unable to talk to other hosts on sw0 because sw0
> still thinks the client is on sw0p1.

The MAC address needs to move, no argument there. But what are the
mechanisms which cause this. Is learning sufficient, or does DSA need
to take an active role?

Forget about DSA for the moment. How does this work for two normal
bridges? Is the flow of unicast packets sufficient? Does it requires a
broadcast packet from the device after it moves?

	  Andrew

Re: [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[Chuanhong Guo]

Hi!

On Tue, Apr 21, 2020 at 12:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> The MAC address needs to move, no argument there. But what are the
> mechanisms which cause this. Is learning sufficient, or does DSA need
> to take an active role?

cpu port learning will break switch operation if for whatever reason
we want to disable bridge offloading (e.g. ebtables?). In this case
a packet received from cpu port need to be sent back through
cpu port to another switch port, and the switch will learn from this
packet incorrectly.

If we want cpu port learning to kick in, we need to make sure that:
1. When bridge offload is enabled, the switch takes care of packet
    flooding on switch ports itself, instead of flooding with software
    bridge.
2. Software bridge shouldn't forward any packet between ports
    on the same switch.
3. cpu port learning should only be enabled when bridge
    offloading is used.

Otherwise we need to manually sync fdb between software bridge
and switch, specifically we need to take over fdb management
on cpu port and tell the switch what devices are on the software
bridge port side.

I haven't read kernel bridge code thoroughly and have no idea
which one is better/easier.

Some switches (e.g. mt753x) have an option to forward packets
with unknown destination port to cpu port only, instead of flooding.
For this type of switch, the solution proposed in this patch is fine,
because removing fdb entries has the same effect as telling switch
that a device is on cpu port. If there's a switch without this feature,
(which I have no idea if it exists) there will be issues on packet
flooding behavior.

>
> Forget about DSA for the moment. How does this work for two normal
> bridges? Is the flow of unicast packets sufficient? Does it requires a
> broadcast packet from the device after it moves?

It doesn't have to be a broadcast packet but it needs a packet to go
through both bridges.

Say we have bridge A and bridge B, port A1 and B1 are connected
together and a device is on port A2 first:
Bridge A knows that this device is on port A2 and will forward traffic
through A1 to B1 if needed. Bridge B sees these packets and knows
device is on port B1.
When the device move from A2 to B2, B updates its fdb and if a
packet reaches A, A will update its fdb too.

The problem addressed in this patch is that switch doesn't update
its fdb when a device moves from switch to software bridge, because
cpu port learning is disabled.

-- 
Regards,
Chuanhong Guo

Re: [Bridge] [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[Chuanhong Guo]

Hi!

On Tue, Apr 21, 2020 at 12:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> The MAC address needs to move, no argument there. But what are the
> mechanisms which cause this. Is learning sufficient, or does DSA need
> to take an active role?

cpu port learning will break switch operation if for whatever reason
we want to disable bridge offloading (e.g. ebtables?). In this case
a packet received from cpu port need to be sent back through
cpu port to another switch port, and the switch will learn from this
packet incorrectly.

If we want cpu port learning to kick in, we need to make sure that:
1. When bridge offload is enabled, the switch takes care of packet
    flooding on switch ports itself, instead of flooding with software
    bridge.
2. Software bridge shouldn't forward any packet between ports
    on the same switch.
3. cpu port learning should only be enabled when bridge
    offloading is used.

Otherwise we need to manually sync fdb between software bridge
and switch, specifically we need to take over fdb management
on cpu port and tell the switch what devices are on the software
bridge port side.

I haven't read kernel bridge code thoroughly and have no idea
which one is better/easier.

Some switches (e.g. mt753x) have an option to forward packets
with unknown destination port to cpu port only, instead of flooding.
For this type of switch, the solution proposed in this patch is fine,
because removing fdb entries has the same effect as telling switch
that a device is on cpu port. If there's a switch without this feature,
(which I have no idea if it exists) there will be issues on packet
flooding behavior.

>
> Forget about DSA for the moment. How does this work for two normal
> bridges? Is the flow of unicast packets sufficient? Does it requires a
> broadcast packet from the device after it moves?

It doesn't have to be a broadcast packet but it needs a packet to go
through both bridges.

Say we have bridge A and bridge B, port A1 and B1 are connected
together and a device is on port A2 first:
Bridge A knows that this device is on port A2 and will forward traffic
through A1 to B1 if needed. Bridge B sees these packets and knows
device is on port B1.
When the device move from A2 to B2, B updates its fdb and if a
packet reaches A, A will update its fdb too.

The problem addressed in this patch is that switch doesn't update
its fdb when a device moves from switch to software bridge, because
cpu port learning is disabled.

-- 
Regards,
Chuanhong Guo

Re: [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[Andrew Lunn]

On Wed, Apr 22, 2020 at 02:01:28PM +0800, Chuanhong Guo wrote:
> Hi!
> 
> On Tue, Apr 21, 2020 at 12:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > The MAC address needs to move, no argument there. But what are the
> > mechanisms which cause this. Is learning sufficient, or does DSA need
> > to take an active role?
> 
> cpu port learning will break switch operation if for whatever reason
> we want to disable bridge offloading (e.g. ebtables?). In this case
> a packet received from cpu port need to be sent back through
> cpu port to another switch port, and the switch will learn from this
> packet incorrectly.
> 
> If we want cpu port learning to kick in, we need to make sure that:
> 1. When bridge offload is enabled, the switch takes care of packet
>     flooding on switch ports itself, instead of flooding with software
>     bridge.

Hi Chuanhong

This is what the skb->offload_fwd_mark is all about. If this is set to
1, it means the switch has done all the forwarding needed for ports in
that switch. Most of the tag drivers set this unconditionally true.

> 2. Software bridge shouldn't forward any packet between ports
>     on the same switch.

If skb->offload_fwd_mark is true, it won't.

> 3. cpu port learning should only be enabled when bridge
>     offloading is used.

So it should be safe for most switch drivers. And the ones which don't
set offload_fwd_mark are probably relying of software bridging, or are
broken and replicating frames.

> It doesn't have to be a broadcast packet but it needs a packet to go
> through both bridges.
> 
> Say we have bridge A and bridge B, port A1 and B1 are connected
> together and a device is on port A2 first:
> Bridge A knows that this device is on port A2 and will forward traffic
> through A1 to B1 if needed. Bridge B sees these packets and knows
> device is on port B1.
> When the device move from A2 to B2, B updates its fdb and if a
> packet reaches A, A will update its fdb too.

The issue here is 'if a packet reaches A'. B might have no reason to
send a unicast packet to A, if none of the destinations the device is
talking to is reached via A. Which is why i think a
broadcast/multicast packet is more likely to be involved.

		    Andrew

Re: [Bridge] [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[Andrew Lunn]

On Wed, Apr 22, 2020 at 02:01:28PM +0800, Chuanhong Guo wrote:
> Hi!
> 
> On Tue, Apr 21, 2020 at 12:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > The MAC address needs to move, no argument there. But what are the
> > mechanisms which cause this. Is learning sufficient, or does DSA need
> > to take an active role?
> 
> cpu port learning will break switch operation if for whatever reason
> we want to disable bridge offloading (e.g. ebtables?). In this case
> a packet received from cpu port need to be sent back through
> cpu port to another switch port, and the switch will learn from this
> packet incorrectly.
> 
> If we want cpu port learning to kick in, we need to make sure that:
> 1. When bridge offload is enabled, the switch takes care of packet
>     flooding on switch ports itself, instead of flooding with software
>     bridge.

Hi Chuanhong

This is what the skb->offload_fwd_mark is all about. If this is set to
1, it means the switch has done all the forwarding needed for ports in
that switch. Most of the tag drivers set this unconditionally true.

> 2. Software bridge shouldn't forward any packet between ports
>     on the same switch.

If skb->offload_fwd_mark is true, it won't.

> 3. cpu port learning should only be enabled when bridge
>     offloading is used.

So it should be safe for most switch drivers. And the ones which don't
set offload_fwd_mark are probably relying of software bridging, or are
broken and replicating frames.

> It doesn't have to be a broadcast packet but it needs a packet to go
> through both bridges.
> 
> Say we have bridge A and bridge B, port A1 and B1 are connected
> together and a device is on port A2 first:
> Bridge A knows that this device is on port A2 and will forward traffic
> through A1 to B1 if needed. Bridge B sees these packets and knows
> device is on port B1.
> When the device move from A2 to B2, B updates its fdb and if a
> packet reaches A, A will update its fdb too.

The issue here is 'if a packet reaches A'. B might have no reason to
send a unicast packet to A, if none of the destinations the device is
talking to is reached via A. Which is why i think a
broadcast/multicast packet is more likely to be involved.

		    Andrew

Re: [Bridge] [RFC PATCH net-next] net: bridge: fix client roaming from DSA user port

[Eric Woudstra]

Hi Andrew and Chuanhong,

What is the status of this patch?

I am running into the same problem on my wrt3200acm and wrt1900acs, 
running on linux kernel, dsa driver and ubuntu.

I tried the patch below, without any luck. Although I have conceived two 
other work-arounds, I am curious if there is a real fix for this issue.

diff -Naur a/drivers/net/dsa/mv88e6xxx/chip.c 
b/drivers/net/dsa/mv88e6xxx/chip.c
--- a/drivers/net/dsa/mv88e6xxx/chip.c    2020-10-03 11:21:32.000000000 
+0200
+++ b/drivers/net/dsa/mv88e6xxx/chip.c    2020-12-18 15:01:58.010404773 
+0100
@@ -2476,3 +2475,0 @@
-    /* Disable learning for CPU port */
-    if (dsa_is_cpu_port(ds, port))
-        reg = 0;


Regards,

Eric Woudstra