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 (5):
net: core: export call_netdevice_notifiers_info
net: core: introduce a new notifier for link-type-specific changes
net: 8021q: notify bridge module of bridge-binding flag change
net: bridge: handle link-type-specific changes in the bridge module
selftests: net: tests for bridge binding behavior
include/linux/if_vlan.h | 4 +
include/linux/netdevice.h | 3 +
include/linux/notifier_info.h | 21 +++
net/8021q/vlan.h | 2 +-
net/8021q/vlan_dev.c | 20 ++-
net/bridge/br.c | 5 +
net/bridge/br_private.h | 7 +
net/bridge/br_vlan.c | 18 +++
net/core/dev.c | 7 +-
tools/testing/selftests/net/Makefile | 1 +
.../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++
11 files changed, 223 insertions(+), 8 deletions(-)
create mode 100644 include/linux/notifier_info.h
create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh
--
2.34.1
The function call_netdevice_notifiers_info will be used by the vlan
module for sending link-type-specific information to other modules.
Signed-off-by: Sevinj Aghayeva <[email protected]>
---
include/linux/netdevice.h | 2 ++
net/core/dev.c | 5 ++---
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f0068c1ff1df..56b96b1e4c4c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2906,6 +2906,8 @@ netdev_notifier_info_to_extack(const struct netdev_notifier_info *info)
}
int call_netdevice_notifiers(unsigned long val, struct net_device *dev);
+int call_netdevice_notifiers_info(unsigned long val,
+ struct netdev_notifier_info *info);
extern rwlock_t dev_base_lock; /* Device list lock */
diff --git a/net/core/dev.c b/net/core/dev.c
index d66c73c1c734..e233145d1452 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -160,8 +160,6 @@ struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;
struct list_head ptype_all __read_mostly; /* Taps */
static int netif_rx_internal(struct sk_buff *skb);
-static int call_netdevice_notifiers_info(unsigned long val,
- struct netdev_notifier_info *info);
static int call_netdevice_notifiers_extack(unsigned long val,
struct net_device *dev,
struct netlink_ext_ack *extack);
@@ -1927,7 +1925,7 @@ static void move_netdevice_notifiers_dev_net(struct net_device *dev,
* are as for raw_notifier_call_chain().
*/
-static int call_netdevice_notifiers_info(unsigned long val,
+int call_netdevice_notifiers_info(unsigned long val,
struct netdev_notifier_info *info)
{
struct net *net = dev_net(info->dev);
@@ -1944,6 +1942,7 @@ static int call_netdevice_notifiers_info(unsigned long val,
return ret;
return raw_notifier_call_chain(&netdev_chain, val, info);
}
+EXPORT_SYMBOL_GPL(call_netdevice_notifiers_info);
/**
* call_netdevice_notifiers_info_robust - call per-netns notifier blocks
--
2.34.1
Notify the bridge module when a user changes the bridge-binding flag
of a VLAN interface using "ip link set ... vlan bridge_binding on"
command, so that the bridge module can take the appropriate action for
this change.
Signed-off-by: Sevinj Aghayeva <[email protected]>
---
net/8021q/vlan.h | 2 +-
net/8021q/vlan_dev.c | 20 +++++++++++++++++---
2 files changed, 18 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 e1bb41a443c4..7c61b813e654 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -22,6 +22,7 @@
#include <linux/skbuff.h>
#include <linux/netdevice.h>
#include <linux/net_tstamp.h>
+#include <linux/notifier_info.h>
#include <linux/etherdevice.h>
#include <linux/ethtool.h>
#include <linux/phy.h>
@@ -211,8 +212,9 @@ 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 netdev_notifier_change_details_info details;
struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
u32 old_flags = vlan->flags;
@@ -223,19 +225,31 @@ 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_master(vlan->real_dev)) {
+ details.info.dev = dev;
+ details.vlan.bridge_binding =
+ !!(vlan->flags & VLAN_FLAG_BRIDGE_BINDING);
+ call_netdevice_notifiers_info(NETDEV_CHANGE_DETAILS, &details.info);
+ }
+
return 0;
}
--
2.34.1
Introduce a handler to bridge module for handling vlan-specific
events.
Signed-off-by: Sevinj Aghayeva <[email protected]>
---
net/bridge/br.c | 5 +++++
net/bridge/br_private.h | 7 +++++++
net/bridge/br_vlan.c | 18 ++++++++++++++++++
3 files changed, 30 insertions(+)
diff --git a/net/bridge/br.c b/net/bridge/br.c
index 96e91d69a9a8..62e939c6a3f0 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -51,6 +51,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
}
}
+ if (is_vlan_dev(dev)) {
+ br_vlan_device_event(dev, event, ptr);
+ return NOTIFY_DONE;
+ }
+
/* not a port of a bridge */
p = br_port_get_rtnl(dev);
if (!p)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 06e5f6faa431..a9a08e49c76c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1470,6 +1470,8 @@ 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_device_event(struct net_device *dev, unsigned long event,
+ void *ptr);
void br_vlan_rtnl_init(void);
void br_vlan_rtnl_uninit(void);
void br_vlan_notify(const struct net_bridge *br,
@@ -1701,6 +1703,11 @@ static inline int br_vlan_bridge_event(struct net_device *dev,
return 0;
}
+static void br_vlan_device_event(struct net_device *dev,
+ unsigned long event, void *ptr)
+{
+}
+
static inline void br_vlan_rtnl_init(void)
{
}
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 6e53dc991409..ba4e3c7a5f03 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/kernel.h>
#include <linux/netdevice.h>
+#include <linux/notifier_info.h>
#include <linux/rtnetlink.h>
#include <linux/slab.h>
#include <net/switchdev.h>
@@ -1768,6 +1769,23 @@ void br_vlan_port_event(struct net_bridge_port *p, unsigned long event)
}
}
+void br_vlan_device_event(struct net_device *dev,
+ unsigned long event, void *ptr)
+{
+ struct netdev_notifier_change_details_info *details;
+ struct net_device *br_dev;
+
+ switch (event) {
+ case NETDEV_CHANGE_DETAILS:
+ details = ptr;
+ if (!netif_is_bridge_master(vlan_dev_priv(dev)->real_dev))
+ break;
+ br_dev = vlan_dev_priv(dev)->real_dev;
+ br_vlan_upper_change(br_dev, dev, details->vlan.bridge_binding);
+ break;
+ }
+}
+
static bool br_vlan_stats_fill(struct sk_buff *skb,
const struct net_bridge_vlan *v)
{
--
2.34.1
The netdev notification subsystem is lacking a generic notifier than
can pass link-type-specific information alongside the device. For
example, the VLAN subsystem needs to notify the bridge of a change in
the VLAN bridge brinding flag. This flag change might result in the
bridge setting VLAN devices UP or DOWN depending on the state of the
bridge ports.
Instead of introducing one new NETDEV_* notification just for this
specific use-case, introduce a generic notifier which can pass
link-type specific information. The notification’s target can decode
the union type by checking the type of the target device. That way,
other link types will be able to reuse this notification type to
notify with their own specific link specific struct. As this
notification is only for internal use, there’s no need to export it to
userspace.
Other NETDEV_* notifiers have also been looked at to see if it is
possible to consolidate:
* NETDEV_CHANGEINFODATA: this notification needs to be sent to
userspace; keep it separate from NETDEV_CHANGE_DETAILS.
* NETDEV_CHANGE: this is to notify net_device->flags change; it is
not link-type specific.
Signed-off-by: Sevinj Aghayeva <[email protected]>
---
include/linux/if_vlan.h | 4 ++++
include/linux/netdevice.h | 1 +
include/linux/notifier_info.h | 21 +++++++++++++++++++++
net/core/dev.c | 2 +-
4 files changed, 27 insertions(+), 1 deletion(-)
create mode 100644 include/linux/notifier_info.h
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index e00c4ee81ff7..38ffd2ee5112 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -37,6 +37,10 @@ struct vlan_hdr {
__be16 h_vlan_encapsulated_proto;
};
+struct vlan_change_details {
+ bool bridge_binding;
+};
+
/**
* struct vlan_ethhdr - vlan ethernet header (ethhdr + vlan_hdr)
* @h_dest: destination ethernet address
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 56b96b1e4c4c..912c04b09ebb 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2770,6 +2770,7 @@ enum netdev_cmd {
NETDEV_UNREGISTER,
NETDEV_CHANGEMTU, /* notify after mtu change happened */
NETDEV_CHANGEADDR, /* notify after the address change */
+ NETDEV_CHANGE_DETAILS,
NETDEV_PRE_CHANGEADDR, /* notify before the address change */
NETDEV_GOING_DOWN,
NETDEV_CHANGENAME,
diff --git a/include/linux/notifier_info.h b/include/linux/notifier_info.h
new file mode 100644
index 000000000000..3e53f18c6da1
--- /dev/null
+++ b/include/linux/notifier_info.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_NOTIFIER_INFO_H_
+#define _LINUX_NOTIFIER_INFO_H_
+
+#include <linux/netdevice.h>
+#include <linux/if_vlan.h>
+
+/*
+ * This struct is used for passing link-type-specific information to
+ * the device.
+ */
+
+struct netdev_notifier_change_details_info {
+ struct netdev_notifier_info info; /* must be first */
+ union {
+ struct vlan_change_details vlan;
+ };
+};
+
+#endif /* !(_LINUX_NOTIFIER_INFO_H_) */
diff --git a/net/core/dev.c b/net/core/dev.c
index e233145d1452..b50470378994 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1622,7 +1622,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER)
N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO)
N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO)
- N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
+ N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGE_DETAILS)
N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)
--
2.34.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]>
---
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 f5ac1433c301..48443928c3dd 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -44,6 +44,7 @@ TEST_PROGS += arp_ndisc_untracked_subnets.sh
TEST_PROGS += stress_reuseport_listen.sh
TEST_PROGS += l2_tos_ttl_inherit.sh
TEST_PROGS += bind_bhash.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.34.1
On 17/09/2022 23:17, 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 (5):
> net: core: export call_netdevice_notifiers_info
> net: core: introduce a new notifier for link-type-specific changes
> net: 8021q: notify bridge module of bridge-binding flag change
> net: bridge: handle link-type-specific changes in the bridge module
> selftests: net: tests for bridge binding behavior
>
> include/linux/if_vlan.h | 4 +
> include/linux/netdevice.h | 3 +
> include/linux/notifier_info.h | 21 +++
> net/8021q/vlan.h | 2 +-
> net/8021q/vlan_dev.c | 20 ++-
> net/bridge/br.c | 5 +
> net/bridge/br_private.h | 7 +
> net/bridge/br_vlan.c | 18 +++
> net/core/dev.c | 7 +-
> tools/testing/selftests/net/Makefile | 1 +
> .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++
> 11 files changed, 223 insertions(+), 8 deletions(-)
> create mode 100644 include/linux/notifier_info.h
> create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh
>
The set looks good to me, the bridge and vlan direct dependency is gone and
the new notification type is used for passing link type specific info.
If the others are ok with it I think you can send it as non-RFC, but I'd give it
a few more days at least. :)
Thanks,
Nik
On Tue, 20 Sep 2022 12:16:26 +0300 Nikolay Aleksandrov wrote:
> The set looks good to me, the bridge and vlan direct dependency is gone and
> the new notification type is used for passing link type specific info.
IDK, vlan knows it's calling the bridge:
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
+ netif_is_bridge_master(vlan->real_dev)) {
bridge knows it's vlan calling:
+ if (is_vlan_dev(dev)) {
+ br_vlan_device_event(dev, event, ptr);
going thru the generic NETDEV notifier seems odd.
If this is just to avoid the dependency we can perhaps add a stub
like net/ipv4/udp_tunnel_stub.c ?
> If the others are ok with it I think you can send it as non-RFC, but I'd give it
> a few more days at least. :)
On 21/09/2022 02:29, Jakub Kicinski wrote:
> On Tue, 20 Sep 2022 12:16:26 +0300 Nikolay Aleksandrov wrote:
>> The set looks good to me, the bridge and vlan direct dependency is gone and
>> the new notification type is used for passing link type specific info.
>
> IDK, vlan knows it's calling the bridge:
>
> + if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
> + netif_is_bridge_master(vlan->real_dev)) {
>
This one is more of an optimization so notifications are sent only when the bridge
is involved, it can be removed if other interested parties show up.
> bridge knows it's vlan calling:
>
> + if (is_vlan_dev(dev)) {
> + br_vlan_device_event(dev, event, ptr);
>
> going thru the generic NETDEV notifier seems odd.
>
> If this is just to avoid the dependency we can perhaps add a stub
> like net/ipv4/udp_tunnel_stub.c ?
>
I suggested the notifier to be more generic and be able to re-use it for other link types although
I don't have other use cases in mind right now. Stubs are an alternative as long as they and
their lifetime are properly managed. I don't have a strong preference here so if you prefer
stubs I'm good.
>> If the others are ok with it I think you can send it as non-RFC, but I'd give it
>> a few more days at least. :)
Cheers,
Nik
On Wed, 21 Sep 2022 07:45:07 +0300 Nikolay Aleksandrov wrote:
> > IDK, vlan knows it's calling the bridge:
> >
> > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
> > + netif_is_bridge_master(vlan->real_dev)) {
>
> This one is more of an optimization so notifications are sent only when the bridge
> is involved, it can be removed if other interested parties show up.
>
> > bridge knows it's vlan calling:
> >
> > + if (is_vlan_dev(dev)) {
> > + br_vlan_device_event(dev, event, ptr);
> >
> > going thru the generic NETDEV notifier seems odd.
> >
> > If this is just to avoid the dependency we can perhaps add a stub
> > like net/ipv4/udp_tunnel_stub.c ?
>
> I suggested the notifier to be more generic and be able to re-use it for other link types although
> I don't have other use cases in mind right now. Stubs are an alternative as long as they and
> their lifetime are properly managed. I don't have a strong preference here so if you prefer
> stubs I'm good.
Yup, stub seems simpler and more efficient to me. Only time will
tell if indeed this ntf type would have been reused further.. ????
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: f6390526ee0b0408d9ed03bd8607abd2a702cb36 ("[PATCH RFC net-next 4/5] net: bridge: handle link-type-specific changes in the bridge module")
url: https://github.com/intel-lab-lkp/linux/commits/Sevinj-Aghayeva/net-vlan-fix-bridge-binding-behavior-and-add-selftests/20220918-041944
base: https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git 44a8535fb87c5503ce01121278ac3058eef701ec
patch link: https://lore.kernel.org/netdev/8ef43d44ebdebe90783325cb68edb70a7c80c038.1663445339.git.sevinj.aghayeva@gmail.com
in testcase: hwsim
version: hwsim-x86_64-717e5d7-1_20220525
with following parameters:
test: group-02
on test machine: 8 threads 1 sockets Intel(R) Core(TM) i7-3770K CPU @ 3.50GHz (Ivy Bridge) with 16G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/r/[email protected]
[ 114.492414][ T4435] ------------[ cut here ]------------
[ 114.492544][ T4435] WARNING: CPU: 0 PID: 4435 at net/core/dev.c:10882 unregister_netdevice_many (net/core/dev.c:10882 (discriminator 1))
[ 114.492717][ T4435] Modules linked in: 8021q garp mrp bridge stp llc cmac ccm mac80211_hwsim mac80211 cfg80211 rfkill libarc4 netconsole btrfs blake2b_generic xor raid6_pq zstd_compress libcrc32c intel_rapl_msr intel_rapl_common x86_pkg_temp_thermal intel_powerclamp i915 sd_mod t10_pi coretemp crc64_rocksoft_generic crc64_rocksoft crc64 sg drm_buddy kvm_intel intel_gtt kvm irqbypass crct10dif_pclmul crc32_pclmul drm_display_helper crc32c_intel ghash_clmulni_intel ipmi_devintf ttm rapl ipmi_msghandler firewire_ohci drm_kms_helper ahci intel_cstate libahci firewire_core intel_uncore syscopyarea crc_itu_t sysfillrect sysimgblt libata mxm_wmi fb_sys_fops wmi video drm fuse ip_tables
[ 114.493770][ T4435] CPU: 0 PID: 4435 Comm: hostapd Not tainted 6.0.0-rc4-00989-gf6390526ee0b #1
[ 114.493948][ T4435] Hardware name: /DZ77BH-55K, BIOS BHZ7710H.86A.0097.2012.1228.1346 12/28/2012
[ 114.494119][ T4435] RIP: 0010:unregister_netdevice_many (net/core/dev.c:10882 (discriminator 1))
[ 114.494234][ T4435] Code: 73 fb ff ff ba ce 1a 00 00 48 c7 c6 60 cd e2 83 48 c7 c7 a0 cd e2 83 c6 05 41 21 3f 02 01 e8 c4 52 6f 00 0f 0b e9 4d fb ff ff <0f> 0b e9 39 fb ff ff 4c 89 0c 24 e8 e3 37 b5 fe 4c 8b 0c 24 e9 40
All code
========
0: 73 fb jae 0xfffffffffffffffd
2: ff (bad)
3: ff (bad)
4: ba ce 1a 00 00 mov $0x1ace,%edx
9: 48 c7 c6 60 cd e2 83 mov $0xffffffff83e2cd60,%rsi
10: 48 c7 c7 a0 cd e2 83 mov $0xffffffff83e2cda0,%rdi
17: c6 05 41 21 3f 02 01 movb $0x1,0x23f2141(%rip) # 0x23f215f
1e: e8 c4 52 6f 00 callq 0x6f52e7
23: 0f 0b ud2
25: e9 4d fb ff ff jmpq 0xfffffffffffffb77
2a:* 0f 0b ud2 <-- trapping instruction
2c: e9 39 fb ff ff jmpq 0xfffffffffffffb6a
31: 4c 89 0c 24 mov %r9,(%rsp)
35: e8 e3 37 b5 fe callq 0xfffffffffeb5381d
3a: 4c 8b 0c 24 mov (%rsp),%r9
3e: e9 .byte 0xe9
3f: 40 rex
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: e9 39 fb ff ff jmpq 0xfffffffffffffb40
7: 4c 89 0c 24 mov %r9,(%rsp)
b: e8 e3 37 b5 fe callq 0xfffffffffeb537f3
10: 4c 8b 0c 24 mov (%rsp),%r9
14: e9 .byte 0xe9
15: 40 rex
[ 114.494583][ T4435] RSP: 0018:ffffc900011df168 EFLAGS: 00010202
[ 114.494718][ T4435] RAX: ffff8881603d6301 RBX: ffff8881603d6b00 RCX: ffffffff812f88d3
[ 114.494871][ T4435] RDX: 1ffff11020451414 RSI: 0000000000000008 RDI: ffffffff84fb2aa0
[ 114.495004][ T4435] RBP: ffffffffa07829c0 R08: 0000000000000000 R09: ffffffff84fb2aa7
[ 114.495137][ T4435] R10: fffffbfff09f6554 R11: 0000000000000001 R12: ffff8881603d6b10
[ 114.495273][ T4435] R13: dffffc0000000000 R14: ffff8881603d6b00 R15: ffff88810228a000
[ 114.495406][ T4435] FS: 00007f67dc94a740(0000) GS:ffff88837d800000(0000) knlGS:0000000000000000
[ 114.495599][ T4435] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 114.495733][ T4435] CR2: 00007f81b15f56f4 CR3: 00000001b56f4003 CR4: 00000000001706f0
[ 114.495869][ T4435] Call Trace:
[ 114.495931][ T4435] <TASK>
[ 114.495989][ T4435] ? flush_backlog (net/core/dev.c:10803)
[ 114.496077][ T4435] ? kfree (mm/slub.c:1780 mm/slub.c:3534 mm/slub.c:4562)
[ 114.496153][ T4435] ? rcu_nocb_try_bypass (kernel/rcu/tree.c:2769)
[ 114.496247][ T4435] ? vlan_vid_del (net/8021q/vlan_core.c:368 net/8021q/vlan_core.c:387)
[ 114.496334][ T4435] vlan_device_event (net/8021q/vlan.c:528) 8021q
[ 114.496439][ T4435] ? br_del_if (net/bridge/br_if.c:743) bridge
[ 114.496572][ T4435] ? unregister_vlan_dev (net/8021q/vlan.c:362) 8021q
[ 114.496701][ T4435] ? br_switchdev_event (net/bridge/br.c:29) bridge
[ 114.496820][ T4435] ? netconsole_netdev_event (drivers/net/netconsole.c:735) netconsole
[ 114.496937][ T4435] raw_notifier_call_chain (kernel/notifier.c:92 kernel/notifier.c:455)
[ 114.497035][ T4435] unregister_netdevice_many (net/core/dev.c:10861)
[ 114.497136][ T4435] ? __kernfs_remove+0x5b7/0x840
[ 114.497235][ T4435] ? flush_backlog (net/core/dev.c:10803)
[ 114.497321][ T4435] ? kernfs_dir_pos (fs/kernfs/dir.c:1342)
[ 114.497408][ T4435] ? __cond_resched (kernel/sched/core.c:8316)
[ 114.497492][ T4435] unregister_netdevice_queue (net/core/dev.c:10792)
[ 114.497611][ T4435] ? unregister_netdevice_many (net/core/dev.c:10781)
[ 114.497747][ T4435] ? up_write (arch/x86/include/asm/atomic64_64.h:172 include/linux/atomic/atomic-long.h:95 include/linux/atomic/atomic-instrumented.h:1348 kernel/locking/rwsem.c:1356 kernel/locking/rwsem.c:1605)
[ 114.497826][ T4435] _cfg80211_unregister_wdev (include/linux/netdevice.h:3030 net/wireless/core.c:1157) cfg80211
[ 114.497985][ T4435] ? mutex_unlock (arch/x86/include/asm/atomic64_64.h:190 include/linux/atomic/atomic-long.h:449 include/linux/atomic/atomic-instrumented.h:1790 kernel/locking/mutex.c:181 kernel/locking/mutex.c:540)
[ 114.498070][ T4435] ieee80211_if_remove (net/mac80211/iface.c:2279) mac80211
[ 114.498223][ T4435] ieee80211_del_iface (net/mac80211/cfg.c:204) mac80211
[ 114.498367][ T4435] cfg80211_remove_virtual_intf (net/wireless/rdev-ops.h:62 net/wireless/util.c:2491) cfg80211
[ 114.498543][ T4435] genl_family_rcv_msg_doit (net/netlink/genetlink.c:731)
[ 114.498670][ T4435] ? genl_family_rcv_msg_attrs_parse+0x240/0x240
[ 114.498798][ T4435] ? security_capable (security/security.c:807 (discriminator 13))
[ 114.498888][ T4435] genl_rcv_msg (net/netlink/genetlink.c:778 net/netlink/genetlink.c:795)
[ 114.498970][ T4435] ? genl_get_cmd (net/netlink/genetlink.c:784)
[ 114.499054][ T4435] ? netlink_recvmsg (net/netlink/af_netlink.c:2000)
[ 114.499142][ T4435] ? nl80211_stop_ap (net/wireless/nl80211.c:4295) cfg80211
[ 114.499290][ T4435] ? kasan_set_free_info (mm/kasan/generic.c:372)
[ 114.499381][ T4435] ? __kasan_slab_free (mm/kasan/common.c:369 mm/kasan/common.c:329 mm/kasan/common.c:375)
[ 114.499473][ T4435] ? kmem_cache_free (mm/slub.c:1780 mm/slub.c:3534 mm/slub.c:3551)
[ 114.499589][ T4435] ? netlink_recvmsg (net/netlink/af_netlink.c:2000)
[ 114.499700][ T4435] ? ____sys_recvmsg (net/socket.c:2701)
[ 114.499789][ T4435] ? ___sys_recvmsg (net/socket.c:2744)
[ 114.499876][ T4435] ? __sys_recvmsg (include/linux/file.h:31 net/socket.c:2775)
[ 114.499961][ T4435] netlink_rcv_skb (net/netlink/af_netlink.c:2540)
[ 114.500046][ T4435] ? genl_get_cmd (net/netlink/genetlink.c:784)
[ 114.500132][ T4435] ? netlink_ack (net/netlink/af_netlink.c:2517)
[ 114.500216][ T4435] ? netlink_recvmsg (net/netlink/af_netlink.c:514)
[ 114.500306][ T4435] ? _copy_from_iter (lib/iov_iter.c:628 (discriminator 11))
[ 114.500400][ T4435] genl_rcv (net/netlink/genetlink.c:807)
[ 114.500474][ T4435] netlink_unicast (net/netlink/af_netlink.c:1320 net/netlink/af_netlink.c:1345)
[ 114.500607][ T4435] ? netlink_attachskb (net/netlink/af_netlink.c:1330)
[ 114.500720][ T4435] ? check_heap_object (arch/x86/include/asm/bitops.h:207 arch/x86/include/asm/bitops.h:239 include/asm-generic/bitops/instrumented-non-atomic.h:142 include/linux/page-flags.h:487 mm/usercopy.c:193)
[ 114.500812][ T4435] netlink_sendmsg (net/netlink/af_netlink.c:1921)
[ 114.500898][ T4435] ? netlink_unicast (net/netlink/af_netlink.c:1841)
[ 114.500987][ T4435] ? __import_iovec (lib/iov_iter.c:1771)
[ 114.501073][ T4435] ? netlink_unicast (net/netlink/af_netlink.c:1841)
[ 114.501161][ T4435] sock_sendmsg (net/socket.c:714 net/socket.c:734)
[ 114.501242][ T4435] ____sys_sendmsg (net/socket.c:2482)
[ 114.501330][ T4435] ? kernel_sendmsg (net/socket.c:2429)
[ 114.501414][ T4435] ? __copy_msghdr (net/socket.c:2409)
[ 114.501500][ T4435] ___sys_sendmsg (net/socket.c:2538)
[ 114.501612][ T4435] ? __ia32_sys_recvmmsg (net/socket.c:2525)
[ 114.501728][ T4435] ? __fsnotify_update_child_dentry_flags (fs/notify/fsnotify.c:180)
[ 114.501846][ T4435] ? __generic_file_write_iter (mm/filemap.c:3866)
[ 114.501948][ T4435] ? _copy_from_user (arch/x86/include/asm/uaccess_64.h:46 arch/x86/include/asm/uaccess_64.h:52 lib/usercopy.c:16)
[ 114.502035][ T4435] ? netlink_setsockopt (net/netlink/af_netlink.c:1630)
[ 114.502130][ T4435] ? __fget_light (arch/x86/include/asm/atomic.h:29 include/linux/atomic/atomic-instrumented.h:28 fs/file.c:1005)
[ 114.502213][ T4435] __sys_sendmsg (include/linux/file.h:31 net/socket.c:2567)
[ 114.502297][ T4435] ? __sys_sendmsg_sock (net/socket.c:2553)
[ 114.502386][ T4435] ? __sys_setsockopt (net/socket.c:2254)
[ 114.502477][ T4435] ? __x64_sys_setsockopt (net/socket.c:2260)
[ 114.502598][ T4435] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
[ 114.502703][ T4435] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
[ 114.502809][ T4435] RIP: 0033:0x7f67dca8d2c3
[ 114.502890][ T4435] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b7 66 2e 0f 1f 84 00 00 00 00 00 90 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 48 83 ec 28 89 54 24 1c 48
All code
========
0: 64 89 02 mov %eax,%fs:(%rdx)
3: 48 c7 c0 ff ff ff ff mov $0xffffffffffffffff,%rax
a: eb b7 jmp 0xffffffffffffffc3
c: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
13: 00 00 00
16: 90 nop
17: 64 8b 04 25 18 00 00 mov %fs:0x18,%eax
1e: 00
1f: 85 c0 test %eax,%eax
21: 75 14 jne 0x37
23: b8 2e 00 00 00 mov $0x2e,%eax
28: 0f 05 syscall
2a:* 48 3d 00 f0 ff ff cmp $0xfffffffffffff000,%rax <-- trapping instruction
30: 77 55 ja 0x87
32: c3 retq
33: 0f 1f 40 00 nopl 0x0(%rax)
37: 48 83 ec 28 sub $0x28,%rsp
3b: 89 54 24 1c mov %edx,0x1c(%rsp)
3f: 48 rex.W
Code starting with the faulting instruction
===========================================
0: 48 3d 00 f0 ff ff cmp $0xfffffffffffff000,%rax
6: 77 55 ja 0x5d
8: c3 retq
9: 0f 1f 40 00 nopl 0x0(%rax)
d: 48 83 ec 28 sub $0x28,%rsp
11: 89 54 24 1c mov %edx,0x1c(%rsp)
15: 48 rex.W
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://01.org/lkp