Skip to contentSkip to conversation

bridge: Handle changes in VLAN_FLAG_BRIDGE_BINDING

PATCH Applied Petr Machata 3 participants 10 messages Dec 18, 2024 – Dec 20, 2024

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover bridge: Handle changes in VLAN_FLAG_BRIDGE_BINDING

When bridge binding is enabled on a VLAN netdevice, its link state should track bridge ports that are members of the corresponding VLAN. This works for a newly-added netdevices. However toggling the option does not have the effect of enabling or disabling the behavior as appropriate.

In this patchset, have bridge react to bridge_binding toggles on VLAN uppers.

There has been another attempt at supporting this behavior in 2022 by Sevinj Aghayeva [0]. A discussion ensued that informed how this new patchset is constructed, namely that the logic is in the bridge as opposed to the 8021q driver, and the bridge reacts to NETDEV_CHANGE events on the 8021q upper.

Patches #1 and #2 contain the implementation, patches #3 and #4 a selftest.

[0] https://lore.kernel.org/netdev/[email protected]/

Petr Machata (4):
net: bridge: Extract a helper to handle bridge_binding toggles
net: bridge: Handle changes in VLAN_FLAG_BRIDGE_BINDING
selftests: net: lib: Add a couple autodefer helpers
selftests: net: Add a VLAN bridge binding selftest

 net/bridge/br.c                               |   7 +
 net/bridge/br_private.h                       |   9 +
 net/bridge/br_vlan.c                          |  44 ++-
 tools/testing/selftests/net/Makefile          |   1 +
 tools/testing/selftests/net/lib.sh            |  31 ++-
 .../selftests/net/vlan_bridge_binding.sh      | 256 ++++++++++++++++++
 6 files changed, 340 insertions(+), 8 deletions(-)
 create mode 100755 tools/testing/selftests/net/vlan_bridge_binding.sh
Signature · 1 line
2.47.0
#1 lore raw

PATCH 1/4 net: bridge: Extract a helper to handle bridge_binding toggles

Currently, the BROPT_VLAN_BRIDGE_BINDING bridge option is only toggled when VLAN devices are added on top of a bridge or removed from it. Extract the toggling of the option to a function so that it could be invoked by a subsequent patch when the state of an upper VLAN device changes.

---
 net/bridge/br_vlan.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
net/bridge/br_vlan.c +14 -6
index 89f51ea4cabe..b728b71e693f 100644
@@ -1664,6 +1664,18 @@ static void br_vlan_set_all_vlan_dev_state(struct net_bridge_port *p)
 	}
 }
 
+static void br_vlan_toggle_bridge_binding(struct net_device *br_dev,
+					  bool enable)
+{
+	struct net_bridge *br = netdev_priv(br_dev);
+
+	if (enable)
+		br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING, true);
+	else
+		br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING,
+			      br_vlan_has_upper_bind_vlan_dev(br_dev));
+}
+
 static void br_vlan_upper_change(struct net_device *dev,
 				 struct net_device *upper_dev,
 				 bool linking)
@@ -1673,13 +1685,9 @@ static void br_vlan_upper_change(struct net_device *dev,
 	if (!br_vlan_is_bind_vlan_dev(upper_dev))
 		return;
 
-	if (linking) {
+	br_vlan_toggle_bridge_binding(dev, linking);
+	if (linking)
 		br_vlan_set_vlan_dev_state(br, upper_dev);
-		br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING, true);
-	} else {
-		br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING,
-			      br_vlan_has_upper_bind_vlan_dev(dev));
-	}
 }
 
 struct br_vlan_link_state_walk_data {
Signature · 1 line
2.47.0
maintainer Acked-by on v1 1/4
#2 lore raw

Re: [PATCH net-next 1/4] net: bridge: Extract a helper to handle bridge_binding toggles

Quoted 40 lines · Petr Machata

Currently, the BROPT_VLAN_BRIDGE_BINDING bridge option is only toggled when VLAN devices are added on top of a bridge or removed from it. Extract the toggling of the option to a function so that it could be invoked by a subsequent patch when the state of an upper VLAN device changes.

---
 net/bridge/br_vlan.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
net/bridge/br_vlan.c +13 -1
index 89f51ea4cabe..b728b71e693f 100644
@@ -1664,6 +1664,18 @@ static void br_vlan_set_all_vlan_dev_state(struct net_bridge_port *p)
 	}
 }
 
+static void br_vlan_toggle_bridge_binding(struct net_device *br_dev,
+					  bool enable)
+{
+	struct net_bridge *br = netdev_priv(br_dev);
+
+	if (enable)
+		br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING, true);
+	else
+		br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING,
+			      br_vlan_has_upper_bind_vlan_dev(br_dev));
+}
+
 static void br_vlan_upper_change(struct net_device *dev,
 				 struct net_device *upper_dev,
 				 bool linking)
@@ -1673,13 +1685,9 @@ static void br_vlan_upper_change(struct net_device *dev,
 	if (!br_vlan_is_bind_vlan_dev(upper_dev))
 		return;
 
-	if (linking) {
+	br_vlan_toggle_bridge_binding(dev, linking);

net/bridge/br_vlan.c static void br_vlan_upper_change(struct net_device *dev, · line 1693

Quoted 10 lines
net/bridge/br_vlan.c +1 -5
+	if (linking)
 		br_vlan_set_vlan_dev_state(br, upper_dev);
-		br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING, true);
-	} else {
-		br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING,
-			      br_vlan_has_upper_bind_vlan_dev(dev));
-	}
 }
 
 struct br_vlan_link_state_walk_data {
#3 lore raw

PATCH 2/4 net: bridge: Handle changes in VLAN_FLAG_BRIDGE_BINDING

In reply to Petr Machata

When bridge binding is enabled on a VLAN netdevice, its link state should track bridge ports that are members of the corresponding VLAN. This works for newly-added netdevices. However toggling the option does not have the effect of enabling or disabling the behavior as appropriate.

In this patch, react to bridge_binding toggles on VLAN uppers.

---
 net/bridge/br.c         |  7 +++++++
 net/bridge/br_private.h |  9 +++++++++
 net/bridge/br_vlan.c    | 24 ++++++++++++++++++++++++
 3 files changed, 40 insertions(+)
net/bridge/br.c +7 -0
index 2cab878e0a39..183fcb362f9e 100644
@@ -51,6 +51,13 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 		}
 	}
 
+	if (is_vlan_dev(dev)) {
+		struct net_device *real_dev = vlan_dev_real_dev(dev);
+
+		if (netif_is_bridge_master(real_dev))
+			br_vlan_vlan_upper_event(real_dev, dev, event);
+	}
+
 	/* not a port of a bridge */
 	p = br_port_get_rtnl(dev);
 	if (!p)
net/bridge/br_private.h +9 -0
index 9853cfbb9d14..29d6ec45cf41 100644
@@ -1571,6 +1571,9 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v,
 void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
 int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
 			 void *ptr);
+void br_vlan_vlan_upper_event(struct net_device *br_dev,
+			      struct net_device *vlan_dev,
+			      unsigned long event);
 int br_vlan_rtnl_init(void);
 void br_vlan_rtnl_uninit(void);
 void br_vlan_notify(const struct net_bridge *br,
@@ -1802,6 +1805,12 @@ static inline int br_vlan_bridge_event(struct net_device *dev,
 	return 0;
 }
 
+static inline void br_vlan_vlan_upper_event(struct net_device *br_dev,
+					    struct net_device *vlan_dev,
+					    unsigned long event)
+{
+}
+
 static inline int br_vlan_rtnl_init(void)
 {
 	return 0;
net/bridge/br_vlan.c +24 -0
index b728b71e693f..d9a69ec9affe 100644
@@ -1772,6 +1772,30 @@ int br_vlan_bridge_event(struct net_device *dev, unsigned long event, void *ptr)
 	return ret;
 }
 
+void br_vlan_vlan_upper_event(struct net_device *br_dev,
+			      struct net_device *vlan_dev,
+			      unsigned long event)
+{
+	struct vlan_dev_priv *vlan = vlan_dev_priv(vlan_dev);
+	struct net_bridge *br = netdev_priv(br_dev);
+	bool bridge_binding;
+
+	switch (event) {
+	case NETDEV_CHANGE:
+	case NETDEV_UP:
+		break;
+	default:
+		return;
+	}
+
+	bridge_binding = vlan->flags & VLAN_FLAG_BRIDGE_BINDING;
+	br_vlan_toggle_bridge_binding(br_dev, bridge_binding);
+	if (bridge_binding)
+		br_vlan_set_vlan_dev_state(br, vlan_dev);
+	else if (!bridge_binding && netif_carrier_ok(br_dev))
+		netif_carrier_on(vlan_dev);
+}
+
 /* Must be protected by RTNL. */
 void br_vlan_port_event(struct net_bridge_port *p, unsigned long event)
 {
Signature · 1 line
2.47.0
maintainer Acked-by on v1 2/4
#4 lore raw

Re: [PATCH net-next 2/4] net: bridge: Handle changes in VLAN_FLAG_BRIDGE_BINDING

Quoted 85 lines · Petr Machata

When bridge binding is enabled on a VLAN netdevice, its link state should track bridge ports that are members of the corresponding VLAN. This works for newly-added netdevices. However toggling the option does not have the effect of enabling or disabling the behavior as appropriate.

In this patch, react to bridge_binding toggles on VLAN uppers.

---
 net/bridge/br.c         |  7 +++++++
 net/bridge/br_private.h |  9 +++++++++
 net/bridge/br_vlan.c    | 24 ++++++++++++++++++++++++
 3 files changed, 40 insertions(+)
net/bridge/br.c +7 -0
index 2cab878e0a39..183fcb362f9e 100644
@@ -51,6 +51,13 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 		}
 	}
 
+	if (is_vlan_dev(dev)) {
+		struct net_device *real_dev = vlan_dev_real_dev(dev);
+
+		if (netif_is_bridge_master(real_dev))
+			br_vlan_vlan_upper_event(real_dev, dev, event);
+	}
+
 	/* not a port of a bridge */
 	p = br_port_get_rtnl(dev);
 	if (!p)
net/bridge/br_private.h +9 -0
index 9853cfbb9d14..29d6ec45cf41 100644
@@ -1571,6 +1571,9 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v,
 void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
 int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
 			 void *ptr);
+void br_vlan_vlan_upper_event(struct net_device *br_dev,
+			      struct net_device *vlan_dev,
+			      unsigned long event);
 int br_vlan_rtnl_init(void);
 void br_vlan_rtnl_uninit(void);
 void br_vlan_notify(const struct net_bridge *br,
@@ -1802,6 +1805,12 @@ static inline int br_vlan_bridge_event(struct net_device *dev,
 	return 0;
 }
 
+static inline void br_vlan_vlan_upper_event(struct net_device *br_dev,
+					    struct net_device *vlan_dev,
+					    unsigned long event)
+{
+}
+
 static inline int br_vlan_rtnl_init(void)
 {
 	return 0;
net/bridge/br_vlan.c +17 -0
index b728b71e693f..d9a69ec9affe 100644
@@ -1772,6 +1772,30 @@ int br_vlan_bridge_event(struct net_device *dev, unsigned long event, void *ptr)
 	return ret;
 }
 
+void br_vlan_vlan_upper_event(struct net_device *br_dev,
+			      struct net_device *vlan_dev,
+			      unsigned long event)
+{
+	struct vlan_dev_priv *vlan = vlan_dev_priv(vlan_dev);
+	struct net_bridge *br = netdev_priv(br_dev);
+	bool bridge_binding;
+
+	switch (event) {
+	case NETDEV_CHANGE:
+	case NETDEV_UP:
+		break;
+	default:
+		return;
+	}
+
+	bridge_binding = vlan->flags & VLAN_FLAG_BRIDGE_BINDING;

net/bridge/br_vlan.c int br_vlan_bridge_event(struct net_device *dev, unsigned long event, void *ptr) · line 1801

Quoted 10 lines
net/bridge/br_vlan.c +7 -0
+	br_vlan_toggle_bridge_binding(br_dev, bridge_binding);
+	if (bridge_binding)
+		br_vlan_set_vlan_dev_state(br, vlan_dev);
+	else if (!bridge_binding && netif_carrier_ok(br_dev))
+		netif_carrier_on(vlan_dev);
+}
+
 /* Must be protected by RTNL. */
 void br_vlan_port_event(struct net_bridge_port *p, unsigned long event)
 {
#5 lore raw

PATCH 3/4 selftests: net: lib: Add a couple autodefer helpers

In reply to Petr Machata

Alongside the helper ip_link_set_up(), one to set the link down will be useful as well. Add a helper to determine the link state as well, ip_link_is_up(), and use it to short-circuit any changes if the state is already the desired one.

Furthermore, add a helper bridge_vlan_add().

---
CC: Shuah Khan <[email protected]>
CC: [email protected]

---
 tools/testing/selftests/net/lib.sh | 31 ++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)
tools/testing/selftests/net/lib.sh +29 -2
index 2cd5c743b2d9..0bd9a038a1f0 100644
@@ -477,12 +477,33 @@ ip_link_set_addr()
 	defer ip link set dev "$name" address "$old_addr"
 }
 
+ip_link_is_up()
+{
+	local name=$1; shift
+
+	local state=$(ip -j link show "$name" |
+		      jq -r '(.[].flags[] | select(. == "UP")) // "DOWN"')
+	[[ $state == "UP" ]]
+}
+
 ip_link_set_up()
 {
 	local name=$1; shift
 
-	ip link set dev "$name" up
-	defer ip link set dev "$name" down
+	if ! ip_link_is_up "$name"; then
+		ip link set dev "$name" up
+		defer ip link set dev "$name" down
+	fi
+}
+
+ip_link_set_down()
+{
+	local name=$1; shift
+
+	if ip_link_is_up "$name"; then
+		ip link set dev "$name" down
+		defer ip link set dev "$name" up
+	fi
 }
 
 ip_addr_add()
@@ -498,3 +519,9 @@ ip_route_add()
 	ip route add "$@"
 	defer ip route del "$@"
 }
+
+bridge_vlan_add()
+{
+	bridge vlan add "$@"
+	defer bridge vlan del "$@"
+}
Signature · 1 line
2.47.0
maintainer Acked-by on v1 3/4
#6 lore raw

Re: [PATCH net-next 3/4] selftests: net: lib: Add a couple autodefer helpers

Quoted 58 lines · Petr Machata

Alongside the helper ip_link_set_up(), one to set the link down will be useful as well. Add a helper to determine the link state as well, ip_link_is_up(), and use it to short-circuit any changes if the state is already the desired one.

Furthermore, add a helper bridge_vlan_add().

---
CC: Shuah Khan <[email protected]>
CC: [email protected]

---
 tools/testing/selftests/net/lib.sh | 31 ++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)
tools/testing/selftests/net/lib.sh +23 -2
index 2cd5c743b2d9..0bd9a038a1f0 100644
@@ -477,12 +477,33 @@ ip_link_set_addr()
 	defer ip link set dev "$name" address "$old_addr"
 }
 
+ip_link_is_up()
+{
+	local name=$1; shift
+
+	local state=$(ip -j link show "$name" |
+		      jq -r '(.[].flags[] | select(. == "UP")) // "DOWN"')
+	[[ $state == "UP" ]]
+}
+
 ip_link_set_up()
 {
 	local name=$1; shift
 
-	ip link set dev "$name" up
-	defer ip link set dev "$name" down
+	if ! ip_link_is_up "$name"; then
+		ip link set dev "$name" up
+		defer ip link set dev "$name" down
+	fi
+}
+
+ip_link_set_down()
+{
+	local name=$1; shift
+
+	if ip_link_is_up "$name"; then
+		ip link set dev "$name" down
+		defer ip link set dev "$name" up
+	fi
 }
 
 ip_addr_add()

tools/testing/selftests/net/lib.sh ip_route_add() · line 527

Quoted 9 lines
tools/testing/selftests/net/lib.sh +6 -0
@@ -498,3 +519,9 @@ ip_route_add()
 	ip route add "$@"
 	defer ip route del "$@"
 }
+
+bridge_vlan_add()
+{
+	bridge vlan add "$@"
+	defer bridge vlan del "$@"
+}
#7 lore raw

PATCH 4/4 selftests: net: Add a VLAN bridge binding selftest

In reply to Petr Machata

Add a test that exercises bridge binding.

---
CC: Shuah Khan <[email protected]>
CC: [email protected]

---
 tools/testing/selftests/net/Makefile          |   1 +
 .../selftests/net/vlan_bridge_binding.sh      | 256 ++++++++++++++++++
 2 files changed, 257 insertions(+)
 create mode 100755 tools/testing/selftests/net/vlan_bridge_binding.sh
tools/testing/selftests/net/Makefile +1 -0
index f09bd96cc978..73ee88d6b043 100644
@@ -96,6 +96,7 @@ TEST_PROGS += test_bridge_backup_port.sh
 TEST_PROGS += fdb_flush.sh fdb_notify.sh
 TEST_PROGS += fq_band_pktlimit.sh
 TEST_PROGS += vlan_hw_filter.sh
+TEST_PROGS += vlan_bridge_binding.sh
 TEST_PROGS += bpf_offload.py
 TEST_PROGS += ipv6_route_update_soft_lockup.sh
 TEST_PROGS += busy_poll_test.sh
tools/testing/selftests/net/vlan_bridge_binding.sh new file +256 -0

256 lines not on this page · raw message

Signature · 1 line
2.47.0
maintainer Acked-by on v1 4/4
#8 lore raw

Re: [PATCH net-next 4/4] selftests: net: Add a VLAN bridge binding selftest

Quoted 8 lines · Petr Machata

Add a test that exercises bridge binding.

---
CC: Shuah Khan <[email protected]>
CC: [email protected]

Quoted 6 lines
---
 tools/testing/selftests/net/Makefile          |   1 +
 .../selftests/net/vlan_bridge_binding.sh      | 256 ++++++++++++++++++
 2 files changed, 257 insertions(+)
 create mode 100755 tools/testing/selftests/net/vlan_bridge_binding.sh
#9 lore raw
In reply to Petr Machata
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 9 lines · On Wed, 18 Dec 2024 18:15:55 +0100 you wrote:

When bridge binding is enabled on a VLAN netdevice, its link state should track bridge ports that are members of the corresponding VLAN. This works for a newly-added netdevices. However toggling the option does not have the effect of enabling or disabling the behavior as appropriate.

In this patchset, have bridge react to bridge_binding toggles on VLAN uppers.

[...]

Here is the summary with links:
- [net-next,1/4] net: bridge: Extract a helper to handle bridge_binding toggles

    https://git.kernel.org/netdev/net-next/c/f284424dc17b
  - [net-next,2/4] net: bridge: Handle changes in VLAN_FLAG_BRIDGE_BINDING
    https://git.kernel.org/netdev/net-next/c/3abd45122c72
  - [net-next,3/4] selftests: net: lib: Add a couple autodefer helpers
    https://git.kernel.org/netdev/net-next/c/976d248bd333
  - [net-next,4/4] selftests: net: Add a VLAN bridge binding selftest
    https://git.kernel.org/netdev/net-next/c/dca12e9ab760

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html