[B.A.T.M.A.N.] [PATCHv2 1/3] batman-adv: Fix crash on module shutdown with multiple ifaces

List overview All Threads
Download

newer

older

[B.A.T.M.A.N.] B.A.T.M.A.N....

Re: [B.A.T.M.A.N.] [PATCHv2 1/3]...

Sven Eckelmann

30 Apr 2011 30 Apr '11

4:56 p.m.

From: Linus Lüssing <linus.luessing(a)web.de> hardif_remove_interfaces() removes all hard interfaces from the hardif_list before freeing and cleaning up any device. However the clean up procedures in orig_hash_del_if() (hardif_remove_interface()->hardif_disable_interface()-> orig_hash_del_if()) need the other interfaces still to be present in the hardif_list. Otherwise it won't renumber any preceding interfaces, which leads to an unhandled kernel paging request in orig_node_del_if()'s "/* copy second part */" due to wrong hard_if->if_num's. With this commit the interface removal on module shutdown will be down in the same way as removing single interfaces from batman only: One interface will be removed and cleaned at a time. Signed-off-by: Linus Lüssing <linus.luessing(a)web.de> [sven(a)narfation.org: Keep locking around list_for_each_entry_safe] Signed-off-by: Sven Eckelmann <sven(a)narfation.org> --- Splitted the first patch to keep Linus' fame. There should be no difference (after all patches are applied) between the first and the second version. hard-interface.c | 11 ++--------- 1 files changed, 2 insertions(+), 9 deletions(-) diff --git a/hard-interface.c b/hard-interface.c index b3058e4..c6edf0a 100644 --- a/hard-interface.c +++ b/hard-interface.c @@ -490,22 +490,15 @@ static void hardif_remove_interface(struct hard_iface *hard_iface) void hardif_remove_interfaces(void) { struct hard_iface *hard_iface, *hard_iface_tmp; - struct list_head if_queue; - - INIT_LIST_HEAD(&if_queue); + rtnl_lock(); spin_lock(&hardif_list_lock); list_for_each_entry_safe(hard_iface, hard_iface_tmp, &hardif_list, list) { list_del_rcu(&hard_iface->list); - list_add_tail(&hard_iface->list, &if_queue); - } - spin_unlock(&hardif_list_lock); - - rtnl_lock(); - list_for_each_entry_safe(hard_iface, hard_iface_tmp, &if_queue, list) { hardif_remove_interface(hard_iface); } + spin_unlock(&hardif_list_lock); rtnl_unlock(); } -- 1.7.4.4

Show replies by date

Sven Eckelmann

30 Apr 30 Apr

4:56 p.m.

New subject: [B.A.T.M.A.N.] [PATCHv2 2/3] batman-adv: Remove unnecessary hardif_list_lock

hardif_list_lock is unneccessary because we already ensure that no multiple admin operations can take place through rtnl_lock. hardif_list_lock only adds additional overhead and complexity. Critical functions now check whether they are called with rtnl_lock using ASSERT_RTNL. Signed-off-by: Sven Eckelmann <sven(a)narfation.org> --- Splitted the first patch to keep Linus' fame. There should be no difference (after all patches are applied) between the first and the second version. hard-interface.c | 14 ++++---------- 1 files changed, 4 insertions(+), 10 deletions(-) diff --git a/hard-interface.c b/hard-interface.c index c6edf0a..ebd7ef0 100644 --- a/hard-interface.c +++ b/hard-interface.c @@ -31,9 +31,6 @@ #include <linux/if_arp.h> -/* protect update critical side of hardif_list - but not the content */ -static DEFINE_SPINLOCK(hardif_list_lock); - static int batman_skb_recv(struct sk_buff *skb, struct net_device *dev, @@ -432,6 +429,8 @@ static struct hard_iface *hardif_add_interface(struct net_device *net_dev) struct hard_iface *hard_iface; int ret; + ASSERT_RTNL(); + ret = is_valid_iface(net_dev); if (ret != 1) goto out; @@ -458,10 +457,7 @@ static struct hard_iface *hardif_add_interface(struct net_device *net_dev) atomic_set(&hard_iface->refcount, 2); check_known_mac_addr(hard_iface->net_dev); - - spin_lock(&hardif_list_lock); list_add_tail_rcu(&hard_iface->list, &hardif_list); - spin_unlock(&hardif_list_lock); return hard_iface; @@ -475,6 +471,8 @@ out: static void hardif_remove_interface(struct hard_iface *hard_iface) { + ASSERT_RTNL(); + /* first deactivate interface */ if (hard_iface->if_status != IF_NOT_IN_USE) hardif_disable_interface(hard_iface); @@ -492,13 +490,11 @@ void hardif_remove_interfaces(void) struct hard_iface *hard_iface, *hard_iface_tmp; rtnl_lock(); - spin_lock(&hardif_list_lock); list_for_each_entry_safe(hard_iface, hard_iface_tmp, &hardif_list, list) { list_del_rcu(&hard_iface->list); hardif_remove_interface(hard_iface); } - spin_unlock(&hardif_list_lock); rtnl_unlock(); } @@ -524,9 +520,7 @@ static int hard_if_event(struct notifier_block *this, hardif_deactivate_interface(hard_iface); break; case NETDEV_UNREGISTER: - spin_lock(&hardif_list_lock); list_del_rcu(&hard_iface->list); - spin_unlock(&hardif_list_lock); hardif_remove_interface(hard_iface); break; -- 1.7.4.4

Sven Eckelmann

8:54 p.m.

New subject: [B.A.T.M.A.N.] [PATCHv3 2/3] batman-adv: Remove unnecessary hardif_list_lock

hardif_list_lock is unneccessary because we already ensure that no multiple admin operations can take place through rtnl_lock. hardif_list_lock only adds additional overhead and complexity. Critical functions now check whether they are called with rtnl_lock using ASSERT_RTNL. Signed-off-by: Sven Eckelmann <sven(a)narfation.org> --- Rebased patch on top of "batman-adv: Make bat_priv->primary_if an rcu protected pointer" hard-interface.c | 21 ++++++--------------- 1 files changed, 6 insertions(+), 15 deletions(-) diff --git a/hard-interface.c b/hard-interface.c index 6636d2f..7e2f772 100644 --- a/hard-interface.c +++ b/hard-interface.c @@ -31,9 +31,6 @@ #include <linux/if_arp.h> -/* protect update critical side of hardif_list - but not the content */ -static DEFINE_SPINLOCK(hardif_list_lock); - static int batman_skb_recv(struct sk_buff *skb, struct net_device *dev, @@ -136,7 +133,7 @@ static void primary_if_select(struct bat_priv *bat_priv, struct hard_iface *curr_hard_iface; struct batman_packet *batman_packet; - spin_lock_bh(&hardif_list_lock); + ASSERT_RTNL(); if (new_hard_iface && !atomic_inc_not_zero(&new_hard_iface->refcount)) new_hard_iface = NULL; @@ -148,7 +145,7 @@ static void primary_if_select(struct bat_priv *bat_priv, hardif_free_ref(curr_hard_iface); if (!new_hard_iface) - goto out; + return; batman_packet = (struct batman_packet *)(new_hard_iface->packet_buff); batman_packet->flags = PRIMARIES_FIRST_HOP; @@ -161,9 +158,6 @@ static void primary_if_select(struct bat_priv *bat_priv, * our new primary interface */ atomic_set(&bat_priv->hna_local_changed, 1); - -out: - spin_unlock_bh(&hardif_list_lock); } static bool hardif_is_iface_up(struct hard_iface *hard_iface) @@ -456,6 +450,8 @@ static struct hard_iface *hardif_add_interface(struct net_device *net_dev) struct hard_iface *hard_iface; int ret; + ASSERT_RTNL(); + ret = is_valid_iface(net_dev); if (ret != 1) goto out; @@ -482,10 +478,7 @@ static struct hard_iface *hardif_add_interface(struct net_device *net_dev) atomic_set(&hard_iface->refcount, 2); check_known_mac_addr(hard_iface->net_dev); - - spin_lock(&hardif_list_lock); list_add_tail_rcu(&hard_iface->list, &hardif_list); - spin_unlock(&hardif_list_lock); return hard_iface; @@ -499,6 +492,8 @@ out: static void hardif_remove_interface(struct hard_iface *hard_iface) { + ASSERT_RTNL(); + /* first deactivate interface */ if (hard_iface->if_status != IF_NOT_IN_USE) hardif_disable_interface(hard_iface); @@ -516,13 +511,11 @@ void hardif_remove_interfaces(void) struct hard_iface *hard_iface, *hard_iface_tmp; rtnl_lock(); - spin_lock(&hardif_list_lock); list_for_each_entry_safe(hard_iface, hard_iface_tmp, &hardif_list, list) { list_del_rcu(&hard_iface->list); hardif_remove_interface(hard_iface); } - spin_unlock(&hardif_list_lock); rtnl_unlock(); } @@ -549,9 +542,7 @@ static int hard_if_event(struct notifier_block *this, hardif_deactivate_interface(hard_iface); break; case NETDEV_UNREGISTER: - spin_lock(&hardif_list_lock); list_del_rcu(&hard_iface->list); - spin_unlock(&hardif_list_lock); hardif_remove_interface(hard_iface); break; -- 1.7.4.4

Linus Lüssing

2 May 2 May

5:16 p.m.

New subject: [B.A.T.M.A.N.] [PATCHv3 2/3] batman-adv: Remove unnecessary hardif_list_lock

Looks quite good and seems to make sense to avoid the double-locking. Tested it here and together with patch 1/3 fixes the initial issue, too. Just one minor thing: ---- [58518.225154] batman_adv: bat0: The MTU of interface eth1 is too small (1500) to handle the transport of batman-adv packets. Packets going over this interface will be fragmented on layer2 which could impact the performance. Setting the MTU to 1527 would solve the problem. [58518.278111] RTNL: assertion failed at /home/mesh-node/incoming/batman-adv-git-builddir/hard-interface.c (136) [58518.293167] Pid: 6366, comm: bash Not tainted 2.6.38+ #4 [58518.299142] Call Trace: [58518.300953] [<f8cb91f3>] primary_if_select+0x2f/0x9f [batman_adv] [58518.447844] [<f8cb8e9c>] ? rcu_read_unlock+0x1c/0x1e [batman_adv] [58518.452572] [<f8cb9146>] ? primary_if_get_selected+0x6a/0x71 [batman_adv] [58518.455759] [<f8cb92c6>] hardif_activate_interface+0x63/0x91 [batman_adv] [58518.459168] [<f8cb98e4>] hardif_enable_interface+0x1cc/0x21e [batman_adv] [58518.462481] [<f8cb7bca>] store_mesh_iface+0xed/0x113 [batman_adv] [58518.465564] [<f8cb7add>] ? store_aggregated_ogms+0x28/0x28 [batman_adv] [58518.468714] [<c11e1121>] kobj_attr_store+0x1a/0x22 [58518.472277] [<c11481dc>] sysfs_write_file+0xbc/0xe7 [58518.613128] [<c1148120>] ? sysfs_open_file+0x205/0x205 [58518.616400] [<c10ff7a7>] vfs_write+0x84/0xde [58518.660437] [<c10ff89a>] sys_write+0x3b/0x60 [58518.667248] [<c151fd9d>] syscall_call+0x7/0xb [58518.672117] batman_adv: bat0: Interface activated: eth1 ---- So maybe it makes sense to add the assertion in primary_if_select() in patch 3/3 instead? Hmm, and I'm wondering if it might make sense to also document it with a little comment above the struct list_head hardif_list;, like: /* List additions and deletions have to be rtnl_lock()'ed, * list traversals just rcu-locked */ (or is that overdoing it?) On Sat, Apr 30, 2011 at 10:54:14PM +0200, Sven Eckelmann wrote:

...

hardif_list_lock is unneccessary because we already ensure that no multiple admin operations can take place through rtnl_lock. hardif_list_lock only adds additional overhead and complexity. Critical functions now check whether they are called with rtnl_lock using ASSERT_RTNL. Signed-off-by: Sven Eckelmann <sven(a)narfation.org> --- Rebased patch on top of "batman-adv: Make bat_priv->primary_if an rcu protected pointer" hard-interface.c | 21 ++++++--------------- 1 files changed, 6 insertions(+), 15 deletions(-) diff --git a/hard-interface.c b/hard-interface.c index 6636d2f..7e2f772 100644 --- a/hard-interface.c +++ b/hard-interface.c @@ -31,9 +31,6 @@ #include <linux/if_arp.h> -/* protect update critical side of hardif_list - but not the content */ -static DEFINE_SPINLOCK(hardif_list_lock); - static int batman_skb_recv(struct sk_buff *skb, struct net_device *dev, @@ -136,7 +133,7 @@ static void primary_if_select(struct bat_priv *bat_priv, struct hard_iface *curr_hard_iface; struct batman_packet *batman_packet; - spin_lock_bh(&hardif_list_lock); + ASSERT_RTNL(); if (new_hard_iface && !atomic_inc_not_zero(&new_hard_iface->refcount)) new_hard_iface = NULL; @@ -148,7 +145,7 @@ static void primary_if_select(struct bat_priv *bat_priv, hardif_free_ref(curr_hard_iface); if (!new_hard_iface) - goto out; + return; batman_packet = (struct batman_packet *)(new_hard_iface->packet_buff); batman_packet->flags = PRIMARIES_FIRST_HOP; @@ -161,9 +158,6 @@ static void primary_if_select(struct bat_priv *bat_priv, * our new primary interface */ atomic_set(&bat_priv->hna_local_changed, 1); - -out: - spin_unlock_bh(&hardif_list_lock); } static bool hardif_is_iface_up(struct hard_iface *hard_iface) @@ -456,6 +450,8 @@ static struct hard_iface *hardif_add_interface(struct net_device *net_dev) struct hard_iface *hard_iface; int ret; + ASSERT_RTNL(); + ret = is_valid_iface(net_dev); if (ret != 1) goto out; @@ -482,10 +478,7 @@ static struct hard_iface *hardif_add_interface(struct net_device *net_dev) atomic_set(&hard_iface->refcount, 2); check_known_mac_addr(hard_iface->net_dev); - - spin_lock(&hardif_list_lock); list_add_tail_rcu(&hard_iface->list, &hardif_list); - spin_unlock(&hardif_list_lock); return hard_iface; @@ -499,6 +492,8 @@ out: static void hardif_remove_interface(struct hard_iface *hard_iface) { + ASSERT_RTNL(); + /* first deactivate interface */ if (hard_iface->if_status != IF_NOT_IN_USE) hardif_disable_interface(hard_iface); @@ -516,13 +511,11 @@ void hardif_remove_interfaces(void) struct hard_iface *hard_iface, *hard_iface_tmp; rtnl_lock(); - spin_lock(&hardif_list_lock); list_for_each_entry_safe(hard_iface, hard_iface_tmp, &hardif_list, list) { list_del_rcu(&hard_iface->list); hardif_remove_interface(hard_iface); } - spin_unlock(&hardif_list_lock); rtnl_unlock(); } @@ -549,9 +542,7 @@ static int hard_if_event(struct notifier_block *this, hardif_deactivate_interface(hard_iface); break; case NETDEV_UNREGISTER: - spin_lock(&hardif_list_lock); list_del_rcu(&hard_iface->list); - spin_unlock(&hardif_list_lock); hardif_remove_interface(hard_iface); break; -- 1.7.4.4

Sven Eckelmann

7:15 p.m.

New subject: [B.A.T.M.A.N.] [PATCHv3 2/3] batman-adv: Remove unnecessary hardif_list_lock

Linus Lüssing wrote:

...

No, the problem is that I changed the patch after I noticed that it wasn't made for the current master - and only tested after all patches were applied. I would like to add rtnl_lock/rtnl_unlock around primary_if_get_selected() instead.

...

Hmm, and I'm wondering if it might make sense to also document it with a little comment above the struct list_head hardif_list;, like: /* List additions and deletions have to be rtnl_lock()'ed, * list traversals just rcu-locked */ (or is that overdoing it?)

Ok, will steal this comment from your mail - ok, change it slightly to present it as my own work. Kind regards, Sven

Sven Eckelmann

30 Apr 30 Apr

4:57 p.m.

New subject: [B.A.T.M.A.N.] [PATCHv2 3/3] batman-adv: Avoid deadlock between rtnl_lock and s_active

The hard_if_event is called by the notifier with rtnl_lock and tries to remove sysfs entries when a NETDEV_UNREGISTER event is received. This will automatically take the s_active lock. The s_active lock is also used when a new interface is added to a meshif through sysfs. In that situation we cannot wait for the rntl_lock before creating the actual batman-adv interface to prevent a deadlock. It is still possible to try to get the rtnl_lock and immediately abort the current operation when the trylock call failed. Signed-off-by: Sven Eckelmann <sven(a)narfation.org> --- Remove '{' '}' after 'if (hard_iface->if_status != IF_NOT_IN_USE)' bat_sysfs.c | 16 +++++++++------- soft-interface.c | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/bat_sysfs.c b/bat_sysfs.c index e449bf6..497a070 100644 --- a/bat_sysfs.c +++ b/bat_sysfs.c @@ -488,22 +488,24 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, (strncmp(hard_iface->soft_iface->name, buff, IFNAMSIZ) == 0)) goto out; + if (!rtnl_trylock()) { + ret = -ERESTARTSYS; + goto out; + } + if (status_tmp == IF_NOT_IN_USE) { - rtnl_lock(); hardif_disable_interface(hard_iface); - rtnl_unlock(); - goto out; + goto unlock; } /* if the interface already is in use */ - if (hard_iface->if_status != IF_NOT_IN_USE) { - rtnl_lock(); + if (hard_iface->if_status != IF_NOT_IN_USE) hardif_disable_interface(hard_iface); - rtnl_unlock(); - } ret = hardif_enable_interface(hard_iface, buff); +unlock: + rtnl_unlock(); out: hardif_free_ref(hard_iface); return ret; diff --git a/soft-interface.c b/soft-interface.c index f4d80ad..bf53aa3 100644 --- a/soft-interface.c +++ b/soft-interface.c @@ -613,7 +613,7 @@ struct net_device *softif_create(char *name) goto out; } - ret = register_netdev(soft_iface); + ret = register_netdevice(soft_iface); if (ret < 0) { pr_err("Unable to register the batman interface '%s': %i\n", name, ret); -- 1.7.4.4

Linus Lüssing

2 May 2 May

5:27 p.m.

New subject: [B.A.T.M.A.N.] [PATCHv2 3/3] batman-adv: Avoid deadlock between rtnl_lock and s_active

Looks good, seems to make sense and tried it. Works so far with no call traces or other uggly stuff. However I'm still trying to reproduce the initial issue of ticket #145 to confirm that this patch fixes it (used a different kernel and batman-adv version back then, of course... so somehow the 'always' stated there does not seem to be valid anymore :) ). PS: Ok, just sometimes I'm still getting a locking dependancy call trace, but that one involves two different locks ((&(&bat_priv->hna_ghash_lock)->rlock){+.-...}, at: [<f83a3dd6>] hna_global_del_orig+0x22/0x90 [batman_adv] (&(&hash->list_locks[i])->rlock){+.-...}, at: [<f839fe4b>] _purge_orig+0x45/0x1be [batman_adv]) and therefore seems to be unrelated - I'll open a new ticket for that. On Sat, Apr 30, 2011 at 06:57:00PM +0200, Sven Eckelmann wrote:

...

Linus Lüssing

6:05 p.m.

New subject: [B.A.T.M.A.N.] [PATCHv2 3/3] batman-adv: Avoid deadlock between rtnl_lock and s_active

Ok, could reproduce and verify it now (latest master branch _without_ patches 1/3 and 2/3, with that version I can still reproduce that issue reliably). Putting only patch 3/3 on top the master branch then fixes that locking dependancy. Thanks, Sven! Cheers, Linus On Mon, May 02, 2011 at 07:27:28PM +0200, Linus Lüssing wrote:

...

4415

days inactive

4417

days old

b.a.t.m.a.n@lists.open-mesh.org

Manage subscription

7 comments

2 participants

tags (0)

participants (2)

Linus Lüssing
Sven Eckelmann