[PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests

[PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests

[Sevinj Aghayeva]

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

[PATCH net-next 1/3] net: bridge: export br_vlan_upper_change

[Sevinj Aghayeva]

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@gmail.com>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: Andy Roulin <aroulin@nvidia.com>
Reviewed-by: Roopa Prabhu <roopa@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

[PATCH net-next 2/3] net: 8021q: fix bridge binding behavior for vlan interfaces

[Sevinj Aghayeva]

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@gmail.com>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: Andy Roulin <aroulin@nvidia.com>
Reviewed-by: Roopa Prabhu <roopa@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

Re: [PATCH net-next 2/3] net: 8021q: fix bridge binding behavior for vlan interfaces

[kernel test robot]

Hi Sevinj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Sevinj-Aghayeva/net-vlan-fix-bridge-binding-behavior-and-add-selftests/20220731-000455
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 63757225a93353bc2ce4499af5501eabdbbf23f9
config: openrisc-randconfig-r031-20220729 (https://download.01.org/0day-ci/archive/20220731/202207310332.cSMhECu3-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/993166d2a01876dc92807f74b3d72f63d25c8227
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sevinj-Aghayeva/net-vlan-fix-bridge-binding-behavior-and-add-selftests/20220731-000455
        git checkout 993166d2a01876dc92807f74b3d72f63d25c8227
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=openrisc SHELL=/bin/bash

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

All errors (new ones prefixed by >>):

   `.exit.text' referenced in section `.data' of sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o
   or1k-linux-ld: net/8021q/vlan_dev.o: in function `vlan_dev_change_flags':
>> net/8021q/vlan_dev.c:249: undefined reference to `br_vlan_upper_change'
   net/8021q/vlan_dev.c:249:(.text+0x1d88): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `br_vlan_upper_change'
>> or1k-linux-ld: net/8021q/vlan_dev.c:251: undefined reference to `br_vlan_upper_change'
   net/8021q/vlan_dev.c:251:(.text+0x1dc4): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `br_vlan_upper_change'


vim +249 net/8021q/vlan_dev.c

   211	
   212	/* Flags are defined in the vlan_flags enum in
   213	 * include/uapi/linux/if_vlan.h file.
   214	 */
   215	int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask)
   216	{
   217		struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
   218		u32 old_flags = vlan->flags;
   219		struct net_device *br_dev;
   220	
   221		if (mask & ~(VLAN_FLAG_REORDER_HDR | VLAN_FLAG_GVRP |
   222			     VLAN_FLAG_LOOSE_BINDING | VLAN_FLAG_MVRP |
   223			     VLAN_FLAG_BRIDGE_BINDING))
   224			return -EINVAL;
   225	
   226		vlan->flags = (old_flags & ~mask) | (flags & mask);
   227	
   228		if (!netif_running(dev))
   229			return 0;
   230	
   231		if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
   232			if (vlan->flags & VLAN_FLAG_GVRP)
   233				vlan_gvrp_request_join(dev);
   234			else
   235				vlan_gvrp_request_leave(dev);
   236		}
   237	
   238		if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
   239			if (vlan->flags & VLAN_FLAG_MVRP)
   240				vlan_mvrp_request_join(dev);
   241			else
   242				vlan_mvrp_request_leave(dev);
   243		}
   244	
   245		if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
   246		    netif_is_bridge_port(dev)) {
   247			br_dev = vlan->real_dev;
   248			if (vlan->flags & VLAN_FLAG_BRIDGE_BINDING)
 > 249				br_vlan_upper_change(br_dev, dev, true);
   250			else
 > 251				br_vlan_upper_change(br_dev, dev, false);
   252		}
   253	
   254		return 0;
   255	}
   256	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH net-next 2/3] net: 8021q: fix bridge binding behavior for vlan interfaces

[kernel test robot]

Hi Sevinj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Sevinj-Aghayeva/net-vlan-fix-bridge-binding-behavior-and-add-selftests/20220731-000455
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 63757225a93353bc2ce4499af5501eabdbbf23f9
config: hexagon-randconfig-r032-20220729 (https://download.01.org/0day-ci/archive/20220731/202207310415.XN6htXAe-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 52cd00cabf479aa7eb6dbb063b7ba41ea57bce9e)
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/intel-lab-lkp/linux/commit/993166d2a01876dc92807f74b3d72f63d25c8227
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sevinj-Aghayeva/net-vlan-fix-bridge-binding-behavior-and-add-selftests/20220731-000455
        git checkout 993166d2a01876dc92807f74b3d72f63d25c8227
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

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

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: br_vlan_upper_change
   >>> referenced by vlan_dev.c:0 (net/8021q/vlan_dev.c:0)
   >>>               8021q/vlan_dev.o:(vlan_dev_change_flags) in archive net/built-in.a
   >>> referenced by vlan_dev.c:0 (net/8021q/vlan_dev.c:0)
   >>>               8021q/vlan_dev.o:(vlan_dev_change_flags) in archive net/built-in.a
   pahole: .tmp_vmlinux.btf: No such file or directory
   ld.lld: error: .btf.vmlinux.bin.o: unknown file type

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH net-next 2/3] net: 8021q: fix bridge binding behavior for vlan interfaces

[kernel test robot]

Hi Sevinj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Sevinj-Aghayeva/net-vlan-fix-bridge-binding-behavior-and-add-selftests/20220731-000455
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 63757225a93353bc2ce4499af5501eabdbbf23f9
config: x86_64-randconfig-a002 (https://download.01.org/0day-ci/archive/20220731/202207310426.rj5ZG7FZ-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/993166d2a01876dc92807f74b3d72f63d25c8227
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sevinj-Aghayeva/net-vlan-fix-bridge-binding-behavior-and-add-selftests/20220731-000455
        git checkout 993166d2a01876dc92807f74b3d72f63d25c8227
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

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

All errors (new ones prefixed by >>):

   ld: net/8021q/vlan_dev.o: in function `vlan_dev_change_flags':
>> vlan_dev.c:(.text+0xdb2): undefined reference to `br_vlan_upper_change'

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH net-next 3/3] selftests: net: tests for bridge binding behavior

[Sevinj Aghayeva]

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@gmail.com>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: Andy Roulin <aroulin@nvidia.com>
Reviewed-by: Roopa Prabhu <roopa@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

Re: [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests

[Nikolay Aleksandrov]

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

Re: [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests

[Sevinj Aghayeva]

(Resending this because the first email was rejected due to being in HTML.)


On Sat, Jul 30, 2022 at 12:46 PM Sevinj Aghayeva
<sevinj.aghayeva@gmail.com> wrote:
>
>
>
> On Sat, Jul 30, 2022 at 12:22 PM Nikolay Aleksandrov <razor@blackwall.org> wrote:
>>
>> 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.
>
>
> Hi Nik,
>
> Can please elaborate on where I should process NETDEV_CHANGE events? I'm doing this as part of outreachy project and this is my first kernel task, so I don't know the bridging code that well.
>
> Thanks!
>
>>
>>
>> Cheers,
>>   Nik
>
>
>
> --
>
> Sevinj.Aghayeva



-- 

Sevinj.Aghayeva

Re: [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests

[Roopa Prabhu]

On 7/30/22 09:48, Sevinj Aghayeva wrote:
> (Resending this because the first email was rejected due to being in HTML.)
>
>
> On Sat, Jul 30, 2022 at 12:46 PM Sevinj Aghayeva
> <sevinj.aghayeva@gmail.com> wrote:
>>
>>
>> On Sat, Jul 30, 2022 at 12:22 PM Nikolay Aleksandrov <razor@blackwall.org> wrote:
>>> 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.
>>
>> Hi Nik,
>>
>> Can please elaborate on where I should process NETDEV_CHANGE events? I'm doing this as part of outreachy project and this is my first kernel task, so I don't know the bridging code that well.
>>
>> Thanks!

good point Nikolay.

Sevinj, see br_vlan_bridge_event and __vlan_device_event  for how both 
drivers react to netdev change events.

I have not looked at it in detail yet, but lets explore and discuss if 
we can make use of events to achieve same results.