pull request [net]: batman-adv 20160216

pull request [net]: batman-adv 20160216

[Antonio Quartulli]

Hello David,

this pull request is intended for net.

Two of the fixes included in this patchset prevent a wrong memory
access - it was triggered when removing an object from a list
after it was already free'd due to bad reference counting.
This misbehaviour existed for both the gw_node and the
orig_node_vlan object and has been fixed by Sven Eckelmann.

The last patch fixes our interface feasibility check and prevents
it from looping indefinitely when two net_device objects
reference each other via iflink index (i.e. veth pair), by
Andrew Lunn


Please pull or let me know of any problem!
Thanks a lot,
	Antonio


The following changes since commit db92ea5d4df00271b57d79c2d03dae5a5d60fcc1:

  dscc4: Undefined signed int shift (2016-02-13 06:10:21 -0500)

are available in the git repository at:

  git://git.open-mesh.org/linux-merge.git tags/batman-adv-fix-for-davem

for you to fetch changes up to 1bc4e2b000e7fa9773d6623bc8850561ce10a4fb:

  batman-adv: Avoid endless loop in bat-on-bat netdevice check (2016-02-16 22:16:33 +0800)

----------------------------------------------------------------
Two of the fixes included in this patchset prevent wrong memory
access - it was triggered when removing an object from a list
after it was already free'd due to bad reference counting.
This misbehaviour existed for both the gw_node and the
orig_node_vlan object and has been fixed by Sven Eckelmann.

The last patch fixes our interface feasibility check and prevents
it from looping indefinitely when two net_device objects
reference each other via iflink index (i.e. veth pair), by
Andrew Lunn

----------------------------------------------------------------
Andrew Lunn (1):
      batman-adv: Avoid endless loop in bat-on-bat netdevice check

Sven Eckelmann (2):
      batman-adv: Only put gw_node list reference when removed
      batman-adv: Only put orig_node_vlan list reference when removed

 net/batman-adv/gateway_client.c    |  7 ++++---
 net/batman-adv/hard-interface.c    | 25 +++++++++++++++++++++++++
 net/batman-adv/translation-table.c |  6 ++++--
 3 files changed, 33 insertions(+), 5 deletions(-)

[PATCH 1/3] batman-adv: Only put gw_node list reference when removed

[Antonio Quartulli]

From: Sven Eckelmann <sven@narfation.org>

The batadv_gw_node reference counter in batadv_gw_node_update can only be
reduced when the list entry was actually removed. Otherwise the reference
counter may reach zero when batadv_gw_node_update is called from two
different contexts for the same gw_node but only one context is actually
removing the entry from the list.

The release function for this gw_node is not called inside the list_lock
spinlock protected region because the function batadv_gw_node_update still
holds a gw_node reference for the object pointer on the stack. Thus the
actual release function (when required) will be called only at the end of
the function.

Fixes: bd3524c14bd0 ("batman-adv: remove obsolete deleted attribute for gateway node")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 net/batman-adv/gateway_client.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/batman-adv/gateway_client.c b/net/batman-adv/gateway_client.c
index e6c8382c79ba..ccf70bed0d0c 100644
--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
@@ -527,11 +527,12 @@ void batadv_gw_node_update(struct batadv_priv *bat_priv,
 		 * gets dereferenced.
 		 */
 		spin_lock_bh(&bat_priv->gw.list_lock);
-		hlist_del_init_rcu(&gw_node->list);
+		if (!hlist_unhashed(&gw_node->list)) {
+			hlist_del_init_rcu(&gw_node->list);
+			batadv_gw_node_free_ref(gw_node);
+		}
 		spin_unlock_bh(&bat_priv->gw.list_lock);
 
-		batadv_gw_node_free_ref(gw_node);
-
 		curr_gw = batadv_gw_get_selected_gw_node(bat_priv);
 		if (gw_node == curr_gw)
 			batadv_gw_reselect(bat_priv);
-- 
2.7.1

[PATCH 2/3] batman-adv: Only put orig_node_vlan list reference when removed

[Antonio Quartulli]

From: Sven Eckelmann <sven@narfation.org>

The batadv_orig_node_vlan reference counter in batadv_tt_global_size_mod
can only be reduced when the list entry was actually removed. Otherwise the
reference counter may reach zero when batadv_tt_global_size_mod is called
from two different contexts for the same orig_node_vlan but only one
context is actually removing the entry from the list.

The release function for this orig_node_vlan is not called inside the
vlan_list_lock spinlock protected region because the function
batadv_tt_global_size_mod still holds a orig_node_vlan reference for the
object pointer on the stack. Thus the actual release function (when
required) will be called only at the end of the function.

Fixes: 7ea7b4a14275 ("batman-adv: make the TT CRC logic VLAN specific")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 net/batman-adv/translation-table.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index cdfc85fa2743..0e80fd1461ab 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -303,9 +303,11 @@ static void batadv_tt_global_size_mod(struct batadv_orig_node *orig_node,
 
 	if (atomic_add_return(v, &vlan->tt.num_entries) == 0) {
 		spin_lock_bh(&orig_node->vlan_list_lock);
-		hlist_del_init_rcu(&vlan->list);
+		if (!hlist_unhashed(&vlan->list)) {
+			hlist_del_init_rcu(&vlan->list);
+			batadv_orig_node_vlan_free_ref(vlan);
+		}
 		spin_unlock_bh(&orig_node->vlan_list_lock);
-		batadv_orig_node_vlan_free_ref(vlan);
 	}
 
 	batadv_orig_node_vlan_free_ref(vlan);
-- 
2.7.1

[PATCH 3/3] batman-adv: Avoid endless loop in bat-on-bat netdevice check

[Antonio Quartulli]

From: Andrew Lunn <andrew@lunn.ch>

batman-adv checks in different situation if a new device is already on top
of a different batman-adv device. This is done by getting the iflink of a
device and all its parent. It assumes that this iflink is always a parent
device in an acyclic graph. But this assumption is broken by devices like
veth which are actually a pair of two devices linked to each other. The
recursive check would therefore get veth0 when calling dev_get_iflink on
veth1. And it gets veth0 when calling dev_get_iflink with veth1.

Creating a veth pair and loading batman-adv freezes parts of the system

    ip link add veth0 type veth peer name veth1
    modprobe batman-adv

An RCU stall will be detected on the system which cannot be fixed.

    INFO: rcu_sched self-detected stall on CPU
            1: (5264 ticks this GP) idle=3e9/140000000000001/0
    softirq=144683/144686 fqs=5249
             (t=5250 jiffies g=46 c=45 q=43)
    Task dump for CPU 1:
    insmod          R  running task        0   247    245 0x00000008
     ffffffff8151f140 ffffffff8107888e ffff88000fd141c0 ffffffff8151f140
     0000000000000000 ffffffff81552df0 ffffffff8107b420 0000000000000001
     ffff88000e3fa700 ffffffff81540b00 ffffffff8107d667 0000000000000001
    Call Trace:
     <IRQ>  [<ffffffff8107888e>] ? rcu_dump_cpu_stacks+0x7e/0xd0
     [<ffffffff8107b420>] ? rcu_check_callbacks+0x3f0/0x6b0
     [<ffffffff8107d667>] ? hrtimer_run_queues+0x47/0x180
     [<ffffffff8107cf9d>] ? update_process_times+0x2d/0x50
     [<ffffffff810873fb>] ? tick_handle_periodic+0x1b/0x60
     [<ffffffff810290ae>] ? smp_trace_apic_timer_interrupt+0x5e/0x90
     [<ffffffff813bbae2>] ? apic_timer_interrupt+0x82/0x90
     <EOI>  [<ffffffff812c3fd7>] ? __dev_get_by_index+0x37/0x40
     [<ffffffffa0031f3e>] ? batadv_hard_if_event+0xee/0x3a0 [batman_adv]
     [<ffffffff812c5801>] ? register_netdevice_notifier+0x81/0x1a0
    [...]

This can be avoided by checking if two devices are each others parent and
stopping the check in this situation.

Fixes: b7eddd0b3950 ("batman-adv: prevent using any virtual device created on batman-adv as hard-interface")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
[sven@narfation.org: rewritten description, extracted fix]
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 net/batman-adv/hard-interface.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
index 01acccc4d218..57f7107169f5 100644
--- a/net/batman-adv/hard-interface.c
+++ b/net/batman-adv/hard-interface.c
@@ -76,6 +76,28 @@ out:
 }
 
 /**
+ * batadv_mutual_parents - check if two devices are each others parent
+ * @dev1: 1st net_device
+ * @dev2: 2nd net_device
+ *
+ * veth devices come in pairs and each is the parent of the other!
+ *
+ * Return: true if the devices are each others parent, otherwise false
+ */
+static bool batadv_mutual_parents(const struct net_device *dev1,
+				  const struct net_device *dev2)
+{
+	int dev1_parent_iflink = dev_get_iflink(dev1);
+	int dev2_parent_iflink = dev_get_iflink(dev2);
+
+	if (!dev1_parent_iflink || !dev2_parent_iflink)
+		return false;
+
+	return (dev1_parent_iflink == dev2->ifindex) &&
+	       (dev2_parent_iflink == dev1->ifindex);
+}
+
+/**
  * batadv_is_on_batman_iface - check if a device is a batman iface descendant
  * @net_dev: the device to check
  *
@@ -108,6 +130,9 @@ static bool batadv_is_on_batman_iface(const struct net_device *net_dev)
 	if (WARN(!parent_dev, "Cannot find parent device"))
 		return false;
 
+	if (batadv_mutual_parents(net_dev, parent_dev))
+		return false;
+
 	ret = batadv_is_on_batman_iface(parent_dev);
 
 	return ret;
-- 
2.7.1

Re: [B.A.T.M.A.N.] pull request [net]: batman-adv 20160216

[Antonio Quartulli]

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

On Tue, Feb 16, 2016 at 11:01:25PM +0800, Antonio Quartulli wrote:
> Hello David,
> 
> this pull request is intended for net.

David,

when merging net into net-next these patches will create a conflict which git
should try to fix on its own.
However, it will still ask you to confirm something. Here are the relevant
chunks:

--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
@@@ -544,11 -527,12 +544,17 @@@ void batadv_gw_node_update(struct batad
                 * gets dereferenced.
                 */
                spin_lock_bh(&bat_priv->gw.list_lock);
-               hlist_del_init_rcu(&gw_node->list);
+               if (!hlist_unhashed(&gw_node->list)) {
+                       hlist_del_init_rcu(&gw_node->list);
+                       batadv_gw_node_free_ref(gw_node);
+               }
                spin_unlock_bh(&bat_priv->gw.list_lock);
  
++<<<<<<< HEAD
 +              batadv_gw_node_put(gw_node);
 +
++=======
++>>>>>>> maint



if you have this conflict, please keep the "maint" block.
Moreover, make sure that every reference to
batadv_gw_node_free_ref() in this file is substituted with
batadv_gw_node_put() (there should be only one).



diff --cc net/batman-adv/translation-table.c
index 2fd5b28,7301a92..0000000
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@@ -346,12 -311,14 +346,18 @@@ static void batadv_tt_global_size_mod(s
  
        if (atomic_add_return(v, &vlan->tt.num_entries) == 0) {
                spin_lock_bh(&orig_node->vlan_list_lock);
-               hlist_del_init_rcu(&vlan->list);
+               if (!hlist_unhashed(&vlan->list)) {
+                       hlist_del_init_rcu(&vlan->list);
+                       batadv_orig_node_vlan_free_ref(vlan);
+               }
                spin_unlock_bh(&orig_node->vlan_list_lock);
++<<<<<<< HEAD
 +              batadv_orig_node_vlan_put(vlan);
++=======
++>>>>>>> maint



if you have this conflict, please keep the "maint" block.
Moreover, make sure that every reference to
batadv_orig_node_vlan_free_ref() in this file is substituted with
batadv_orig_node_vlan_put() (there should be only one).



Obviously, I can check the final result after your merge operation.
Thanks!


Cheers,


-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: pull request [net]: batman-adv 20160216

[David Miller]

From: Antonio Quartulli <a@unstable.cc>
Date: Tue, 16 Feb 2016 23:01:25 +0800

> this pull request is intended for net.
> 
> Two of the fixes included in this patchset prevent a wrong memory
> access - it was triggered when removing an object from a list
> after it was already free'd due to bad reference counting.
> This misbehaviour existed for both the gw_node and the
> orig_node_vlan object and has been fixed by Sven Eckelmann.
> 
> The last patch fixes our interface feasibility check and prevents
> it from looping indefinitely when two net_device objects
> reference each other via iflink index (i.e. veth pair), by
> Andrew Lunn

Pulled, thanks Antonio.

And thanks for the heads up about the potential merge issues, I'll watch
for that.

Re: pull request [net]: batman-adv 20160216

[Antonio Quartulli]

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

On Fri, Feb 19, 2016 at 03:37:18PM -0500, David Miller wrote:
> And thanks for the heads up about the potential merge issues, I'll watch
> for that.
> 

Hi David,

actually I just realized that the patches that will create the conflict
are not yet in net-next, but they are still pending in my queue.

At this point I will wait for you to merge net into net-next first
(there should be no conflict at that point) and then I will rebase
my pending patches on top of that.


This should prevent you from dealing with any conflict.


Regards,


-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] pull request [net]: batman-adv 20160216

[Antonio Quartulli]

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

On Sat, Feb 20, 2016 at 01:28:40PM +0800, Antonio Quartulli wrote:
> On Fri, Feb 19, 2016 at 03:37:18PM -0500, David Miller wrote:
> > And thanks for the heads up about the potential merge issues, I'll watch
> > for that.
> > 
> 
> Hi David,
> 
> actually I just realized that the patches that will create the conflict
> are not yet in net-next, but they are still pending in my queue.
> 
> At this point I will wait for you to merge net into net-next first
> (there should be no conflict at that point) and then I will rebase
> my pending patches on top of that.
> 
> 
> This should prevent you from dealing with any conflict.

Hi David,

I know this kind of mails steal you some time, but do you have any plan
about merging net into net-next in the next days ?

I have some more new changes coming, but I'd like to send them only after
the merge operation is done to avoid you to deal with some conflicts (as per
above).

Thanks !


-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] pull request [net]: batman-adv 20160216

[David Miller]

From: Antonio Quartulli <a@unstable.cc>
Date: Tue, 23 Feb 2016 13:07:50 +0800

> I know this kind of mails steal you some time, but do you have any plan
> about merging net into net-next in the next days ?

It's what I'm working on right now.

Re: pull request [net]: batman-adv 20160216

[David Miller]

From: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Date: Tue, 23 Feb 2016 00:12:35 -0500 (EST)

> From: Antonio Quartulli <a@unstable.cc>
> Date: Tue, 23 Feb 2016 13:07:50 +0800
> 
>> I know this kind of mails steal you some time, but do you have any plan
>> about merging net into net-next in the next days ?
> 
> It's what I'm working on right now.

And, done.