[PATCH 0/1] Fixup unicast filter for new vlan-aware-bridge ports

With this patch I'd like to fix an issue with vlan-aware bridges with a
single auto-port and a different MAC address on the bridge than its
port.

It took me a while to dig into this, and I hope to get some feedback and
would very much like to help out with more debugging if necessary and
hope this patch is a good first start.

Here's are the details I gathered:

When using devices with IFF_UNICAST_FLT support as bridge ports, they
sometimes don't get both MAC addresses added to their MAC filter
(seemingly limited to the time they're *added* to the bridge).

Since 2796d0c648c9 ("bridge: Automatically manage port promiscuous mode."),
bridges with `vlan_filtering 1` and only 1 auto-port don't set
IFF_PROMISC for unicast-filtering-capable ports.
This causes the bridge to "fail" if it has a different MAC than its
port, as the address is not added to the unicast filter when the port is
added.

This has become apparent with systemd's switch to
`MACAddressPolicy=persistent` which causes bridges to get different mac
addresses.

This is easily reproduced in qemu with `e1000` cards or virtio with
`vhost=off`.

Normally this should happen right when creating the bridge...

    ## This should be perfectly sufficient to reproduce this:
    # ip link add br0 type bridge vlan_filtering 1
    # ip link set eno1 master br0
    ## Setup addresses on the bridge and try to ping something

unless the port had IFF_PROMISC set when it was added to the bridge. In
that case it should be sufficient to simply re-plug the port.

    # ip link set eno1 nomaster
    # ip link set eno1 master br0

In my virtio-net based reproducer, I sprinkled some debugging output
into qemu (on top of the 6.0 tag from git, if you'd like the patch for a
quick test I'd be happy to provide it, too) and got the following:
----
VIRTIO_NET_CTRL_MAC
handle mac table set
    mac_data.entries = 1
    copying 1 single macs
    in_use now 1
    first_multi=1
    mac_data.entries = 1 (multi)
    copying 1 multi macs
    now have: in_use=2 first_multi=1, uni_ovf=0 multi_ovf=0
    mac 0: 52:54:00:12:34:56
    mac 1: 01:00:5e:00:00:01
----

This shows only 1 unicast MAC (the one I assigned to the virtio NIC).
The bridge has a different MAC.

The quickest fix here is to change the mac address on the bridge while
the port is connected, this ends up re-syncing the MAC filter:

# ip link set br0 address 52:54:00:12:11:12

This created the following debug output:
----
VIRTIO_NET_CTRL_MAC
handle mac table set
    mac_data.entries = 2
    copying 2 single macs
    in_use now 2
    first_multi=2
    mac_data.entries = 1 (multi)
    copying 1 multi macs
    now have: in_use=3 first_multi=2, uni_ovf=0 multi_ovf=0
    mac 0: 52:54:00:12:34:56
    mac 1: 52:54:00:12:11:12
    mac 2: 01:00:5e:00:00:01
----

Above, both MAC addresses are visible in the mac table, and the bridge
works as expected.

Other noteworthy behaviors:

* setting `vlan_filtering 1` *after* adding a port does not cause the
  issue.
* adding another auto-port puts all of the bridge's ports in promisc
  mode and flushes the UC MAC list, networking works; removing the new
  port resyncs the UC MAC list to correctly contain both addresses and
  networking keeps working

So it seems to be limited to the time where the port is being *added* to
the bridge.

I've tested git-master, and proxmox kernels (ubuntu based) 5.11 and 5.4,
all of which experience the same behavior, patch applies cleanly to all
of them.

Wolfgang Bumiller (1):
  net: bridge: sync fdb to new unicast-filtering ports

 net/bridge/br_if.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.32.0

Since commit 2796d0c648c9 ("bridge: Automatically manage
port promiscuous mode.")
bridges with `vlan_filtering 1` and only 1 auto-port don't
set IFF_PROMISC for unicast-filtering-capable ports.

Normally on port changes `br_manage_promisc` is called to
update the promisc flags and unicast filters if necessary,
but it cannot distinguish between *new* ports and ones
losing their promisc flag, and new ports end up not
receiving the MAC address list.

Fix this by calling `br_fdb_sync_static` in `br_add_if`
after the port promisc flags are updated and the unicast
filter was supposed to have been filled.

Signed-off-by: Wolfgang Bumiller <redacted>
---
 net/bridge/br_if.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index f7d2f472ae24..183e72e7b65e 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -652,6 +652,18 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 	list_add_rcu(&p->list, &br->port_list);
 
 	nbp_update_port_count(br);
+	if (!br_promisc_port(p) && (p->dev->priv_flags & IFF_UNICAST_FLT)) {
+		/* When updating the port count we also update all ports'
+		 * promiscuous mode.
+		 * A port leaving promiscuous mode normally gets the bridge's
+		 * fdb synced to the unicast filter (if supported), however,
+		 * `br_port_clear_promisc` does not distinguish between
+		 * non-promiscuous ports and *new* ports, so we need to
+		 * sync explicitly here.
+		 */
+		if (br_fdb_sync_static(br, p))
+			netdev_err(dev, "failed to sync bridge addresses to this port\n");
+	}
 
 	netdev_update_features(br->dev);
 

-- 
2.32.0

On 01/07/2021 15:28, Wolfgang Bumiller wrote:

Hi,
The patch is wrong because br_add_if() can fail after you sync these entries and
then nothing will unsync them. Out of curiousity what's the use case of a bridge with a
single port only ? Because, as you've also noted, this will be an issue only if there is
a single port and sounds like a corner case, maybe there's a better way to handle it.

To be honest this promisc management has caused us headaches with scale setups with thousands
of permanent and static entries where we don't need to sync uc lists, we've actually thought
about flags to disable this altogether.

Thanks,
 Nik

On 01.07.21 15:49, Nikolay Aleksandrov wrote:

On 01/07/2021 15:28, Wolfgang Bumiller wrote:

quoted

Since commit 2796d0c648c9 ("bridge: Automatically manage
port promiscuous mode.")
bridges with `vlan_filtering 1` and only 1 auto-port don't
set IFF_PROMISC for unicast-filtering-capable ports.

Normally on port changes `br_manage_promisc` is called to
update the promisc flags and unicast filters if necessary,
but it cannot distinguish between *new* ports and ones
losing their promisc flag, and new ports end up not
receiving the MAC address list.

Fix this by calling `br_fdb_sync_static` in `br_add_if`
after the port promisc flags are updated and the unicast
filter was supposed to have been filled.

Signed-off-by: Wolfgang Bumiller <redacted>
---
 net/bridge/br_if.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index f7d2f472ae24..183e72e7b65e 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -652,6 +652,18 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 	list_add_rcu(&p->list, &br->port_list);
 
 	nbp_update_port_count(br);
+	if (!br_promisc_port(p) && (p->dev->priv_flags & IFF_UNICAST_FLT)) {
+		/* When updating the port count we also update all ports'
+		 * promiscuous mode.
+		 * A port leaving promiscuous mode normally gets the bridge's
+		 * fdb synced to the unicast filter (if supported), however,
+		 * `br_port_clear_promisc` does not distinguish between
+		 * non-promiscuous ports and *new* ports, so we need to
+		 * sync explicitly here.
+		 */
+		if (br_fdb_sync_static(br, p))
+			netdev_err(dev, "failed to sync bridge addresses to this port\n");
+	}
 
 	netdev_update_features(br->dev);
 

Hi,

Hi, commenting as was peripherally involved into this.

The patch is wrong because br_add_if() can fail after you sync these entries and
then nothing will unsync them. Out of curiousity what's the use case of a bridge with a
single port only ? Because, as you've also noted, this will be an issue only if there is
a single port and sounds like a corner case, maybe there's a better way to handle it.

In practice you're right, it is not often useful, but that does not means that it
won't happen. For example, in Proxmox VE, a hypervisor/clustering debian-based distro,
we recommend users that they need to migrate all (QEMU) VMs to another cluster-node when
doing a (major) upgrade as with that way they get no downtime for the VMs.

Now, if the user had a bridge with a single port this was not an issue as long as VMs
where running the TAP device we use for them where bridge ports too.

But on reboot, with all VMs and thus ports still gone, the system comes up with that
bridge having a single port.

That itself was seen as a problem until recently because the system set the MAC of the
bridge to one of the bridge ports.

But with the next Debian Version (Bullseye) we're pulling in a systemd version which
now defaults to MACAddressPolicy=persistent[0] also for virtual devices like bridges,
so with that update done and rebooted the bridge has another MAC address, not matching
the one of a bridge port anymore, which means the host may, depending on some other
side effects like vlan-awareness on (as without that promisc would be enabled anyway),
not be ping'able and other issue anymore.
Due to some specialty handling of learning/filtering in specific drivers this is not
reproducible on every NIC model (IIRC, it was in igb and e1000e ones but not in some
realtek ones).

Hope that was not written to confusingly.

[0]: https://www.freedesktop.org/software/systemd/man/systemd.link.html#MACAddressPolicy=

To be honest this promisc management has caused us headaches with scale setups with thousands
of permanent and static entries where we don't need to sync uc lists, we've actually thought
about flags to disable this altogether.

FWIW, when we got this reported by a beta tester a initial (not really thought out) idea
of mine was to drop the special br_manage_promisc case to disable promisc on the bridge
port for one single auto-port, introduced by commit 2796d0c648c940b4796f84384fbcfb0a2399db84
in 2014, i.e., something like (still not 100% thought out):


----8<----

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index f7d2f472ae24..520c79c21362 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -147,18 +147,7 @@ void br_manage_promisc(struct net_bridge *br)
 		if (set_all) {
 			br_port_set_promisc(p);
 		} else {
-			/* If the number of auto-ports is <= 1, then all other
-			 * ports will have their output configuration
-			 * statically specified through fdbs.  Since ingress
-			 * on the auto-port becomes forwarding/egress to other
-			 * ports and egress configuration is statically known,
-			 * we can say that ingress configuration of the
-			 * auto-port is also statically known.
-			 * This lets us disable promiscuous mode and write
-			 * this config to hw.
-			 */
-			if (br->auto_cnt == 0 ||
-			    (br->auto_cnt == 1 && br_auto_port(p)))
+			if (br->auto_cnt == 0)
 				br_port_clear_promisc(p);
 			else
 				br_port_set_promisc(p);

On 01/07/2021 17:51, Thomas Lamprecht wrote:

On 01.07.21 15:49, Nikolay Aleksandrov wrote:

quoted

On 01/07/2021 15:28, Wolfgang Bumiller wrote:

quoted

Since commit 2796d0c648c9 ("bridge: Automatically manage
port promiscuous mode.")
bridges with `vlan_filtering 1` and only 1 auto-port don't
set IFF_PROMISC for unicast-filtering-capable ports.

Normally on port changes `br_manage_promisc` is called to
update the promisc flags and unicast filters if necessary,
but it cannot distinguish between *new* ports and ones
losing their promisc flag, and new ports end up not
receiving the MAC address list.

Fix this by calling `br_fdb_sync_static` in `br_add_if`
after the port promisc flags are updated and the unicast
filter was supposed to have been filled.

Signed-off-by: Wolfgang Bumiller <redacted>
---
 net/bridge/br_if.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index f7d2f472ae24..183e72e7b65e 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -652,6 +652,18 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 	list_add_rcu(&p->list, &br->port_list);
 
 	nbp_update_port_count(br);
+	if (!br_promisc_port(p) && (p->dev->priv_flags & IFF_UNICAST_FLT)) {
+		/* When updating the port count we also update all ports'
+		 * promiscuous mode.
+		 * A port leaving promiscuous mode normally gets the bridge's
+		 * fdb synced to the unicast filter (if supported), however,
+		 * `br_port_clear_promisc` does not distinguish between
+		 * non-promiscuous ports and *new* ports, so we need to
+		 * sync explicitly here.
+		 */
+		if (br_fdb_sync_static(br, p))
+			netdev_err(dev, "failed to sync bridge addresses to this port\n");
+	}
 
 	netdev_update_features(br->dev);
 

Hi,

Hi, commenting as was peripherally involved into this.

quoted

The patch is wrong because br_add_if() can fail after you sync these entries and
then nothing will unsync them. Out of curiousity what's the use case of a bridge with a
single port only ? Because, as you've also noted, this will be an issue only if there is
a single port and sounds like a corner case, maybe there's a better way to handle it.

In practice you're right, it is not often useful, but that does not means that it
won't happen. For example, in Proxmox VE, a hypervisor/clustering debian-based distro,
we recommend users that they need to migrate all (QEMU) VMs to another cluster-node when
doing a (major) upgrade as with that way they get no downtime for the VMs.

Now, if the user had a bridge with a single port this was not an issue as long as VMs
where running the TAP device we use for them where bridge ports too.

But on reboot, with all VMs and thus ports still gone, the system comes up with that
bridge having a single port.

That itself was seen as a problem until recently because the system set the MAC of the
bridge to one of the bridge ports.

But with the next Debian Version (Bullseye) we're pulling in a systemd version which
now defaults to MACAddressPolicy=persistent[0] also for virtual devices like bridges,
so with that update done and rebooted the bridge has another MAC address, not matching
the one of a bridge port anymore, which means the host may, depending on some other
side effects like vlan-awareness on (as without that promisc would be enabled anyway),
not be ping'able and other issue anymore.
Due to some specialty handling of learning/filtering in specific drivers this is not
reproducible on every NIC model (IIRC, it was in igb and e1000e ones but not in some
realtek ones).

Hope that was not written to confusingly.

[0]: https://www.freedesktop.org/software/systemd/man/systemd.link.html#MACAddressPolicy=

I see, thank you for the details. Just to clarify I'm not against fixing it or against this patch,
the question was out of curiousity only, as for the patch it needs to be fixed so unsync will be
handled in the error paths after the sync and also I'd suggest changing the error message to contain
what exactly couldn't be synced:
"failed to sync bridge static fdb addresses to this port"
or something in those lines. Since this fixes actual bug please also add a Fixes: tag with the
appropriate commit id where it was introduced.

quoted

To be honest this promisc management has caused us headaches with scale setups with thousands
of permanent and static entries where we don't need to sync uc lists, we've actually thought
about flags to disable this altogether.

FWIW, when we got this reported by a beta tester a initial (not really thought out) idea
of mine was to drop the special br_manage_promisc case to disable promisc on the bridge
port for one single auto-port, introduced by commit 2796d0c648c940b4796f84384fbcfb0a2399db84
in 2014, i.e., something like (still not 100% thought out):

This sounds very tempting, but we might break setups which depend on that behaviour,
e.g. having 1 auto port and additional non-auto ports. There were some assumptions made
about having 1 auto port back in 2014 when these changes were made[0].

Thanks,
 Nik

[0] https://bridge.linux-foundation.narkive.com/d9Qnpa69/rfc-patch-v2-net-next-0-7-non-promisc-bidge-ports-support#post10

On Thu, Jul 01, 2021 at 06:33:20PM +0300, Nikolay Aleksandrov wrote:

On 01/07/2021 17:51, Thomas Lamprecht wrote:

quoted

On 01.07.21 15:49, Nikolay Aleksandrov wrote:

quoted

On 01/07/2021 15:28, Wolfgang Bumiller wrote:

quoted

Since commit 2796d0c648c9 ("bridge: Automatically manage
port promiscuous mode.")
bridges with `vlan_filtering 1` and only 1 auto-port don't
set IFF_PROMISC for unicast-filtering-capable ports.

Normally on port changes `br_manage_promisc` is called to
update the promisc flags and unicast filters if necessary,
but it cannot distinguish between *new* ports and ones
losing their promisc flag, and new ports end up not
receiving the MAC address list.

Fix this by calling `br_fdb_sync_static` in `br_add_if`
after the port promisc flags are updated and the unicast
filter was supposed to have been filled.

Signed-off-by: Wolfgang Bumiller <redacted>
---
 net/bridge/br_if.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index f7d2f472ae24..183e72e7b65e 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -652,6 +652,18 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 	list_add_rcu(&p->list, &br->port_list);
 
 	nbp_update_port_count(br);
+	if (!br_promisc_port(p) && (p->dev->priv_flags & IFF_UNICAST_FLT)) {
+		/* When updating the port count we also update all ports'
+		 * promiscuous mode.
+		 * A port leaving promiscuous mode normally gets the bridge's
+		 * fdb synced to the unicast filter (if supported), however,
+		 * `br_port_clear_promisc` does not distinguish between
+		 * non-promiscuous ports and *new* ports, so we need to
+		 * sync explicitly here.
+		 */
+		if (br_fdb_sync_static(br, p))
+			netdev_err(dev, "failed to sync bridge addresses to this port\n");
+	}
 
 	netdev_update_features(br->dev);
 

Hi,

Hi, commenting as was peripherally involved into this.

quoted

The patch is wrong because br_add_if() can fail after you sync these entries and
then nothing will unsync them. Out of curiousity what's the use case of a bridge with a
single port only ? Because, as you've also noted, this will be an issue only if there is
a single port and sounds like a corner case, maybe there's a better way to handle it.

In practice you're right, it is not often useful, but that does not means that it
won't happen. For example, in Proxmox VE, a hypervisor/clustering debian-based distro,
we recommend users that they need to migrate all (QEMU) VMs to another cluster-node when
doing a (major) upgrade as with that way they get no downtime for the VMs.

Now, if the user had a bridge with a single port this was not an issue as long as VMs
where running the TAP device we use for them where bridge ports too.

But on reboot, with all VMs and thus ports still gone, the system comes up with that
bridge having a single port.

That itself was seen as a problem until recently because the system set the MAC of the
bridge to one of the bridge ports.

But with the next Debian Version (Bullseye) we're pulling in a systemd version which
now defaults to MACAddressPolicy=persistent[0] also for virtual devices like bridges,
so with that update done and rebooted the bridge has another MAC address, not matching
the one of a bridge port anymore, which means the host may, depending on some other
side effects like vlan-awareness on (as without that promisc would be enabled anyway),
not be ping'able and other issue anymore.
Due to some specialty handling of learning/filtering in specific drivers this is not
reproducible on every NIC model (IIRC, it was in igb and e1000e ones but not in some
realtek ones).

Hope that was not written to confusingly.

[0]: https://www.freedesktop.org/software/systemd/man/systemd.link.html#MACAddressPolicy=

I see, thank you for the details. Just to clarify I'm not against fixing it or against this patch,
the question was out of curiousity only, as for the patch it needs to be fixed so unsync will be
handled in the error paths after the sync and also I'd suggest changing the error message to contain

Ah sorry, somehow I thought there was already an unsync reachable in
that code path, but I was wrong. Looks like I can just add the unsync
before the list_del in err7 since list_add happens pretty much right
before the sync.
I'll test with a knob to force a failure, I still have my patched qemu
to observe what happens to the mac list on the NIC :-)

what exactly couldn't be synced:
"failed to sync bridge static fdb addresses to this port"

Yeah that sounds better! Will change it in v2.

or something in those lines. Since this fixes actual bug please also add a Fixes: tag with the
appropriate commit id where it was introduced.

I was a bit hesitant at first about adding this, since I hadn't done any
before/after testing with the particular commit introducing the change,
though I'm fairly confident about that by now (maybe more so since the
`auto_cnt` condition was wrong (fixed up in e0a47d1f7816 ("bridge: Fix
incorrect judgment of promisc"))).