[maint] batman-adv: wait for rtnl in batadv_store_mesh_iface instead of failing if it is taken

Message ID 179dd3afbd3ecdcd2144bd3016e1b01eaf0d5263.1369754967.git.mschiffer@universe-factory.net (mailing list archive)
State Accepted, archived
Commit 96cd7725540f4dccdd6fbb4fde59243e1cc1ad80
Headers

Commit Message

Matthias Schiffer May 28, 2013, 3:32 p.m. UTC
  The rtnl_lock in batadv_store_mesh_iface has been converted to a rtnl_trylock
some time ago to avoid a possible deadlock between rtnl and s_active on removal
of the sysfs nodes.

The behaviour introduced by that was quite confusing as it could lead to the
sysfs store to fail, making batman-adv setup scripts unreliable. As recently the
sysfs removal was postponed to a worker not running with the rtnl taken, the
deadlock can't occur any more and it is safe to change the trylock back to a
lock to make the sysfs store reliable again.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 sysfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
  

Comments

Simon Wunderlich May 28, 2013, 3:42 p.m. UTC | #1
On Tue, May 28, 2013 at 05:32:32PM +0200, Matthias Schiffer wrote:
> The rtnl_lock in batadv_store_mesh_iface has been converted to a rtnl_trylock
> some time ago to avoid a possible deadlock between rtnl and s_active on removal
> of the sysfs nodes.
> 
> The behaviour introduced by that was quite confusing as it could lead to the
> sysfs store to fail, making batman-adv setup scripts unreliable. As recently the
> sysfs removal was postponed to a worker not running with the rtnl taken, the
> deadlock can't occur any more and it is safe to change the trylock back to a
> lock to make the sysfs store reliable again.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>

Reviewed-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>

Thanks for posting,
	Simon

> ---
>  sysfs.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/sysfs.c b/sysfs.c
> index 15a22ef..929e304 100644
> --- a/sysfs.c
> +++ b/sysfs.c
> @@ -582,10 +582,7 @@ static ssize_t batadv_store_mesh_iface(struct kobject *kobj,
>  	    (strncmp(hard_iface->soft_iface->name, buff, IFNAMSIZ) == 0))
>  		goto out;
>  
> -	if (!rtnl_trylock()) {
> -		ret = -ERESTARTSYS;
> -		goto out;
> -	}
> +	rtnl_lock();
>  
>  	if (status_tmp == BATADV_IF_NOT_IN_USE) {
>  		batadv_hardif_disable_interface(hard_iface,
> -- 
> 1.8.3
>
  
Marek Lindner June 4, 2013, 9:14 a.m. UTC | #2
On Tuesday, May 28, 2013 23:42:48 Simon Wunderlich wrote:
>   On Tue, May 28, 2013 at 05:32:32PM +0200, Matthias Schiffer wrote:
> > The rtnl_lock in batadv_store_mesh_iface has been converted to a
> > rtnl_trylock some time ago to avoid a possible deadlock between rtnl and
> > s_active on removal of the sysfs nodes.
> >
> > The behaviour introduced by that was quite confusing as it could lead to
> > the sysfs store to fail, making batman-adv setup scripts unreliable. As
> > recently the sysfs removal was postponed to a worker not running with
> > the rtnl taken, the deadlock can't occur any more and it is safe to
> > change the trylock back to a lock to make the sysfs store reliable
> > again.
> >
> > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> 
> Reviewed-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>

Applied in revision 96cd772.

Thanks,
Marek
  

Patch

diff --git a/sysfs.c b/sysfs.c
index 15a22ef..929e304 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -582,10 +582,7 @@  static ssize_t batadv_store_mesh_iface(struct kobject *kobj,
 	    (strncmp(hard_iface->soft_iface->name, buff, IFNAMSIZ) == 0))
 		goto out;
 
-	if (!rtnl_trylock()) {
-		ret = -ERESTARTSYS;
-		goto out;
-	}
+	rtnl_lock();
 
 	if (status_tmp == BATADV_IF_NOT_IN_USE) {
 		batadv_hardif_disable_interface(hard_iface,