Re: [B.A.T.M.A.N.] [PATCH] batman-adv: postpone sysfs removal when unregistering

2013-01-10 Thread Simon Wunderlich
Hey Antonio,


On Thu, Jan 10, 2013 at 04:34:47AM +0200, Antonio Quartulli wrote:
 Hello Simon,
 
 thanks a lot for taking care of this and sorry for my very late reply..

no problem, thanks for your review!

 
 
 Il 31.12.2012 01:40 Simon Wunderlich ha scritto:
 diff --git a/hard-interface.c b/hard-interface.c
 index f1d37cd..eb3a12d 100644
 --- a/hard-interface.c
 +++ b/hard-interface.c
 @@ -506,6 +506,17 @@ out:
  return NULL;
  }
 
 +static void batadv_hardif_remove_interface_finish(struct
 work_struct *work)
 +{
 +struct batadv_hard_iface *hard_iface;
 +
 +hard_iface = container_of(work, struct batadv_hard_iface,
 +  cleanup_work);
 +
 +batadv_sysfs_del_hardif(hard_iface-hardif_obj);
 +batadv_hardif_free_ref(hard_iface);
 +}
 +
  static void batadv_hardif_remove_interface(struct batadv_hard_iface
 *hard_iface)
  {
  ASSERT_RTNL();
 @@ -518,8 +529,9 @@ static void batadv_hardif_remove_interface(struct
 batadv_hard_iface *hard_iface)
  return;
 
  hard_iface-if_status = BATADV_IF_TO_BE_REMOVED;
 -batadv_sysfs_del_hardif(hard_iface-hardif_obj);
 -batadv_hardif_free_ref(hard_iface);
 +INIT_WORK(hard_iface-cleanup_work,
 +  batadv_hardif_remove_interface_finish);
 +queue_work(batadv_event_workqueue, hard_iface-cleanup_work);
  }
 
 
 why do we need to postpone the invocation of
 batadv_hardif_remove_interface_finish() ? Is it also creating a
 possible
 deadlock? As far as I understood only rtnl_lock() would create the
 problem, but it
 is not invoked in batadv_hardif_remove_interface_finish(). It seems
 I am missing
 something?

batadv_hardif_remove_interface() is called by batadv_hard_if_event(),
which is protected rtnl_lock(). You can find an ASSERT_RTNL() there too.
As batadv_hardif_remove_interface() then removes sysfs, this could
be problematic.

 
 Other than this, remember that the INIT_WORK macro does not need to
 be invoked
 each and every time you want to enqueue the work. It should be
 invoked once only
 (A patch fixing this issue for all the delayed works we have in
 the code has
 been recently applied). However it is not a real issue, but we want
 to keep the
 code consistent :)
 

Well, the work item is actually only initialized once, before taking
the interface down. Then the struct is destroyed, so it can't be called
again. But you are right, for consistency I will move the INIT_WORK
macros to positions where the structs are initialized.

  int batadv_softif_is_valid(const struct net_device *net_dev)
 diff --git a/types.h b/types.h
 index d8061ac..a9800ee 100644
 --- a/types.h
 +++ b/types.h
 @@ -63,6 +63,7 @@ struct batadv_hard_iface {
  struct net_device *soft_iface;
  struct rcu_head rcu;
  struct batadv_hard_iface_bat_iv bat_iv;
 +struct work_struct cleanup_work;
  };
 
 
 kernel doc :)
 

The Mareks kernel doc patch wasn't applied at this time, so I didn't
bother documenting my single new variable, but will do that in the
next revision. :)

Thanks,
Simon


signature.asc
Description: Digital signature


Re: [B.A.T.M.A.N.] [PATCH] batman-adv: postpone sysfs removal when unregistering

2013-01-09 Thread Antonio Quartulli

Hello Simon,

thanks a lot for taking care of this and sorry for my very late reply..


Il 31.12.2012 01:40 Simon Wunderlich ha scritto:

diff --git a/hard-interface.c b/hard-interface.c
index f1d37cd..eb3a12d 100644
--- a/hard-interface.c
+++ b/hard-interface.c
@@ -506,6 +506,17 @@ out:
return NULL;
 }

+static void batadv_hardif_remove_interface_finish(struct work_struct 
*work)

+{
+   struct batadv_hard_iface *hard_iface;
+
+   hard_iface = container_of(work, struct batadv_hard_iface,
+ cleanup_work);
+
+   batadv_sysfs_del_hardif(hard_iface-hardif_obj);
+   batadv_hardif_free_ref(hard_iface);
+}
+
 static void batadv_hardif_remove_interface(struct batadv_hard_iface
*hard_iface)
 {
ASSERT_RTNL();
@@ -518,8 +529,9 @@ static void batadv_hardif_remove_interface(struct
batadv_hard_iface *hard_iface)
return;

hard_iface-if_status = BATADV_IF_TO_BE_REMOVED;
-   batadv_sysfs_del_hardif(hard_iface-hardif_obj);
-   batadv_hardif_free_ref(hard_iface);
+   INIT_WORK(hard_iface-cleanup_work,
+ batadv_hardif_remove_interface_finish);
+   queue_work(batadv_event_workqueue, hard_iface-cleanup_work);
 }



why do we need to postpone the invocation of
batadv_hardif_remove_interface_finish() ? Is it also creating a 
possible
deadlock? As far as I understood only rtnl_lock() would create the 
problem, but it
is not invoked in batadv_hardif_remove_interface_finish(). It seems I 
am missing

something?

Other than this, remember that the INIT_WORK macro does not need to be 
invoked
each and every time you want to enqueue the work. It should be invoked 
once only
(A patch fixing this issue for all the delayed works we have in the 
code has
been recently applied). However it is not a real issue, but we want to 
keep the

code consistent :)


+static void batadv_softif_destroy_finish(struct work_struct *work)
+{
+   struct batadv_priv *bat_priv;
+   struct net_device *soft_iface;
+
+   bat_priv = container_of(work, struct batadv_priv,
+   cleanup_work);
+   soft_iface = bat_priv-soft_iface;
+
+   batadv_debugfs_del_meshif(soft_iface);
+   batadv_sysfs_del_meshif(soft_iface);
+
+   rtnl_lock();
+   unregister_netdevice(soft_iface);
+   rtnl_unlock();
+}
+
 void batadv_softif_destroy(struct net_device *soft_iface)
 {
-   batadv_debugfs_del_meshif(soft_iface);
-   batadv_sysfs_del_meshif(soft_iface);
+   struct batadv_priv *bat_priv = netdev_priv(soft_iface);
+
batadv_mesh_free(soft_iface);
-   unregister_netdevice(soft_iface);
+
+   INIT_WORK(bat_priv-cleanup_work, batadv_softif_destroy_finish);
+   queue_work(batadv_event_workqueue, bat_priv-cleanup_work);
 }



Same as above (Assuming we really need two scheduled tasks).


 int batadv_softif_is_valid(const struct net_device *net_dev)
diff --git a/types.h b/types.h
index d8061ac..a9800ee 100644
--- a/types.h
+++ b/types.h
@@ -63,6 +63,7 @@ struct batadv_hard_iface {
struct net_device *soft_iface;
struct rcu_head rcu;
struct batadv_hard_iface_bat_iv bat_iv;
+   struct work_struct cleanup_work;
 };



kernel doc :)


 /**
@@ -266,6 +267,7 @@ struct batadv_priv_dat {

 struct batadv_priv {
atomic_t mesh_state;
+   struct net_device *soft_iface;
struct net_device_stats stats;
uint64_t __percpu *bat_counters; /* Per cpu counters */
atomic_t aggregated_ogms;   /* boolean */
@@ -302,6 +304,7 @@ struct batadv_priv {
spinlock_t forw_bat_list_lock; /* protects forw_bat_list */
spinlock_t forw_bcast_list_lock; /* protects forw_bcast_list */
struct delayed_work orig_work;
+   struct work_struct cleanup_work;
 	struct batadv_hard_iface __rcu *primary_if;  /* rcu protected 
pointer */

struct batadv_algo_ops *bat_algo_ops;
 #ifdef CONFIG_BATMAN_ADV_BLA


here too :)


Cheers,

--
Antonio Quartulli


[B.A.T.M.A.N.] [PATCH] batman-adv: postpone sysfs removal when unregistering

2012-12-30 Thread Simon Wunderlich
When processing the unregister notify for a hard interface, removing
the sysfs files may lead to a circular deadlock (rtnl mutex -
s_active).

To overcome this problem, postpone the sysfs removal in a worker.

Reported-by: Sasha Levin sasha.le...@oracle.com
Reported-by: Sven Eckelmann s...@narfation.org
Signed-off-by: Simon Wunderlich s...@hrz.tu-chemnitz.de
---
Changes from RFCv1:
 * use work_struct properly, instead of delayed_work
 * postpone for softifs as well as for hardifs

Postponing the sysfs removal for the hardif unregister is easier than
other alternatives involving deferring. This should bring us to the
same level to the bridge code, which also messes with sysfs in the
notifier processing function, and uses rtnl_trylock when called
from within sysfs.

As far as I could understand the net/core code, only the unregister
case is the critical one, so the original bug should hopefully be
fixed.
---
 hard-interface.c |   16 ++--
 soft-interface.c |   26 +++---
 types.h  |3 +++
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/hard-interface.c b/hard-interface.c
index f1d37cd..eb3a12d 100644
--- a/hard-interface.c
+++ b/hard-interface.c
@@ -506,6 +506,17 @@ out:
return NULL;
 }
 
+static void batadv_hardif_remove_interface_finish(struct work_struct *work)
+{
+   struct batadv_hard_iface *hard_iface;
+
+   hard_iface = container_of(work, struct batadv_hard_iface,
+ cleanup_work);
+
+   batadv_sysfs_del_hardif(hard_iface-hardif_obj);
+   batadv_hardif_free_ref(hard_iface);
+}
+
 static void batadv_hardif_remove_interface(struct batadv_hard_iface 
*hard_iface)
 {
ASSERT_RTNL();
@@ -518,8 +529,9 @@ static void batadv_hardif_remove_interface(struct 
batadv_hard_iface *hard_iface)
return;
 
hard_iface-if_status = BATADV_IF_TO_BE_REMOVED;
-   batadv_sysfs_del_hardif(hard_iface-hardif_obj);
-   batadv_hardif_free_ref(hard_iface);
+   INIT_WORK(hard_iface-cleanup_work,
+ batadv_hardif_remove_interface_finish);
+   queue_work(batadv_event_workqueue, hard_iface-cleanup_work);
 }
 
 void batadv_hardif_remove_interfaces(void)
diff --git a/soft-interface.c b/soft-interface.c
index a8eb963..b8a307b 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -464,6 +464,7 @@ struct net_device *batadv_softif_create(const char *name)
goto out;
 
bat_priv = netdev_priv(soft_iface);
+   bat_priv-soft_iface = soft_iface;
 
/* batadv_interface_stats() needs to be available as soon as
 * register_netdevice() has been called
@@ -550,12 +551,31 @@ out:
return NULL;
 }
 
+static void batadv_softif_destroy_finish(struct work_struct *work)
+{
+   struct batadv_priv *bat_priv;
+   struct net_device *soft_iface;
+
+   bat_priv = container_of(work, struct batadv_priv,
+   cleanup_work);
+   soft_iface = bat_priv-soft_iface;
+
+   batadv_debugfs_del_meshif(soft_iface);
+   batadv_sysfs_del_meshif(soft_iface);
+
+   rtnl_lock();
+   unregister_netdevice(soft_iface);
+   rtnl_unlock();
+}
+
 void batadv_softif_destroy(struct net_device *soft_iface)
 {
-   batadv_debugfs_del_meshif(soft_iface);
-   batadv_sysfs_del_meshif(soft_iface);
+   struct batadv_priv *bat_priv = netdev_priv(soft_iface);
+
batadv_mesh_free(soft_iface);
-   unregister_netdevice(soft_iface);
+
+   INIT_WORK(bat_priv-cleanup_work, batadv_softif_destroy_finish);
+   queue_work(batadv_event_workqueue, bat_priv-cleanup_work);
 }
 
 int batadv_softif_is_valid(const struct net_device *net_dev)
diff --git a/types.h b/types.h
index d8061ac..a9800ee 100644
--- a/types.h
+++ b/types.h
@@ -63,6 +63,7 @@ struct batadv_hard_iface {
struct net_device *soft_iface;
struct rcu_head rcu;
struct batadv_hard_iface_bat_iv bat_iv;
+   struct work_struct cleanup_work;
 };
 
 /**
@@ -266,6 +267,7 @@ struct batadv_priv_dat {
 
 struct batadv_priv {
atomic_t mesh_state;
+   struct net_device *soft_iface;
struct net_device_stats stats;
uint64_t __percpu *bat_counters; /* Per cpu counters */
atomic_t aggregated_ogms;   /* boolean */
@@ -302,6 +304,7 @@ struct batadv_priv {
spinlock_t forw_bat_list_lock; /* protects forw_bat_list */
spinlock_t forw_bcast_list_lock; /* protects forw_bcast_list */
struct delayed_work orig_work;
+   struct work_struct cleanup_work;
struct batadv_hard_iface __rcu *primary_if;  /* rcu protected pointer */
struct batadv_algo_ops *bat_algo_ops;
 #ifdef CONFIG_BATMAN_ADV_BLA
-- 
1.7.10.4