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
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 <[email protected]>
Reviewed-by: Stefano Brivio <[email protected]>
Reviewed-by: Andy Roulin <[email protected]>
Reviewed-by: Roopa Prabhu <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Stefano Brivio <[email protected]>
Reviewed-by: Andy Roulin <[email protected]>
Reviewed-by: Roopa Prabhu <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Stefano Brivio <[email protected]>
Reviewed-by: Andy Roulin <[email protected]>
Reviewed-by: Roopa Prabhu <[email protected]>
---
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
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
(Resending this because the first email was rejected due to being in HTML.)
On Sat, Jul 30, 2022 at 12:46 PM Sevinj Aghayeva
<[email protected]> wrote:
>
>
>
> On Sat, Jul 30, 2022 at 12:22 PM Nikolay Aleksandrov <[email protected]> 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
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/[email protected]/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 <[email protected]>
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
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/[email protected]/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 <[email protected]>
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
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/[email protected]/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 <[email protected]>
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
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
> <[email protected]> wrote:
>>
>>
>> On Sat, Jul 30, 2022 at 12:22 PM Nikolay Aleksandrov <[email protected]> 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.