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

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

[Sven Eckelmann]

From: Linus Lüssing <linus.luessing@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@web.de>
[sven@narfation.org: Keep locking around list_for_each_entry_safe]
Signed-off-by: Sven Eckelmann <sven@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

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

[Sven Eckelmann]

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@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

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

[Sven Eckelmann]

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@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

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

[Linus Lüssing]

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@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
> 

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

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 2435 bytes --]

Linus Lüssing wrote:
> 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?

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Sven Eckelmann]

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@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

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

[Linus Lüssing]

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:
> 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@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
> 

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

[Linus Lüssing]

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:
> 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:
> > 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@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
> > 
>