Commit 091adf9b authored by Nikolay Aleksandrov's avatar Nikolay Aleksandrov Committed by David S. Miller

net: bridge: move default pvid init/deinit to NETDEV_REGISTER/UNREGISTER

Most of the bridge device's vlan init bugs come from the fact that its
default pvid is created at the wrong time, way too early in ndo_init()
before the device is even assigned an ifindex. It introduces a bug when the
bridge's dev_addr is added as fdb during the initial default pvid creation
the notification has ifindex/NDA_MASTER both equal to 0 (see example below)
which really makes no sense for user-space[0] and is wrong.
Usually user-space software would ignore such entries, but they are
actually valid and will eventually have all necessary attributes.
It makes much more sense to send a notification *after* the device has
registered and has a proper ifindex allocated rather than before when
there's a chance that the registration might still fail or to receive
it with ifindex/NDA_MASTER == 0. Note that we can remove the fdb flush
from br_vlan_flush() since that case can no longer happen. At
NETDEV_REGISTER br->default_pvid is always == 1 as it's initialized by
br_vlan_init() before that and at NETDEV_UNREGISTER it can be anything
depending why it was called (if called due to NETDEV_REGISTER error
it'll still be == 1, otherwise it could be any value changed during the
device life time).

For the demonstration below a small change to iproute2 for printing all fdb
notifications is added, because it contained a workaround not to show
entries with ifindex == 0.
Command executed while monitoring: $ ip l add br0 type bridge
Before (both ifindex and master == 0):
$ bridge monitor fdb
36:7e:8a:b3:56:ba dev * vlan 1 master * permanent

After (proper br0 ifindex):
$ bridge monitor fdb
e6:2a:ae:7a:b7:48 dev br0 vlan 1 master br0 permanent

v4: move only the default pvid init/deinit to NETDEV_REGISTER/UNREGISTER
v3: send the correct v2 patch with all changes (stub should return 0)
v2: on error in br_vlan_init set br->vlgrp to NULL and return 0 in
    the br_vlan_bridge_event stub when bridge vlans are disabled

[0] https://bugzilla.kernel.org/show_bug.cgi?id=204389Reported-by: default avatarmichael-dev <michael-dev@fami-braun.de>
Fixes: 5be5a2df ("bridge: Add filtering support for default_pvid")
Signed-off-by: default avatarNikolay Aleksandrov <nikolay@cumulusnetworks.com>
Acked-by: default avatarRoopa Prabhu <roopa@cumulusnetworks.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent d934423a
...@@ -37,12 +37,15 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v ...@@ -37,12 +37,15 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
int err; int err;
if (dev->priv_flags & IFF_EBRIDGE) { if (dev->priv_flags & IFF_EBRIDGE) {
err = br_vlan_bridge_event(dev, event, ptr);
if (err)
return notifier_from_errno(err);
if (event == NETDEV_REGISTER) { if (event == NETDEV_REGISTER) {
/* register of bridge completed, add sysfs entries */ /* register of bridge completed, add sysfs entries */
br_sysfs_addbr(dev); br_sysfs_addbr(dev);
return NOTIFY_DONE; return NOTIFY_DONE;
} }
br_vlan_bridge_event(dev, event, ptr);
} }
/* not a port of a bridge */ /* not a port of a bridge */
......
...@@ -894,8 +894,8 @@ int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask); ...@@ -894,8 +894,8 @@ int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
void br_vlan_get_stats(const struct net_bridge_vlan *v, void br_vlan_get_stats(const struct net_bridge_vlan *v,
struct br_vlan_stats *stats); struct br_vlan_stats *stats);
void br_vlan_port_event(struct net_bridge_port *p, unsigned long event); void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
void br_vlan_bridge_event(struct net_device *dev, unsigned long event, int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
void *ptr); void *ptr);
static inline struct net_bridge_vlan_group *br_vlan_group( static inline struct net_bridge_vlan_group *br_vlan_group(
const struct net_bridge *br) const struct net_bridge *br)
...@@ -1085,9 +1085,10 @@ static inline void br_vlan_port_event(struct net_bridge_port *p, ...@@ -1085,9 +1085,10 @@ static inline void br_vlan_port_event(struct net_bridge_port *p,
{ {
} }
static inline void br_vlan_bridge_event(struct net_device *dev, static inline int br_vlan_bridge_event(struct net_device *dev,
unsigned long event, void *ptr) unsigned long event, void *ptr)
{ {
return 0;
} }
#endif #endif
......
...@@ -715,11 +715,6 @@ void br_vlan_flush(struct net_bridge *br) ...@@ -715,11 +715,6 @@ void br_vlan_flush(struct net_bridge *br)
ASSERT_RTNL(); ASSERT_RTNL();
/* delete auto-added default pvid local fdb before flushing vlans
* otherwise it will be leaked on bridge device init failure
*/
br_fdb_delete_by_port(br, NULL, 0, 1);
vg = br_vlan_group(br); vg = br_vlan_group(br);
__vlan_flush(vg); __vlan_flush(vg);
RCU_INIT_POINTER(br->vlgrp, NULL); RCU_INIT_POINTER(br->vlgrp, NULL);
...@@ -1058,7 +1053,6 @@ int br_vlan_init(struct net_bridge *br) ...@@ -1058,7 +1053,6 @@ int br_vlan_init(struct net_bridge *br)
{ {
struct net_bridge_vlan_group *vg; struct net_bridge_vlan_group *vg;
int ret = -ENOMEM; int ret = -ENOMEM;
bool changed;
vg = kzalloc(sizeof(*vg), GFP_KERNEL); vg = kzalloc(sizeof(*vg), GFP_KERNEL);
if (!vg) if (!vg)
...@@ -1073,17 +1067,10 @@ int br_vlan_init(struct net_bridge *br) ...@@ -1073,17 +1067,10 @@ int br_vlan_init(struct net_bridge *br)
br->vlan_proto = htons(ETH_P_8021Q); br->vlan_proto = htons(ETH_P_8021Q);
br->default_pvid = 1; br->default_pvid = 1;
rcu_assign_pointer(br->vlgrp, vg); rcu_assign_pointer(br->vlgrp, vg);
ret = br_vlan_add(br, 1,
BRIDGE_VLAN_INFO_PVID | BRIDGE_VLAN_INFO_UNTAGGED |
BRIDGE_VLAN_INFO_BRENTRY, &changed, NULL);
if (ret)
goto err_vlan_add;
out: out:
return ret; return ret;
err_vlan_add:
vlan_tunnel_deinit(vg);
err_tunnel_init: err_tunnel_init:
rhashtable_destroy(&vg->vlan_hash); rhashtable_destroy(&vg->vlan_hash);
err_rhtbl: err_rhtbl:
...@@ -1469,13 +1456,23 @@ static void nbp_vlan_set_vlan_dev_state(struct net_bridge_port *p, u16 vid) ...@@ -1469,13 +1456,23 @@ static void nbp_vlan_set_vlan_dev_state(struct net_bridge_port *p, u16 vid)
} }
/* Must be protected by RTNL. */ /* Must be protected by RTNL. */
void br_vlan_bridge_event(struct net_device *dev, unsigned long event, int br_vlan_bridge_event(struct net_device *dev, unsigned long event, void *ptr)
void *ptr)
{ {
struct netdev_notifier_changeupper_info *info; struct netdev_notifier_changeupper_info *info;
struct net_bridge *br; struct net_bridge *br = netdev_priv(dev);
bool changed;
int ret = 0;
switch (event) { switch (event) {
case NETDEV_REGISTER:
ret = br_vlan_add(br, br->default_pvid,
BRIDGE_VLAN_INFO_PVID |
BRIDGE_VLAN_INFO_UNTAGGED |
BRIDGE_VLAN_INFO_BRENTRY, &changed, NULL);
break;
case NETDEV_UNREGISTER:
br_vlan_delete(br, br->default_pvid);
break;
case NETDEV_CHANGEUPPER: case NETDEV_CHANGEUPPER:
info = ptr; info = ptr;
br_vlan_upper_change(dev, info->upper_dev, info->linking); br_vlan_upper_change(dev, info->upper_dev, info->linking);
...@@ -1483,12 +1480,13 @@ void br_vlan_bridge_event(struct net_device *dev, unsigned long event, ...@@ -1483,12 +1480,13 @@ void br_vlan_bridge_event(struct net_device *dev, unsigned long event,
case NETDEV_CHANGE: case NETDEV_CHANGE:
case NETDEV_UP: case NETDEV_UP:
br = netdev_priv(dev);
if (!br_opt_get(br, BROPT_VLAN_BRIDGE_BINDING)) if (!br_opt_get(br, BROPT_VLAN_BRIDGE_BINDING))
return; break;
br_vlan_link_state_change(dev, br); br_vlan_link_state_change(dev, br);
break; break;
} }
return ret;
} }
/* Must be protected by RTNL. */ /* Must be protected by RTNL. */
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment