idosch at mellanox.com
2017-Apr-08 11:41 UTC
[Bridge] [PATCH net v2 0/2] bridge: Fix kernel oops during bridge creation
From: Ido Schimmel <idosch at mellanox.com> First patch adds a missing ndo_uninit() in the bridge driver, which is a prerequisite for the second patch that actually fixes the oops. First version was rejected for being "half baked", but given the amount of changes in this version and that three stable kernels need to be patched, a better approach might be to simply revert the patch in the Fixes line and have this patchset go through net-next. Ido Schimmel (2): bridge: implement missing ndo_uninit() bridge: netlink: register netdevice before executing changelink net/bridge/br_device.c | 13 ++++++++++--- net/bridge/br_if.c | 1 - net/bridge/br_multicast.c | 7 +++++-- net/bridge/br_netlink.c | 12 ++++++++++-- net/bridge/br_private.h | 5 +++++ 5 files changed, 30 insertions(+), 8 deletions(-) -- 2.9.3
idosch at mellanox.com
2017-Apr-08 11:41 UTC
[Bridge] [PATCH net v2 1/2] bridge: implement missing ndo_uninit()
From: Ido Schimmel <idosch at mellanox.com>
While the bridge driver implements an ndo_init(), it was missing a
symmetric ndo_uninit(), causing the different de-initialization
operations to be scattered around its dellink() and destructor().
Implement a symmetric ndo_uninit() and remove the overlapping operations
from its dellink() and destructor().
This is a prerequisite for the next patch, as it allows us to have a
proper cleanup upon changelink() failure during the bridge's newlink().
Fixes: b6677449dff6 ("bridge: netlink: call br_changelink() during
br_dev_newlink()")
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Signed-off-by: Ido Schimmel <idosch at mellanox.com>
---
net/bridge/br_device.c | 13 ++++++++++---
net/bridge/br_if.c | 1 -
net/bridge/br_multicast.c | 7 +++++--
net/bridge/br_private.h | 5 +++++
4 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index ea71513..607210f 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -119,6 +119,15 @@ static int br_dev_init(struct net_device *dev)
return err;
}
+static void br_dev_uninit(struct net_device *dev)
+{
+ struct net_bridge *br = netdev_priv(dev);
+
+ br_multicast_uninit_stats(br);
+ br_vlan_flush(br);
+ free_percpu(br->stats);
+}
+
static int br_dev_open(struct net_device *dev)
{
struct net_bridge *br = netdev_priv(dev);
@@ -332,6 +341,7 @@ static const struct net_device_ops br_netdev_ops = {
.ndo_open = br_dev_open,
.ndo_stop = br_dev_stop,
.ndo_init = br_dev_init,
+ .ndo_uninit = br_dev_uninit,
.ndo_start_xmit = br_dev_xmit,
.ndo_get_stats64 = br_get_stats64,
.ndo_set_mac_address = br_set_mac_address,
@@ -358,9 +368,6 @@ static const struct net_device_ops br_netdev_ops = {
static void br_dev_free(struct net_device *dev)
{
- struct net_bridge *br = netdev_priv(dev);
-
- free_percpu(br->stats);
free_netdev(dev);
}
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 8ac1770..56a2a72 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -311,7 +311,6 @@ void br_dev_delete(struct net_device *dev, struct list_head
*head)
br_fdb_delete_by_port(br, NULL, 0, 1);
- br_vlan_flush(br);
br_multicast_dev_del(br);
cancel_delayed_work_sync(&br->gc_work);
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index b760f26..faa7261 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -2031,8 +2031,6 @@ void br_multicast_dev_del(struct net_bridge *br)
out:
spin_unlock_bh(&br->multicast_lock);
-
- free_percpu(br->mcast_stats);
}
int br_multicast_set_router(struct net_bridge *br, unsigned long val)
@@ -2531,6 +2529,11 @@ int br_multicast_init_stats(struct net_bridge *br)
return 0;
}
+void br_multicast_uninit_stats(struct net_bridge *br)
+{
+ free_percpu(br->mcast_stats);
+}
+
static void mcast_stats_add_dir(u64 *dst, u64 *src)
{
dst[BR_MCAST_DIR_RX] += src[BR_MCAST_DIR_RX];
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 6136818..0d17728 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -620,6 +620,7 @@ void br_rtr_notify(struct net_device *dev, struct
net_bridge_port *port,
void br_multicast_count(struct net_bridge *br, const struct net_bridge_port *p,
const struct sk_buff *skb, u8 type, u8 dir);
int br_multicast_init_stats(struct net_bridge *br);
+void br_multicast_uninit_stats(struct net_bridge *br);
void br_multicast_get_stats(const struct net_bridge *br,
const struct net_bridge_port *p,
struct br_mcast_stats *dest);
@@ -760,6 +761,10 @@ static inline int br_multicast_init_stats(struct net_bridge
*br)
return 0;
}
+static inline void br_multicast_uninit_stats(struct net_bridge *br)
+{
+}
+
static inline int br_multicast_igmp_type(const struct sk_buff *skb)
{
return 0;
--
2.9.3
idosch at mellanox.com
2017-Apr-08 11:41 UTC
[Bridge] [PATCH net v2 2/2] bridge: netlink: register netdevice before executing changelink
From: Ido Schimmel <idosch at mellanox.com>
Peter reported a kernel oops when executing the following command:
$ ip link add name test type bridge vlan_default_pvid 1
[13634.939408] BUG: unable to handle kernel NULL pointer dereference at
0000000000000190
[13634.939436] IP: __vlan_add+0x73/0x5f0
[...]
[13634.939783] Call Trace:
[13634.939791] ? pcpu_next_unpop+0x3b/0x50
[13634.939801] ? pcpu_alloc+0x3d2/0x680
[13634.939810] ? br_vlan_add+0x135/0x1b0
[13634.939820] ? __br_vlan_set_default_pvid.part.28+0x204/0x2b0
[13634.939834] ? br_changelink+0x120/0x4e0
[13634.939844] ? br_dev_newlink+0x50/0x70
[13634.939854] ? rtnl_newlink+0x5f5/0x8a0
[13634.939864] ? rtnl_newlink+0x176/0x8a0
[13634.939874] ? mem_cgroup_commit_charge+0x7c/0x4e0
[13634.939886] ? rtnetlink_rcv_msg+0xe1/0x220
[13634.939896] ? lookup_fast+0x52/0x370
[13634.939905] ? rtnl_newlink+0x8a0/0x8a0
[13634.939915] ? netlink_rcv_skb+0xa1/0xc0
[13634.939925] ? rtnetlink_rcv+0x24/0x30
[13634.939934] ? netlink_unicast+0x177/0x220
[13634.939944] ? netlink_sendmsg+0x2fe/0x3b0
[13634.939954] ? _copy_from_user+0x39/0x40
[13634.939964] ? sock_sendmsg+0x30/0x40
[13634.940159] ? ___sys_sendmsg+0x29d/0x2b0
[13634.940326] ? __alloc_pages_nodemask+0xdf/0x230
[13634.940478] ? mem_cgroup_commit_charge+0x7c/0x4e0
[13634.940592] ? mem_cgroup_try_charge+0x76/0x1a0
[13634.940701] ? __handle_mm_fault+0xdb9/0x10b0
[13634.940809] ? __sys_sendmsg+0x51/0x90
[13634.940917] ? entry_SYSCALL_64_fastpath+0x1e/0xad
The problem is that the bridge's VLAN group is created after setting the
default PVID, when registering the netdevice and executing its
ndo_init().
Fix this by changing the order of both operations, so that
br_changelink() is only processed after the netdevice is registered,
when the VLAN group is already initialized.
Fixes: b6677449dff6 ("bridge: netlink: call br_changelink() during
br_dev_newlink()")
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Signed-off-by: Ido Schimmel <idosch at mellanox.com>
Reported-by: Peter V. Saveliev <peter at svinota.eu>
---
net/bridge/br_netlink.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index a8f6acd..c38104e 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1165,11 +1165,19 @@ static int br_dev_newlink(struct net *src_net, struct
net_device *dev,
spin_unlock_bh(&br->lock);
}
- err = br_changelink(dev, tb, data);
+ err = register_netdevice(dev);
if (err)
return err;
- return register_netdevice(dev);
+ err = br_changelink(dev, tb, data);
+ if (err)
+ goto unregister;
+
+ return 0;
+
+unregister:
+ unregister_netdevice(dev);
+ return err;
}
static size_t br_get_size(const struct net_device *brdev)
--
2.9.3