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(-)
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);
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);
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;
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,
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.
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,
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 !
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.
From: David Miller davem@davemloft.net 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.
b.a.t.m.a.n@lists.open-mesh.org