Sevinj Aghayeva
2022-Jul-30 16:03 UTC
[Bridge] [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests
When bridge binding is enabled for a vlan interface, it is expected
that the link state of the vlan interface will track the subset of the
ports that are also members of the corresponding vlan, rather than
that of all ports.
Currently, this feature works as expected when a vlan interface is
created with bridge binding enabled:
ip link add link br name vlan10 type vlan id 10 protocol 802.1q \
bridge_binding on
However, the feature does not work when a vlan interface is created
with bridge binding disabled, and then enabled later:
ip link add link br name vlan10 type vlan id 10 protocol 802.1q \
bridge_binding off
ip link set vlan10 type vlan bridge_binding on
After these two commands, the link state of the vlan interface
continues to track that of all ports, which is inconsistent and
confusing to users. This series fixes this bug and introduces two
tests for the valid behavior.
Sevinj Aghayeva (3):
net: bridge: export br_vlan_upper_change
net: 8021q: fix bridge binding behavior for vlan interfaces
selftests: net: tests for bridge binding behavior
include/linux/if_bridge.h | 9 ++
net/8021q/vlan.h | 2 +-
net/8021q/vlan_dev.c | 21 ++-
net/bridge/br_vlan.c | 7 +-
tools/testing/selftests/net/Makefile | 1 +
.../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++
6 files changed, 176 insertions(+), 7 deletions(-)
create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh
--
2.25.1
Sevinj Aghayeva
2022-Jul-30 16:03 UTC
[Bridge] [PATCH net-next 1/3] net: bridge: export br_vlan_upper_change
This function contains the logic for correctly setting the bridge
binding behavior of a vlan interface. Therefore, it should be executed
whenever the bridge binding flag of a vlan interface changes via the
vlan_dev_change_flags function in the 8021q module. Currently this
function is private, and it is only executed when a vlan interface is
first created. Export the function so that it can be called from
vlan_dev_change_flags.
Signed-off-by: Sevinj Aghayeva <sevinj.aghayeva at gmail.com>
Reviewed-by: Stefano Brivio <sbrivio at redhat.com>
Reviewed-by: Andy Roulin <aroulin at nvidia.com>
Reviewed-by: Roopa Prabhu <roopa at nvidia.com>
---
include/linux/if_bridge.h | 9 +++++++++
net/bridge/br_vlan.c | 7 ++++---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index d62ef428e3aa..0d92b0ed0961 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -180,6 +180,9 @@ void br_fdb_clear_offload(const struct net_device *dev, u16
vid);
bool br_port_flag_is_set(const struct net_device *dev, unsigned long flag);
u8 br_port_get_stp_state(const struct net_device *dev);
clock_t br_get_ageing_time(const struct net_device *br_dev);
+void br_vlan_upper_change(struct net_device *dev,
+ struct net_device *upper_dev,
+ bool linking);
#else
static inline struct net_device *
br_fdb_find_port(const struct net_device *br_dev,
@@ -208,6 +211,12 @@ static inline clock_t br_get_ageing_time(const struct
net_device *br_dev)
{
return 0;
}
+
+static inline void br_vlan_upper_change(struct net_device *dev,
+ struct net_device *upper_dev,
+ bool linking)
+{
+}
#endif
#endif
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 6e53dc991409..6bfc36da5a88 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -1653,9 +1653,9 @@ static void br_vlan_set_all_vlan_dev_state(struct
net_bridge_port *p)
}
}
-static void br_vlan_upper_change(struct net_device *dev,
- struct net_device *upper_dev,
- bool linking)
+void br_vlan_upper_change(struct net_device *dev,
+ struct net_device *upper_dev,
+ bool linking)
{
struct net_bridge *br = netdev_priv(dev);
@@ -1670,6 +1670,7 @@ static void br_vlan_upper_change(struct net_device *dev,
br_vlan_has_upper_bind_vlan_dev(dev));
}
}
+EXPORT_SYMBOL_GPL(br_vlan_upper_change);
struct br_vlan_link_state_walk_data {
struct net_bridge *br;
--
2.25.1
Sevinj Aghayeva
2022-Jul-30 16:03 UTC
[Bridge] [PATCH net-next 2/3] net: 8021q: fix bridge binding behavior for vlan interfaces
Currently, when one creates a vlan interface with the bridge binding
flag disabled (using ip link add... command) and then enables the
bridge binding flag afterwards (using ip link set... command), the
second command has no effect. In other words, the vlan interface does
not follow the status of the ports in its vlan.
The root cause of this problem is as follows. The correct bridge
binding behavior depends on two flags being set: a per vlan interface
flag (VLAN_FLAG_BRIDGE_BINDING) and global per bridge flag
(BROPT_VLAN_BRIDGE_BINDING); the ip link set command calls
vlan_dev_change_flags function, which sets only the per vlan interface
flag.
The correct behavior is to set/unset per bridge flag as well,
depending on whether there are other vlan interfaces with bridge
binding flags set. The logic for this behavior is already captured in
br_vlan_upper_change function. This patch fixes the bridge binding
behavior by calling the br_vlan_upper_change function whenever the per
interface flag is changed via vlan_dev_change_flags function.
Signed-off-by: Sevinj Aghayeva <sevinj.aghayeva at gmail.com>
Reviewed-by: Stefano Brivio <sbrivio at redhat.com>
Reviewed-by: Andy Roulin <aroulin at nvidia.com>
Reviewed-by: Roopa Prabhu <roopa at nvidia.com>
---
net/8021q/vlan.h | 2 +-
net/8021q/vlan_dev.c | 21 ++++++++++++++++++---
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
index 5eaf38875554..71947cdcfaaa 100644
--- a/net/8021q/vlan.h
+++ b/net/8021q/vlan.h
@@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct net_device
*dev,
int vlan_dev_set_egress_priority(const struct net_device *dev,
u32 skb_prio, u16 vlan_prio);
void vlan_dev_free_egress_priority(const struct net_device *dev);
-int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32 mask);
+int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask);
void vlan_dev_get_realdev_name(const struct net_device *dev, char *result,
size_t size);
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 035812b0461c..93680bac60b3 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -30,6 +30,7 @@
#include "vlan.h"
#include "vlanproc.h"
#include <linux/if_vlan.h>
+#include <linux/if_bridge.h>
#include <linux/netpoll.h>
/*
@@ -211,10 +212,11 @@ int vlan_dev_set_egress_priority(const struct net_device
*dev,
/* Flags are defined in the vlan_flags enum in
* include/uapi/linux/if_vlan.h file.
*/
-int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask)
+int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask)
{
struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
u32 old_flags = vlan->flags;
+ struct net_device *br_dev;
if (mask & ~(VLAN_FLAG_REORDER_HDR | VLAN_FLAG_GVRP |
VLAN_FLAG_LOOSE_BINDING | VLAN_FLAG_MVRP |
@@ -223,19 +225,32 @@ int vlan_dev_change_flags(const struct net_device *dev,
u32 flags, u32 mask)
vlan->flags = (old_flags & ~mask) | (flags & mask);
- if (netif_running(dev) && (vlan->flags ^ old_flags) &
VLAN_FLAG_GVRP) {
+ if (!netif_running(dev))
+ return 0;
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
if (vlan->flags & VLAN_FLAG_GVRP)
vlan_gvrp_request_join(dev);
else
vlan_gvrp_request_leave(dev);
}
- if (netif_running(dev) && (vlan->flags ^ old_flags) &
VLAN_FLAG_MVRP) {
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
if (vlan->flags & VLAN_FLAG_MVRP)
vlan_mvrp_request_join(dev);
else
vlan_mvrp_request_leave(dev);
}
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
+ netif_is_bridge_port(dev)) {
+ br_dev = vlan->real_dev;
+ if (vlan->flags & VLAN_FLAG_BRIDGE_BINDING)
+ br_vlan_upper_change(br_dev, dev, true);
+ else
+ br_vlan_upper_change(br_dev, dev, false);
+ }
+
return 0;
}
--
2.25.1
Sevinj Aghayeva
2022-Jul-30 16:03 UTC
[Bridge] [PATCH net-next 3/3] selftests: net: tests for bridge binding behavior
This patch adds two tests in a single file. The first of these is in
function run_test_late_bridge_binding_set, which tests that when a
vlan interface is created with bridge binding turned off, and later
bridge binding is turned on (using ip link set... command), the vlan
interface behaves accordingly, that is, it tracks the status of the
ports in its vlan.
The second test, which is in function run_test_multiple_vlan, tests
that when there are two vlan interfaces with bridge binding turned on,
turning off the bridge binding in one of the vlan interfaces does not
affect the bridge binding on the other interface.
Signed-off-by: Sevinj Aghayeva <sevinj.aghayeva at gmail.com>
Reviewed-by: Stefano Brivio <sbrivio at redhat.com>
Reviewed-by: Andy Roulin <aroulin at nvidia.com>
Reviewed-by: Roopa Prabhu <roopa at nvidia.com>
---
tools/testing/selftests/net/Makefile | 1 +
.../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++
2 files changed, 144 insertions(+)
create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh
diff --git a/tools/testing/selftests/net/Makefile
b/tools/testing/selftests/net/Makefile
index 80628bf8413a..f3ac4cb01695 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -40,6 +40,7 @@ TEST_PROGS += arp_ndisc_evict_nocarrier.sh
TEST_PROGS += ndisc_unsolicited_na_test.sh
TEST_PROGS += arp_ndisc_untracked_subnets.sh
TEST_PROGS += stress_reuseport_listen.sh
+TEST_PROGS += bridge_vlan_binding_test.sh
TEST_PROGS_EXTENDED := in_netns.sh setup_loopback.sh setup_veth.sh
TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh
TEST_GEN_FILES = socket nettest
diff --git a/tools/testing/selftests/net/bridge_vlan_binding_test.sh
b/tools/testing/selftests/net/bridge_vlan_binding_test.sh
new file mode 100755
index 000000000000..d094d847800c
--- /dev/null
+++ b/tools/testing/selftests/net/bridge_vlan_binding_test.sh
@@ -0,0 +1,143 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+cleanup() {
+ # Remove interfaces created by the previous run
+ ip link delete veth10 2>/dev/null
+ ip link delete veth20 2>/dev/null
+ ip link delete veth30 2>/dev/null
+ ip link delete br_default 2>/dev/null
+}
+
+trap cleanup EXIT
+
+setup() {
+ cleanup
+
+ # Create a bridge and add three ports to it.
+ ip link add dev br_default type bridge
+ ip link add dev veth10 type veth peer name veth11
+ ip link add dev veth20 type veth peer name veth21
+ ip link add dev veth30 type veth peer name veth31
+ ip link set dev veth10 master br_default
+ ip link set dev veth20 master br_default
+ ip link set dev veth30 master br_default
+
+ # Create VLAN 10 and VLAN 20.
+ bridge vlan add vid 10 dev br_default self
+ bridge vlan add vid 20 dev br_default self
+
+ # Add veth10 to VLAN 10 and veth20 to VLAN 20.
+ bridge vlan add vid 10 dev veth10
+ bridge vlan add vid 20 dev veth20
+
+ # Bring up the ports and the bridge.
+ ip link set veth10 up
+ ip link set veth11 up
+ ip link set veth20 up
+ ip link set veth21 up
+ ip link set veth30 up
+ ip link set veth31 up
+ ip link set br_default up
+}
+
+# This test checks that when a vlan interface is created with bridge
+# binding off, and then bridge binding turned on using "ip link set"
+# command, bridge binding is actually turned on -- this hasn't been
+# the case in the past.
+run_test_late_bridge_binding_set() {
+ setup
+
+ # Add VLAN interface vlan10 to VLAN 10 with bridge binding off.
+ ip link add link br_default name vlan10 type vlan id 10 protocol \
+ 802.1q bridge_binding off
+
+ # Bring up VLAN interface.
+ ip link set vlan10 up
+
+ # Turn bridge binding on for vlan10.
+ ip link set vlan10 type vlan bridge_binding on
+
+ # Bring down the port in vlan 10.
+ ip link set veth10 down
+
+ # Since bridge binding is turned on for vlan10 interface, it
+ # should be tracking only the port, veth10 in its vlan. Since
+ # veth10 is down, vlan10 should be down as well.
+ if ! ip link show vlan10 | grep -q 'state LOWERLAYERDOWN'; then
+ echo "FAIL - vlan10 should be LOWERLAYERDOWN but it is not"
+ exit 1
+ fi
+
+ # Bringe the port back up.
+ ip link set veth10 up
+
+ # The vlan 10 interface should be up now.
+ if ! ip link show vlan10 | grep -q 'state UP'; then
+ echo "FAIL - vlan10 should be UP but it is not"
+ exit 1
+ fi
+
+ echo "OK"
+}
+
+# This test checks that when there are multiple vlan interfaces with
+# bridge binding on, turning off bride binding in one of the vlan
+# interfaces does not affect the bridge binding of the other
+# interface.
+run_test_multiple_vlan() {
+ setup
+
+ # Add VLAN interface vlan10 to VLAN 10 with bridge binding on.
+ ip link add link br_default name vlan10 type vlan id 10 protocol \
+ 802.1q bridge_binding on
+ # Add VLAN interface vlan20 to VLAN 20 with bridge binding on.
+ ip link add link br_default name vlan20 type vlan id 20 protocol \
+ 802.1q bridge_binding on
+
+ # Bring up VLAN interfaces.
+ ip link set vlan10 up
+ ip link set vlan20 up
+
+ # Turn bridge binding off for vlan10.
+ ip link set vlan10 type vlan bridge_binding off
+
+ # Bring down the ports in vlans 10 and 20.
+ ip link set veth10 down
+ ip link set veth20 down
+
+ # Since bridge binding is off for vlan10 interface, it should
+ # be tracking all of the ports in the bridge; since veth30 is
+ # still up, vlan10 should also be up.
+ if ! ip link show vlan10 | grep -q 'state UP'; then
+ echo "FAIL - vlan10 should be UP but it is not"
+ exit 1
+ fi
+
+ # Since bridge binding is on for vlan20 interface, it should
+ # be tracking only the ports in its vlan. This port is veth20,
+ # and it is down; therefore, vlan20 should be down as well.
+ if ! ip link show vlan20 | grep -q 'state LOWERLAYERDOWN'; then
+ echo "FAIL - vlan20 should be LOWERLAYERDOWN but it is not"
+ exit 1
+ fi
+
+ # Bring the ports back up.
+ ip link set veth10 up
+ ip link set veth20 up
+
+ # Both vlan interfaces should be up now.
+ if ! ip link show vlan10 | grep -q 'state UP'; then
+ echo "FAIL - vlan10 should be UP but it is not"
+ exit 1
+ fi
+ if ! ip link show vlan20 | grep -q 'state UP' ; then
+ echo "FAIL - vlan20 should be UP but it is not"
+ exit 1
+ fi
+
+ echo "OK"
+}
+
+run_test_late_bridge_binding_set
+run_test_multiple_vlan
--
2.25.1
Nikolay Aleksandrov
2022-Jul-30 16:21 UTC
[Bridge] [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests
On 7/30/22 19:03, Sevinj Aghayeva wrote:> When bridge binding is enabled for a vlan interface, it is expected > that the link state of the vlan interface will track the subset of the > ports that are also members of the corresponding vlan, rather than > that of all ports. > > Currently, this feature works as expected when a vlan interface is > created with bridge binding enabled: > > ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ > bridge_binding on > > However, the feature does not work when a vlan interface is created > with bridge binding disabled, and then enabled later: > > ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ > bridge_binding off > ip link set vlan10 type vlan bridge_binding on > > After these two commands, the link state of the vlan interface > continues to track that of all ports, which is inconsistent and > confusing to users. This series fixes this bug and introduces two > tests for the valid behavior. > > Sevinj Aghayeva (3): > net: bridge: export br_vlan_upper_change > net: 8021q: fix bridge binding behavior for vlan interfaces > selftests: net: tests for bridge binding behavior > > include/linux/if_bridge.h | 9 ++ > net/8021q/vlan.h | 2 +- > net/8021q/vlan_dev.c | 21 ++- > net/bridge/br_vlan.c | 7 +- > tools/testing/selftests/net/Makefile | 1 + > .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++ > 6 files changed, 176 insertions(+), 7 deletions(-) > create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh >Hmm.. I don't like this and don't think this bridge function should be exported at all. Calling bridge state changing functions from 8021q module is not the proper way to solve this. The problem is that the bridge doesn't know that the state has changed, so you can process NETDEV_CHANGE events and check for the bridge vlan which got its state changed and react based on it. I haven't checked in detail, but I think it should be doable. So all the logic is kept inside the bridge. Cheers, Nik