diff mbox

[v2,1/2] batman-adv: Fix rcu_barrier() miss due to double call_rcu() in TT code

Message ID 1364952313-3163-1-git-send-email-linus.luessing@web.de (mailing list archive)
State Accepted, archived
Headers

Commit Message

Linus Lüssing April 3, 2013, 1:25 a.m. UTC 
  rcu_barrier() only waits for the currently scheduled rcu functions
to finish - it won't wait for any function scheduled via another
call_rcu() within an rcu scheduled function.

Unfortunately our batadv_tt_orig_list_entry_free_ref() does just that,
via a batadv_orig_node_free_ref() call, leading to our rcu_barrier()
call potentially missing such a batadv_orig_node_free_ref().

This patch fixes this issue by calling the batadv_orig_node_free_rcu()
directly from the rcu callback, removing the unnecessary, additional
call_rcu() layer here.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
* v2: Added a code comment as discussed on IRC:
      To avoid forgetting about it, to avoid accidentally changing things
      back in the future.

 originator.c        |    2 +-
 originator.h        |    1 +
 translation-table.c |    8 +++++++-
 3 files changed, 9 insertions(+), 2 deletions(-)

Comments

Antonio Quartulli April 3, 2013, 8:11 a.m. UTC | #1 
On Wed, Apr 03, 2013 at 03:25:13AM +0200, Linus Lüssing wrote:
> rcu_barrier() only waits for the currently scheduled rcu functions
> to finish - it won't wait for any function scheduled via another
> call_rcu() within an rcu scheduled function.
> 
> Unfortunately our batadv_tt_orig_list_entry_free_ref() does just that,
> via a batadv_orig_node_free_ref() call, leading to our rcu_barrier()
> call potentially missing such a batadv_orig_node_free_ref().
> 
> This patch fixes this issue by calling the batadv_orig_node_free_rcu()
> directly from the rcu callback, removing the unnecessary, additional
> call_rcu() layer here.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Acked-by: Antonio Quartulli <ordex@autistici.org>

Thanks a lot
Marek Lindner April 15, 2013, 1:48 p.m. UTC | #2 
On Wednesday, April 03, 2013 16:11:45 Antonio Quartulli wrote:
> > This patch fixes this issue by calling the batadv_orig_node_free_rcu()
> > directly from the rcu callback, removing the unnecessary, additional
> > call_rcu() layer here.
> >
> > Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> 
> Acked-by: Antonio Quartulli <ordex@autistici.org>

Applied in revision c8bda21.

Cheers,
Marek


PS: I did some style adjustments to the patch for the sake of clarity.
diff mbox

Patch

diff --git a/originator.c b/originator.c
index 2f34525..1f01e93 100644
--- a/originator.c
+++ b/originator.c
@@ -117,7 +117,7 @@  out:
 	return neigh_node;
 }
 
-static void batadv_orig_node_free_rcu(struct rcu_head *rcu)
+void batadv_orig_node_free_rcu(struct rcu_head *rcu)
 {
 	struct hlist_node *node_tmp;
 	struct batadv_neigh_node *neigh_node, *tmp_neigh_node;
diff --git a/originator.h b/originator.h
index 7df48fa..4f9f88b 100644
--- a/originator.h
+++ b/originator.h
@@ -25,6 +25,7 @@ 
 int batadv_originator_init(struct batadv_priv *bat_priv);
 void batadv_originator_free(struct batadv_priv *bat_priv);
 void batadv_purge_orig_ref(struct batadv_priv *bat_priv);
+void batadv_orig_node_free_rcu(struct rcu_head *rcu);
 void batadv_orig_node_free_ref(struct batadv_orig_node *orig_node);
 struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv,
 					      const uint8_t *addr);
diff --git a/translation-table.c b/translation-table.c
index 9322320..4fe07cf 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -144,7 +144,13 @@  static void batadv_tt_orig_list_entry_free_rcu(struct rcu_head *rcu)
 	struct batadv_tt_orig_list_entry *orig_entry;
 
 	orig_entry = container_of(rcu, struct batadv_tt_orig_list_entry, rcu);
-	batadv_orig_node_free_ref(orig_entry->orig_node);
+
+	/* We are in an rcu callback here, therefore we cannot use
+	 * batadv_orig_node_free_ref() and its call_rcu():
+	 * An rcu_barrier() wouldn't wait for that to finish */
+	if (atomic_dec_and_test(&orig_entry->orig_node->refcount))
+		batadv_orig_node_free_rcu(&orig_entry->orig_node->rcu);
+
 	kfree(orig_entry);
 }