[PATCH net v2 0/2] bridge: Fix kernel oops during bridge creation

[PATCH net v2 0/2] bridge: Fix kernel oops during bridge creation

[idosch]

From: Ido Schimmel <idosch@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

[PATCH net v2 1/2] bridge: implement missing ndo_uninit()

[idosch]

From: Ido Schimmel <idosch@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@cumulusnetworks.com>
Signed-off-by: Ido Schimmel <idosch@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

Re: [PATCH net v2 1/2] bridge: implement missing ndo_uninit()

[Stephen Hemminger]

On Sat, 8 Apr 2017 14:41:58 +0300
<idosch@mellanox.com> wrote:

>  static void br_dev_free(struct net_device *dev)
>  {
> -	struct net_bridge *br = netdev_priv(dev);
> -
> -	free_percpu(br->stats);
>  	free_netdev(dev);
>  }
>  

Since the only thing left is free_netdev, you can now just set dev->destructor
to be free_netdev.

Re: [PATCH net v2 1/2] bridge: implement missing ndo_uninit()

[Ido Schimmel]

On Sat, Apr 08, 2017 at 09:30:42AM -0400, Stephen Hemminger wrote:
> On Sat, 8 Apr 2017 14:41:58 +0300
> <idosch@mellanox.com> wrote:
> 
> >  static void br_dev_free(struct net_device *dev)
> >  {
> > -	struct net_bridge *br = netdev_priv(dev);
> > -
> > -	free_percpu(br->stats);
> >  	free_netdev(dev);
> >  }
> >  
> 
> Since the only thing left is free_netdev, you can now just set dev->destructor
> to be free_netdev.

Fine.

Beside stylistic issues, I would appreciate comments on how this should
be handled. Are we reverting the patch in the Fixes line or applying
this patchset?

I prefer the first option. Then after net is merged into net-next I can
re-post this patchset with the requested changes.

Re: [PATCH net v2 1/2] bridge: implement missing ndo_uninit()

[Nikolay Aleksandrov]

On 08/04/17 16:49, Ido Schimmel wrote:
> On Sat, Apr 08, 2017 at 09:30:42AM -0400, Stephen Hemminger wrote:
>> On Sat, 8 Apr 2017 14:41:58 +0300
>> <idosch@mellanox.com> wrote:
>>
>>>  static void br_dev_free(struct net_device *dev)
>>>  {
>>> -	struct net_bridge *br = netdev_priv(dev);
>>> -
>>> -	free_percpu(br->stats);
>>>  	free_netdev(dev);
>>>  }
>>>  
>>
>> Since the only thing left is free_netdev, you can now just set dev->destructor
>> to be free_netdev.
> 
> Fine.
> 
> Beside stylistic issues, I would appreciate comments on how this should
> be handled. Are we reverting the patch in the Fixes line or applying
> this patchset?
> 
> I prefer the first option. Then after net is merged into net-next I can
> re-post this patchset with the requested changes.
> 

+1

Re: [PATCH net v2 1/2] bridge: implement missing ndo_uninit()

[Stephen Hemminger]

On Sat, 8 Apr 2017 17:05:48 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> On 08/04/17 16:49, Ido Schimmel wrote:
> > On Sat, Apr 08, 2017 at 09:30:42AM -0400, Stephen Hemminger wrote:  
> >> On Sat, 8 Apr 2017 14:41:58 +0300
> >> <idosch@mellanox.com> wrote:
> >>  
> >>>  static void br_dev_free(struct net_device *dev)
> >>>  {
> >>> -	struct net_bridge *br = netdev_priv(dev);
> >>> -
> >>> -	free_percpu(br->stats);
> >>>  	free_netdev(dev);
> >>>  }
> >>>    
> >>
> >> Since the only thing left is free_netdev, you can now just set dev->destructor
> >> to be free_netdev.  
> > 
> > Fine.
> > 
> > Beside stylistic issues, I would appreciate comments on how this should
> > be handled. Are we reverting the patch in the Fixes line or applying
> > this patchset?
> > 
> > I prefer the first option. Then after net is merged into net-next I can
> > re-post this patchset with the requested changes.
> >   
> 
> +1
> 
> 

If this fixes the issue, then the one fix should go to stable, net and net-next.
There is no good reason to have two versions.

Re: [PATCH net v2 1/2] bridge: implement missing ndo_uninit()

[Ivan Vecera]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

Dne 8. 4. 2017 10:14 napsal uživatel "Stephen Hemminger" <
stephen@networkplumber.org>:

On Sat, 8 Apr 2017 17:05:48 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> On 08/04/17 16:49, Ido Schimmel wrote:
> > On Sat, Apr 08, 2017 at 09:30:42AM -0400, Stephen Hemminger wrote:
> >> On Sat, 8 Apr 2017 14:41:58 +0300
> >> <idosch@mellanox.com> wrote:
> >>
> >>>  static void br_dev_free(struct net_device *dev)
> >>>  {
> >>> - struct net_bridge *br = netdev_priv(dev);
> >>> -
> >>> - free_percpu(br->stats);
> >>>   free_netdev(dev);
> >>>  }
> >>>
> >>
> >> Since the only thing left is free_netdev, you can now just set
dev->destructor
> >> to be free_netdev.
> >
> > Fine.
> >
> > Beside stylistic issues, I would appreciate comments on how this should
> > be handled. Are we reverting the patch in the Fixes line or applying
> > this patchset?
> >
> > I prefer the first option. Then after net is merged into net-next I can
> > re-post this patchset with the requested changes.
> >
>
> +1
>
>

If this fixes the issue, then the one fix should go to stable, net and
net-next.
There is no good reason to have two versions.

+1

[-- Attachment #2: Type: text/html, Size: 2004 bytes --]

Re: [PATCH net v2 1/2] bridge: implement missing ndo_uninit()

[Ivan Vecera]

2017-04-08 16:14 GMT+02:00 Stephen Hemminger <stephen@networkplumber.org>:
> On Sat, 8 Apr 2017 17:05:48 +0300
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>
>> On 08/04/17 16:49, Ido Schimmel wrote:
>> > On Sat, Apr 08, 2017 at 09:30:42AM -0400, Stephen Hemminger wrote:
>> >> On Sat, 8 Apr 2017 14:41:58 +0300
>> >> <idosch@mellanox.com> wrote:
>> >>
>> >>>  static void br_dev_free(struct net_device *dev)
>> >>>  {
>> >>> - struct net_bridge *br = netdev_priv(dev);
>> >>> -
>> >>> - free_percpu(br->stats);
>> >>>   free_netdev(dev);
>> >>>  }
>> >>>
>> >>
>> >> Since the only thing left is free_netdev, you can now just set dev->destructor
>> >> to be free_netdev.
>> >
>> > Fine.
>> >
>> > Beside stylistic issues, I would appreciate comments on how this should
>> > be handled. Are we reverting the patch in the Fixes line or applying
>> > this patchset?
>> >
>> > I prefer the first option. Then after net is merged into net-next I can
>> > re-post this patchset with the requested changes.
>> >
>>
>> +1
>>
>>
>
> If this fixes the issue, then the one fix should go to stable, net and net-next.
> There is no good reason to have two versions.
>
+1

[PATCH net v2 2/2] bridge: netlink: register netdevice before executing changelink

[idosch]

From: Ido Schimmel <idosch@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@cumulusnetworks.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reported-by: Peter V. Saveliev <peter@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

Re: [PATCH net v2 2/2] bridge: netlink: register netdevice before executing changelink

[Stephen Hemminger]

On Sat, 8 Apr 2017 14:41:59 +0300
<idosch@mellanox.com> wrote:

> +	err = br_changelink(dev, tb, data);
> +	if (err)
> +		goto unregister;
> +
> +	return 0;
> +
> +unregister:
> +	unregister_netdevice(dev);
> +	return err;
>  }

Why use a goto? just do:
	err = br_changelink(dev, tb, data);
	if (err)
		unregister_netdevice(dev)
	return err;
}


The goto looks ugly