[B.A.T.M.A.N.] Reordered rcu refcounting

[B.A.T.M.A.N.] Reordered rcu refcounting

[Sven Eckelmann]

Hi,

I told Linus and Marek that the implementation of some datastructures using
reference counting and RCUs must be redesigned. The problem is that call_rcu
should only be used when the reference count is zero because multiple call_rcu
cannot be initiated at the same time. The next problem is that we may try to
access an element with the refcount 0 when we try to traverse a rcu protected
list. This problem cannot be solved by kref - thus we have to use atomic_t
again to _not_ increase the reference count when it was already zero.

Marek asked me what we must change - I try to summarize those things in the
following patches. They aren't tested and should only be seen as suggestions
what must be done to finish the "remove orig_hash lock"-patches.

If you try these patches then please keep in mind that they will sell your soul
to the next confectioner (for some cookies or a pie).

More information can be found inside Documentation/RCU/rcuref.txt

Btw, maybe I've found a bug inside the find_router with enabled bonding (see
"BUG" in patch 4).

Best regards,
	Sven

[B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node

[Sven Eckelmann]

<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 batman-adv/gateway_client.c |   28 +++++++++++++---------------
 batman-adv/types.h          |    2 +-
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 429a013..8ce3a63 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -28,20 +28,18 @@
 #include <linux/udp.h>
 #include <linux/if_vlan.h>
 
-static void gw_node_free_ref(struct kref *refcount)
-{
-	struct gw_node *gw_node;
-
-	gw_node = container_of(refcount, struct gw_node, refcount);
-	kfree(gw_node);
-}
-
 static void gw_node_free_rcu(struct rcu_head *rcu)
 {
 	struct gw_node *gw_node;
 
 	gw_node = container_of(rcu, struct gw_node, rcu);
-	kref_put(&gw_node->refcount, gw_node_free_ref);
+	kfree(gw_node);
+}
+
+static void gw_node_free_ref(struct gw_node *gw_node)
+{
+	if (atomic_dec_and_test(&gw_node->refcount))
+		call_rcu(&gw_node->rcu, gw_node_free_rcu);
 }
 
 void *gw_get_selected(struct bat_priv *bat_priv)
@@ -61,7 +59,7 @@ void gw_deselect(struct bat_priv *bat_priv)
 	bat_priv->curr_gw = NULL;
 
 	if (gw_node)
-		kref_put(&gw_node->refcount, gw_node_free_ref);
+		gw_node_free_ref(gw_node);
 }
 
 static struct gw_node *gw_select(struct bat_priv *bat_priv,
@@ -69,8 +67,8 @@ static struct gw_node *gw_select(struct bat_priv *bat_priv,
 {
 	struct gw_node *curr_gw_node = bat_priv->curr_gw;
 
-	if (new_gw_node)
-		kref_get(&new_gw_node->refcount);
+	if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+		return NULL;
 
 	bat_priv->curr_gw = new_gw_node;
 	return curr_gw_node;
@@ -181,7 +179,7 @@ void gw_election(struct bat_priv *bat_priv)
 
 	/* the kfree() has to be outside of the rcu lock */
 	if (old_gw_node)
-		kref_put(&old_gw_node->refcount, gw_node_free_ref);
+		gw_node_free_ref(old_gw_node);
 }
 
 void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
@@ -242,7 +240,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
 	memset(gw_node, 0, sizeof(struct gw_node));
 	INIT_HLIST_NODE(&gw_node->list);
 	gw_node->orig_node = orig_node;
-	kref_init(&gw_node->refcount);
+	atomic_set(&gw_node->refcount, 1);
 
 	spin_lock_bh(&bat_priv->gw_list_lock);
 	hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -325,7 +323,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
 			gw_deselect(bat_priv);
 
 		hlist_del_rcu(&gw_node->list);
-		call_rcu(&gw_node->rcu, gw_node_free_rcu);
+		gw_node_free_ref(gw_node);
 	}
 
 
diff --git a/batman-adv/types.h b/batman-adv/types.h
index e4a0462..ca5f20a 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -100,7 +100,7 @@ struct gw_node {
 	struct hlist_node list;
 	struct orig_node *orig_node;
 	unsigned long deleted;
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
 };
 
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node

[Linus Lüssing]

From: Sven Eckelmann <sven@narfation.org>

Was:
---
<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---

So after some more discussions with Marek and Sven, it looks like we
have to use the rcu protected macros rcu_dereference() and
rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node.

Changes here also include moving the kref_get() from unicast_send_skb()
into gw_get_selected(). The orig_node could have been freed already at
the time the kref_get() was called in unicast_send_skb().

Some things that are still not that clear to me:

gw_election():
* can the if-block before gw_deselect() be ommited, we had a nullpointer
  check for curr_gw just a couple of lines before during the rcu-lock.

gw_deselet():
* is the refcount at this time always 1 for gw_node, can the null
pointer check + a rcu_dereference be ommited? (at least that's what
it looks like when comparing to the rcuref.txt example)

gw_get_selected():
* Probably the orig_node's refcounting has to be made atomic, too?

Cheers, Linus

Not-Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 gateway_client.c |  169 +++++++++++++++++++++++++++++++++---------------------
 main.c           |    1 +
 types.h          |    7 +-
 unicast.c        |    1 -
 4 files changed, 109 insertions(+), 69 deletions(-)

diff --git a/gateway_client.c b/gateway_client.c
index 429a013..96a67bc 100644
--- a/gateway_client.c
+++ b/gateway_client.c
@@ -28,40 +28,54 @@
 #include <linux/udp.h>
 #include <linux/if_vlan.h>
 
-static void gw_node_free_ref(struct kref *refcount)
+static void gw_node_free_rcu(struct rcu_head *rcu)
 {
 	struct gw_node *gw_node;
 
-	gw_node = container_of(refcount, struct gw_node, refcount);
+	gw_node = container_of(rcu, struct gw_node, rcu);
 	kfree(gw_node);
 }
 
-static void gw_node_free_rcu(struct rcu_head *rcu)
+static void gw_node_free_ref(struct gw_node *gw_node)
 {
-	struct gw_node *gw_node;
-
-	gw_node = container_of(rcu, struct gw_node, rcu);
-	kref_put(&gw_node->refcount, gw_node_free_ref);
+	if (atomic_dec_and_test(&gw_node->refcount))
+		call_rcu(&gw_node->rcu, gw_node_free_rcu);
 }
 
+/* increases the returned orig_node's refcount */
 void *gw_get_selected(struct bat_priv *bat_priv)
 {
-	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+	struct gw_node *curr_gateway_tmp;
+	struct orig_node *orig_node;
 
-	if (!curr_gateway_tmp)
+	rcu_read_lock();
+	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
+	if (!curr_gateway_tmp) {
+		rcu_read_unlock();
 		return NULL;
+	}
 
-	return curr_gateway_tmp->orig_node;
+	orig_node = rcu_dereference(curr_gateway_tmp->orig_node);
+	if (orig_node) {
+		kref_get(&orig_node->refcount);
+		rcu_read_unlock();
+		return NULL;
+	}
+
+	rcu_read_unlock();
+	return orig_node;
 }
 
 void gw_deselect(struct bat_priv *bat_priv)
 {
-	struct gw_node *gw_node = bat_priv->curr_gw;
+	struct gw_node *gw_node;
 
-	bat_priv->curr_gw = NULL;
+	spin_lock_bh(&bat_priv->curr_gw_lock);
+	gw_node = bat_priv->curr_gw;
+	rcu_assign_pointer(bat_priv->curr_gw, NULL);
+	spin_unlock_bh(&bat_priv->curr_gw_lock);
 
-	if (gw_node)
-		kref_put(&gw_node->refcount, gw_node_free_ref);
+	gw_node_free_ref(gw_node);
 }
 
 static struct gw_node *gw_select(struct bat_priv *bat_priv,
@@ -69,17 +83,21 @@ static struct gw_node *gw_select(struct bat_priv *bat_priv,
 {
 	struct gw_node *curr_gw_node = bat_priv->curr_gw;
 
-	if (new_gw_node)
-		kref_get(&new_gw_node->refcount);
+	if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+		return NULL;
+
+	spin_lock_bh(&bat_priv->curr_gw_lock);
+	rcu_assign_pointer(bat_priv->curr_gw, new_gw_node);
+	spin_unlock_bh(&bat_priv->curr_gw_lock);
 
-	bat_priv->curr_gw = new_gw_node;
 	return curr_gw_node;
 }
 
 void gw_election(struct bat_priv *bat_priv)
 {
 	struct hlist_node *node;
-	struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+	struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+	struct orig_node *orig_node;
 	uint8_t max_tq = 0;
 	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
 	int down, up;
@@ -93,25 +111,28 @@ void gw_election(struct bat_priv *bat_priv)
 	if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT)
 		return;
 
-	if (bat_priv->curr_gw)
-		return;
-
 	rcu_read_lock();
-	if (hlist_empty(&bat_priv->gw_list)) {
+	curr_gw = rcu_dereference(bat_priv->curr_gw);
+	if (curr_gw) {
 		rcu_read_unlock();
+		return;
+	}
 
-		if (bat_priv->curr_gw) {
+	if (hlist_empty(&bat_priv->gw_list)) {
+		if (curr_gw) {
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Removing selected gateway - "
 				"no gateway in range\n");
 			gw_deselect(bat_priv);
 		}
+		rcu_read_unlock();
 
 		return;
 	}
 
 	hlist_for_each_entry_rcu(gw_node, node, &bat_priv->gw_list, list) {
-		if (!gw_node->orig_node->router)
+		orig_node = rcu_dereference(gw_node->orig_node);
+		if (!orig_node->router)
 			continue;
 
 		if (gw_node->deleted)
@@ -119,18 +140,17 @@ void gw_election(struct bat_priv *bat_priv)
 
 		switch (atomic_read(&bat_priv->gw_sel_class)) {
 		case 1: /* fast connection */
-			gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags,
-					     &down, &up);
+			gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);
 
-			tmp_gw_factor = (gw_node->orig_node->router->tq_avg *
-					 gw_node->orig_node->router->tq_avg *
+			tmp_gw_factor = (orig_node->router->tq_avg *
+					 orig_node->router->tq_avg *
 					 down * 100 * 100) /
 					 (TQ_LOCAL_WINDOW_SIZE *
 					 TQ_LOCAL_WINDOW_SIZE * 64);
 
 			if ((tmp_gw_factor > max_gw_factor) ||
 			    ((tmp_gw_factor == max_gw_factor) &&
-			     (gw_node->orig_node->router->tq_avg > max_tq)))
+			     (orig_node->router->tq_avg > max_tq)))
 				curr_gw_tmp = gw_node;
 			break;
 
@@ -142,37 +162,38 @@ void gw_election(struct bat_priv *bat_priv)
 			  *     soon as a better gateway appears which has
 			  *     $routing_class more tq points)
 			  **/
-			if (gw_node->orig_node->router->tq_avg > max_tq)
+			if (orig_node->router->tq_avg > max_tq)
 				curr_gw_tmp = gw_node;
 			break;
 		}
 
-		if (gw_node->orig_node->router->tq_avg > max_tq)
-			max_tq = gw_node->orig_node->router->tq_avg;
+		if (orig_node->router->tq_avg > max_tq)
+			max_tq = orig_node->router->tq_avg;
 
 		if (tmp_gw_factor > max_gw_factor)
 			max_gw_factor = tmp_gw_factor;
 	}
 
-	if (bat_priv->curr_gw != curr_gw_tmp) {
-		if ((bat_priv->curr_gw) && (!curr_gw_tmp))
+	if (curr_gw != curr_gw_tmp) {
+		orig_node = rcu_dereference(curr_gw_tmp->orig_node);
+		if ((curr_gw) && (!curr_gw_tmp))
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Removing selected gateway - "
 				"no gateway in range\n");
-		else if ((!bat_priv->curr_gw) && (curr_gw_tmp))
+		else if ((!curr_gw) && (curr_gw_tmp))
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Adding route to gateway %pM "
 				"(gw_flags: %i, tq: %i)\n",
-				curr_gw_tmp->orig_node->orig,
-				curr_gw_tmp->orig_node->gw_flags,
-				curr_gw_tmp->orig_node->router->tq_avg);
+				orig_node->orig,
+				orig_node->gw_flags,
+				orig_node->router->tq_avg);
 		else
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Changing route to gateway %pM "
 				"(gw_flags: %i, tq: %i)\n",
-				curr_gw_tmp->orig_node->orig,
-				curr_gw_tmp->orig_node->gw_flags,
-				curr_gw_tmp->orig_node->router->tq_avg);
+				orig_node->orig,
+				orig_node->gw_flags,
+				orig_node->router->tq_avg);
 
 		old_gw_node = gw_select(bat_priv, curr_gw_tmp);
 	}
@@ -181,36 +202,40 @@ void gw_election(struct bat_priv *bat_priv)
 
 	/* the kfree() has to be outside of the rcu lock */
 	if (old_gw_node)
-		kref_put(&old_gw_node->refcount, gw_node_free_ref);
+		gw_node_free_ref(old_gw_node);
 }
 
 void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 {
-	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+	struct gw_node *curr_gateway_tmp;
+	struct orig_node *curr_gw_orig;
 	uint8_t gw_tq_avg, orig_tq_avg;
 
+	rcu_read_lock();
+	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
 	if (!curr_gateway_tmp)
-		return;
+		goto rcu_unlock;
 
-	if (!curr_gateway_tmp->orig_node)
+	curr_gw_orig = rcu_dereference(curr_gateway_tmp->orig_node);
+	if (!curr_gw_orig)
 		goto deselect;
 
-	if (!curr_gateway_tmp->orig_node->router)
+	if (!curr_gw_orig->router)
 		goto deselect;
 
 	/* this node already is the gateway */
-	if (curr_gateway_tmp->orig_node == orig_node)
-		return;
+	if (curr_gw_orig == orig_node)
+		goto deselect;
 
 	if (!orig_node->router)
-		return;
+		goto rcu_unlock;
 
-	gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg;
+	gw_tq_avg = curr_gw_orig ->router->tq_avg;
 	orig_tq_avg = orig_node->router->tq_avg;
 
 	/* the TQ value has to be better */
 	if (orig_tq_avg < gw_tq_avg)
-		return;
+		goto rcu_unlock;
 
 	/**
 	 * if the routing class is greater than 3 the value tells us how much
@@ -218,7 +243,7 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 	 **/
 	if ((atomic_read(&bat_priv->gw_sel_class) > 3) &&
 	    (orig_tq_avg - gw_tq_avg < atomic_read(&bat_priv->gw_sel_class)))
-		return;
+		goto rcu_unlock;
 
 	bat_dbg(DBG_BATMAN, bat_priv,
 		"Restarting gateway selection: better gateway found (tq curr: "
@@ -227,6 +252,8 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 
 deselect:
 	gw_deselect(bat_priv);
+rcu_unlock:
+	rcu_read_unlock();
 }
 
 static void gw_node_add(struct bat_priv *bat_priv,
@@ -242,7 +269,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
 	memset(gw_node, 0, sizeof(struct gw_node));
 	INIT_HLIST_NODE(&gw_node->list);
 	gw_node->orig_node = orig_node;
-	kref_init(&gw_node->refcount);
+	atomic_set(&gw_node->refcount, 1);
 
 	spin_lock_bh(&bat_priv->gw_list_lock);
 	hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -325,7 +352,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
 			gw_deselect(bat_priv);
 
 		hlist_del_rcu(&gw_node->list);
-		call_rcu(&gw_node->rcu, gw_node_free_rcu);
+		gw_node_free_ref(gw_node);
 	}
 
 
@@ -335,21 +362,29 @@ void gw_node_purge(struct bat_priv *bat_priv)
 static int _write_buffer_text(struct bat_priv *bat_priv,
 			      struct seq_file *seq, struct gw_node *gw_node)
 {
-	int down, up;
+	struct gw_node *curr_gw;
+	struct orig_node *orig_node;
+	int down, up, ret;
 
-	gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
-
-	return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
-		       (bat_priv->curr_gw == gw_node ? "=>" : "  "),
-		       gw_node->orig_node->orig,
-		       gw_node->orig_node->router->tq_avg,
-		       gw_node->orig_node->router->addr,
-		       gw_node->orig_node->router->if_incoming->net_dev->name,
-		       gw_node->orig_node->gw_flags,
+	rcu_read_lock();
+	curr_gw = rcu_dereference(bat_priv->curr_gw);
+	orig_node = rcu_dereference(gw_node->orig_node);
+	gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);
+
+	ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
+		       (curr_gw == gw_node ? "=>" : "  "),
+		       orig_node->orig,
+		       orig_node->router->tq_avg,
+		       orig_node->router->addr,
+		       orig_node->router->if_incoming->net_dev->name,
+		       orig_node->gw_flags,
 		       (down > 2048 ? down / 1024 : down),
 		       (down > 2048 ? "MBit" : "KBit"),
 		       (up > 2048 ? up / 1024 : up),
 		       (up > 2048 ? "MBit" : "KBit"));
+	rcu_read_unlock();
+
+	return ret;
 }
 
 int gw_client_seq_print_text(struct seq_file *seq, void *offset)
@@ -470,8 +505,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb)
 	if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER)
 		return -1;
 
-	if (!bat_priv->curr_gw)
+	rcu_read_lock();
+	if (!rcu_dereference(bat_priv->curr_gw)) {
+		rcu_read_unlock();
 		return 0;
+	}
+	rcu_read_unlock();
 
 	return 1;
 }
diff --git a/main.c b/main.c
index e687e7f..8679260 100644
--- a/main.c
+++ b/main.c
@@ -85,6 +85,7 @@ int mesh_init(struct net_device *soft_iface)
 	spin_lock_init(&bat_priv->hna_lhash_lock);
 	spin_lock_init(&bat_priv->hna_ghash_lock);
 	spin_lock_init(&bat_priv->gw_list_lock);
+	spin_lock_init(&bat_priv->curr_gw_lock);
 	spin_lock_init(&bat_priv->vis_hash_lock);
 	spin_lock_init(&bat_priv->vis_list_lock);
 	spin_lock_init(&bat_priv->softif_neigh_lock);
diff --git a/types.h b/types.h
index e4a0462..b9b20b6 100644
--- a/types.h
+++ b/types.h
@@ -98,9 +98,9 @@ struct orig_node {
 
 struct gw_node {
 	struct hlist_node list;
-	struct orig_node *orig_node;
+	struct orig_node *orig_node;	/* rcu protected pointer */
 	unsigned long deleted;
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
 };
 
@@ -163,6 +163,7 @@ struct bat_priv {
 	spinlock_t hna_lhash_lock; /* protects hna_local_hash */
 	spinlock_t hna_ghash_lock; /* protects hna_global_hash */
 	spinlock_t gw_list_lock; /* protects gw_list */
+	spinlock_t curr_gw_lock; /* protects curr_gw updates */
 	spinlock_t vis_hash_lock; /* protects vis_hash */
 	spinlock_t vis_list_lock; /* protects vis_info::recv_list */
 	spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */
@@ -171,7 +172,7 @@ struct bat_priv {
 	struct delayed_work hna_work;
 	struct delayed_work orig_work;
 	struct delayed_work vis_work;
-	struct gw_node *curr_gw;
+	struct gw_node *curr_gw;	/* rcu protected pointer */
 	struct vis_info *my_vis_info;
 };
 
diff --git a/unicast.c b/unicast.c
index 6a9ab61..8816102 100644
--- a/unicast.c
+++ b/unicast.c
@@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
 		if (!orig_node)
 			goto trans_search;
 
-		kref_get(&orig_node->refcount);
 		goto find_router;
 	} else {
 		rcu_read_lock();
-- 
1.7.2.3

Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node

[Marek Lindner]

On Wednesday 02 February 2011 18:37:18 Linus Lüssing wrote:
> So after some more discussions with Marek and Sven, it looks like we
> have to use the rcu protected macros rcu_dereference() and
> rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node.
> 
> Changes here also include moving the kref_get() from unicast_send_skb()
> into gw_get_selected(). The orig_node could have been freed already at
> the time the kref_get() was called in unicast_send_skb().

I'd suggest you make a standalone patch because the patches address different 
problems.

Thanks,
Marek

Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node

[Linus Lüssing]

On Wed, Feb 02, 2011 at 08:49:17PM +0100, Marek Lindner wrote:
> On Wednesday 02 February 2011 18:37:18 Linus Lüssing wrote:
> > So after some more discussions with Marek and Sven, it looks like we
> > have to use the rcu protected macros rcu_dereference() and
> > rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node.
> > 
> > Changes here also include moving the kref_get() from unicast_send_skb()
> > into gw_get_selected(). The orig_node could have been freed already at
> > the time the kref_get() was called in unicast_send_skb().
> 
> I'd suggest you make a standalone patch because the patches address different 
> problems.
> 
> Thanks,
> Marek
> 

Oki doki, will do that (wasn't ment to be a clean patch yet anyway
- I wanted to know first if the usage of
rcu_dereference/rcu_assign_pointer() was going in the right
direction).

Cheers, Linus

Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node

[Sven Eckelmann]

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

On Wednesday 02 February 2011 18:37:18 Linus Lüssing wrote:
> From: Sven Eckelmann <sven@narfation.org>
> 
> Was:
> ---
> <TODO: write a long monologue about every problem we have or could have or
> maybe never had and would have when we not have it>
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> 
> So after some more discussions with Marek and Sven, it looks like we
> have to use the rcu protected macros rcu_dereference() and
> rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node.
> 
> Changes here also include moving the kref_get() from unicast_send_skb()
> into gw_get_selected(). The orig_node could have been freed already at
> the time the kref_get() was called in unicast_send_skb().
> 
> Some things that are still not that clear to me:
> 
> gw_election():
> * can the if-block before gw_deselect() be ommited, we had a nullpointer
>   check for curr_gw just a couple of lines before during the rcu-lock.

I thought that this if block should be moved to gw_select. And your gw_select 
still has the bug that the bat_priv->curr_gw isn't set to NULL when 
new_gw_node is NULL.


> gw_deselet():
> * is the refcount at this time always 1 for gw_node, can the null
> pointer check + a rcu_dereference be ommited? (at least that's what
> it looks like when comparing to the rcuref.txt example)

Why can't it be NULL? And _always_ use rcu_dereference. What example tells you 
that it isn't needed? None of the examples has any kind of rcu pointer in it 
(just el as pointer which is stored in a struct were the pointer inside the 
struct is rcu protected).


> gw_get_selected():
> * Probably the orig_node's refcounting has to be made atomic, too?

This part is still a little bit ugly and I cannot give you an easy answer. 
Just think about following:
 * Hash list is a bunch of rcu protected lists
 * pointer to originator is stored inside a bucket (list elements inside the
   hash)
 * hash bucket wants to get removed - call_rcu; reference count of the
   originator is decremented immediately
 * (!!!! lots of reordering of read and write commands inside the cpu!!!! -
    aren't we happy about the added complexity which tries to hide the memory
    latency?)
 * the originator was removed, the bucket which is removed in the call_rcu
   still points to the removed originator
 * a parallel running operation tries to find a originator, the rcu list
   iterator gets the to-be-deleted bucket to the originator
 * the pointer to the already removed originator inside the bucket is
   dereferenced, data is read/written -> Kernel Oops

Does this sound scary? At least it could be used in some horror movies (and I 
would watch them).

But that is the other problem I currently have with the state of batman-adv in 
trunk - and I think I forget to tell you about it after the release of 
v2011.0.0.

So, a good idea would be the removal of the buckets for the hash. Usage of 
"struct hlist_node" inside the hash elements should be a good starting point. 
But think about the problem that the different hashes could have the same 
element. So you need for each distinct hash an extra "struct hlist_node" 
inside the element which should be part of the hash. The hash_add (and 
related) functions don't get the actual pointer to the element, but the 
pointer to the correct "struct hlist_node" inside the element/struct. The 
comparison and hashing function would also receive "struct hlist_node" as 
parameter and must get the pointer to the element using the container_of 
macro.


> @@ -171,7 +172,7 @@ struct bat_priv {
>         struct delayed_work hna_work;
>         struct delayed_work orig_work;
>         struct delayed_work vis_work;
> -       struct gw_node *curr_gw;
> +       struct gw_node *curr_gw;        /* rcu protected pointer */
>         struct vis_info *my_vis_info;
>  };

Sry, but I have to say that: FAIL ;)

I think it should look that way:
> -       struct gw_node *curr_gw;
> +       struct gw_node __rcu *curr_gw;

Best regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node

[Marek Lindner]

On Wednesday 02 February 2011 22:42:46 Sven Eckelmann wrote:
> > gw_election():
> > * can the if-block before gw_deselect() be ommited, we had a nullpointer
> > 
> >   check for curr_gw just a couple of lines before during the rcu-lock.
> 
> I thought that this if block should be moved to gw_select. And your
> gw_select still has the bug that the bat_priv->curr_gw isn't set to NULL
> when new_gw_node is NULL.

Yes, but we discussed this without Linus.

@Linus: This section will be changed - I'm working on this.

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node

[Linus Lüssing]

Hi Sven,

On Wed, Feb 02, 2011 at 10:42:46PM +0100, Sven Eckelmann wrote:
> > gw_deselet():
> > * is the refcount at this time always 1 for gw_node, can the null
> > pointer check + a rcu_dereference be ommited? (at least that's what
> > it looks like when comparing to the rcuref.txt example)
> 
> Why can't it be NULL? And _always_ use rcu_dereference. What example tells you 
> that it isn't needed? None of the examples has any kind of rcu pointer in it 
> (just el as pointer which is stored in a struct were the pointer inside the 
> struct is rcu protected).
Ok, you got a point there with the always-rcu-dereference pointers. I
somehow was thinking that in between the spin-lock/unlock there
could possibly be no other thread reading/writing to it then - but
I guess at that moment I forgot about the reordering and the whole
point of using the rcu macros between the spinlock there :). So,
yes, you're right with that one, will change it.

For the NULL pointer, guess you're right again. I was looking at
the delete() example in rcuref.txt which was not doing any NULL
pointer check. But either that's the case there because it's more
pseudo-code there or because it's more related to lists, meaning
that after the delete_element there it's not in the list anymore
and not possible for any other thread to have the idea to free the
same thing again.
> 
> 
> > gw_get_selected():
> > * Probably the orig_node's refcounting has to be made atomic, too?
> 
> This part is still a little bit ugly and I cannot give you an easy answer. 
> Just think about following:
>  * Hash list is a bunch of rcu protected lists
>  * pointer to originator is stored inside a bucket (list elements inside the
>    hash)
>  * hash bucket wants to get removed - call_rcu; reference count of the
>    originator is decremented immediately
>  * (!!!! lots of reordering of read and write commands inside the cpu!!!! -
>     aren't we happy about the added complexity which tries to hide the memory
>     latency?)
>  * the originator was removed, the bucket which is removed in the call_rcu
>    still points to the removed originator
>  * a parallel running operation tries to find a originator, the rcu list
>    iterator gets the to-be-deleted bucket to the originator
>  * the pointer to the already removed originator inside the bucket is
>    dereferenced, data is read/written -> Kernel Oops
> 
> Does this sound scary? At least it could be used in some horror movies (and I 
> would watch them).
> 
> But that is the other problem I currently have with the state of batman-adv in 
> trunk - and I think I forget to tell you about it after the release of 
> v2011.0.0.
> 
> So, a good idea would be the removal of the buckets for the hash. Usage of 
> "struct hlist_node" inside the hash elements should be a good starting point. 
> But think about the problem that the different hashes could have the same 
> element. So you need for each distinct hash an extra "struct hlist_node" 
> inside the element which should be part of the hash. The hash_add (and 
> related) functions don't get the actual pointer to the element, but the 
> pointer to the correct "struct hlist_node" inside the element/struct. The 
> comparison and hashing function would also receive "struct hlist_node" as 
> parameter and must get the pointer to the element using the container_of 
> macro.
> 
> 
> > @@ -171,7 +172,7 @@ struct bat_priv {
> >         struct delayed_work hna_work;
> >         struct delayed_work orig_work;
> >         struct delayed_work vis_work;
> > -       struct gw_node *curr_gw;
> > +       struct gw_node *curr_gw;        /* rcu protected pointer */
> >         struct vis_info *my_vis_info;
> >  };
> 
> Sry, but I have to say that: FAIL ;)
> 
> I think it should look that way:
> > -       struct gw_node *curr_gw;
> > +       struct gw_node __rcu *curr_gw;
Eh, had been looking at whatisRCU.txt and there gbl_foo in section
3 did not have a "__rcu" (actually I hadn't seen that in any of the
documentations before).
> 
> Best regards,
> 	Sven

Cheers, Linus

Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node

[Sven Eckelmann]

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

Linus Lüssing wrote:
> > I think it should look that way:
> > > -       struct gw_node *curr_gw;
> > > +       struct gw_node __rcu *curr_gw;
> 
> Eh, had been looking at whatisRCU.txt and there gbl_foo in section
> 3 did not have a "__rcu" (actually I hadn't seen that in any of the
> documentations before).

$ git show v2.6.37:Documentation/RCU/checklist.txt|grep __rcu

That document explains it quite well how the sparse check works and why you 
must use __rcu to say "hey, this is a pointer which is used in a special way).

Best regards,
	Sven

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

[B.A.T.M.A.N.] [PATCH 1/3] batman-adv: Correct rcu refcounting for gw_node

[Linus Lüssing]

For one thing, atomic refcounters for the gw_list need to be used when
using both refcounting and rcu locking (see
Documentation/RCU/rcuref.txt for details).

For another the rcu protected macros rcu_dereference() and
rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node
need to be used.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 gateway_client.c |  182 +++++++++++++++++++++++++++++++++---------------------
 main.c           |    1 +
 types.h          |    3 +-
 unicast.c        |    1 -
 4 files changed, 114 insertions(+), 73 deletions(-)

diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 429a013..ee71335 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -28,58 +28,77 @@
 #include <linux/udp.h>
 #include <linux/if_vlan.h>
 
-static void gw_node_free_ref(struct kref *refcount)
-{
-	struct gw_node *gw_node;
-
-	gw_node = container_of(refcount, struct gw_node, refcount);
-	kfree(gw_node);
-}
-
 static void gw_node_free_rcu(struct rcu_head *rcu)
 {
 	struct gw_node *gw_node;
 
 	gw_node = container_of(rcu, struct gw_node, rcu);
-	kref_put(&gw_node->refcount, gw_node_free_ref);
+	kfree(gw_node);
 }
 
+static void gw_node_free_ref(struct gw_node *gw_node)
+{
+	if (atomic_dec_and_test(&gw_node->refcount))
+		call_rcu(&gw_node->rcu, gw_node_free_rcu);
+}
+
+/* increases the returned orig_node's refcount */
 void *gw_get_selected(struct bat_priv *bat_priv)
 {
-	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+	struct gw_node *curr_gateway_tmp;
+	struct orig_node *orig_node;
 
-	if (!curr_gateway_tmp)
+	rcu_read_lock();
+	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
+	if (!curr_gateway_tmp) {
+		rcu_read_unlock();
 		return NULL;
+	}
 
-	return curr_gateway_tmp->orig_node;
+	orig_node = rcu_dereference(curr_gateway_tmp->orig_node);
+	if (orig_node) {
+		rcu_read_unlock();
+		return NULL;
+	}
+
+	rcu_read_unlock();
+	return orig_node;
 }
 
 void gw_deselect(struct bat_priv *bat_priv)
 {
-	struct gw_node *gw_node = bat_priv->curr_gw;
+	struct gw_node *gw_node;
 
-	bat_priv->curr_gw = NULL;
+	spin_lock_bh(&bat_priv->curr_gw_lock);
+	gw_node = rcu_dereference(bat_priv->curr_gw);
+	rcu_assign_pointer(bat_priv->curr_gw, NULL);
+	spin_unlock_bh(&bat_priv->curr_gw_lock);
 
 	if (gw_node)
-		kref_put(&gw_node->refcount, gw_node_free_ref);
+		gw_node_free_ref(gw_node);
 }
 
 static struct gw_node *gw_select(struct bat_priv *bat_priv,
 			  struct gw_node *new_gw_node)
 {
-	struct gw_node *curr_gw_node = bat_priv->curr_gw;
+	struct gw_node *curr_gw_node;
 
-	if (new_gw_node)
-		kref_get(&new_gw_node->refcount);
+	if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+		return NULL;
+
+	spin_lock_bh(&bat_priv->curr_gw_lock);
+	curr_gw_node = rcu_dereference(bat_priv->curr_gw);
+	rcu_assign_pointer(bat_priv->curr_gw, new_gw_node);
+	spin_unlock_bh(&bat_priv->curr_gw_lock);
 
-	bat_priv->curr_gw = new_gw_node;
 	return curr_gw_node;
 }
 
 void gw_election(struct bat_priv *bat_priv)
 {
 	struct hlist_node *node;
-	struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+	struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+	struct orig_node *orig_node;
 	uint8_t max_tq = 0;
 	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
 	int down, up;
@@ -93,25 +112,28 @@ void gw_election(struct bat_priv *bat_priv)
 	if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT)
 		return;
 
-	if (bat_priv->curr_gw)
+	rcu_read_lock();
+	curr_gw = rcu_dereference(bat_priv->curr_gw);
+	if (curr_gw) {
+		rcu_read_unlock();
 		return;
+	}
 
-	rcu_read_lock();
 	if (hlist_empty(&bat_priv->gw_list)) {
-		rcu_read_unlock();
-
-		if (bat_priv->curr_gw) {
+		if (curr_gw) {
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Removing selected gateway - "
 				"no gateway in range\n");
 			gw_deselect(bat_priv);
 		}
+		rcu_read_unlock();
 
 		return;
 	}
 
 	hlist_for_each_entry_rcu(gw_node, node, &bat_priv->gw_list, list) {
-		if (!gw_node->orig_node->router)
+		orig_node = rcu_dereference(gw_node->orig_node);
+		if (!orig_node->router)
 			continue;
 
 		if (gw_node->deleted)
@@ -119,18 +141,17 @@ void gw_election(struct bat_priv *bat_priv)
 
 		switch (atomic_read(&bat_priv->gw_sel_class)) {
 		case 1: /* fast connection */
-			gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags,
-					     &down, &up);
+			gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);
 
-			tmp_gw_factor = (gw_node->orig_node->router->tq_avg *
-					 gw_node->orig_node->router->tq_avg *
+			tmp_gw_factor = (orig_node->router->tq_avg *
+					 orig_node->router->tq_avg *
 					 down * 100 * 100) /
 					 (TQ_LOCAL_WINDOW_SIZE *
 					 TQ_LOCAL_WINDOW_SIZE * 64);
 
 			if ((tmp_gw_factor > max_gw_factor) ||
 			    ((tmp_gw_factor == max_gw_factor) &&
-			     (gw_node->orig_node->router->tq_avg > max_tq)))
+			     (orig_node->router->tq_avg > max_tq)))
 				curr_gw_tmp = gw_node;
 			break;
 
@@ -142,37 +163,38 @@ void gw_election(struct bat_priv *bat_priv)
 			  *     soon as a better gateway appears which has
 			  *     $routing_class more tq points)
 			  **/
-			if (gw_node->orig_node->router->tq_avg > max_tq)
+			if (orig_node->router->tq_avg > max_tq)
 				curr_gw_tmp = gw_node;
 			break;
 		}
 
-		if (gw_node->orig_node->router->tq_avg > max_tq)
-			max_tq = gw_node->orig_node->router->tq_avg;
+		if (orig_node->router->tq_avg > max_tq)
+			max_tq = orig_node->router->tq_avg;
 
 		if (tmp_gw_factor > max_gw_factor)
 			max_gw_factor = tmp_gw_factor;
 	}
 
-	if (bat_priv->curr_gw != curr_gw_tmp) {
-		if ((bat_priv->curr_gw) && (!curr_gw_tmp))
+	if (curr_gw != curr_gw_tmp) {
+		orig_node = rcu_dereference(curr_gw_tmp->orig_node);
+		if ((curr_gw) && (!curr_gw_tmp))
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Removing selected gateway - "
 				"no gateway in range\n");
-		else if ((!bat_priv->curr_gw) && (curr_gw_tmp))
+		else if ((!curr_gw) && (curr_gw_tmp))
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Adding route to gateway %pM "
 				"(gw_flags: %i, tq: %i)\n",
-				curr_gw_tmp->orig_node->orig,
-				curr_gw_tmp->orig_node->gw_flags,
-				curr_gw_tmp->orig_node->router->tq_avg);
+				orig_node->orig,
+				orig_node->gw_flags,
+				orig_node->router->tq_avg);
 		else
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Changing route to gateway %pM "
 				"(gw_flags: %i, tq: %i)\n",
-				curr_gw_tmp->orig_node->orig,
-				curr_gw_tmp->orig_node->gw_flags,
-				curr_gw_tmp->orig_node->router->tq_avg);
+				orig_node->orig,
+				orig_node->gw_flags,
+				orig_node->router->tq_avg);
 
 		old_gw_node = gw_select(bat_priv, curr_gw_tmp);
 	}
@@ -181,36 +203,40 @@ void gw_election(struct bat_priv *bat_priv)
 
 	/* the kfree() has to be outside of the rcu lock */
 	if (old_gw_node)
-		kref_put(&old_gw_node->refcount, gw_node_free_ref);
+		gw_node_free_ref(old_gw_node);
 }
 
 void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 {
-	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+	struct gw_node *curr_gateway_tmp;
+	struct orig_node *curr_gw_orig;
 	uint8_t gw_tq_avg, orig_tq_avg;
 
+	rcu_read_lock();
+	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
 	if (!curr_gateway_tmp)
-		return;
+		goto rcu_unlock;
 
-	if (!curr_gateway_tmp->orig_node)
+	curr_gw_orig = rcu_dereference(curr_gateway_tmp->orig_node);
+	if (!curr_gw_orig)
 		goto deselect;
 
-	if (!curr_gateway_tmp->orig_node->router)
+	if (!curr_gw_orig->router)
 		goto deselect;
 
 	/* this node already is the gateway */
-	if (curr_gateway_tmp->orig_node == orig_node)
-		return;
+	if (curr_gw_orig == orig_node)
+		goto deselect;
 
 	if (!orig_node->router)
-		return;
+		goto rcu_unlock;
 
-	gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg;
+	gw_tq_avg = curr_gw_orig ->router->tq_avg;
 	orig_tq_avg = orig_node->router->tq_avg;
 
 	/* the TQ value has to be better */
 	if (orig_tq_avg < gw_tq_avg)
-		return;
+		goto rcu_unlock;
 
 	/**
 	 * if the routing class is greater than 3 the value tells us how much
@@ -218,7 +244,7 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 	 **/
 	if ((atomic_read(&bat_priv->gw_sel_class) > 3) &&
 	    (orig_tq_avg - gw_tq_avg < atomic_read(&bat_priv->gw_sel_class)))
-		return;
+		goto rcu_unlock;
 
 	bat_dbg(DBG_BATMAN, bat_priv,
 		"Restarting gateway selection: better gateway found (tq curr: "
@@ -227,6 +253,8 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 
 deselect:
 	gw_deselect(bat_priv);
+rcu_unlock:
+	rcu_read_unlock();
 }
 
 static void gw_node_add(struct bat_priv *bat_priv,
@@ -242,7 +270,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
 	memset(gw_node, 0, sizeof(struct gw_node));
 	INIT_HLIST_NODE(&gw_node->list);
 	gw_node->orig_node = orig_node;
-	kref_init(&gw_node->refcount);
+	atomic_set(&gw_node->refcount, 1);
 
 	spin_lock_bh(&bat_priv->gw_list_lock);
 	hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -266,13 +294,13 @@ void gw_node_update(struct bat_priv *bat_priv,
 
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(gw_node, node, &bat_priv->gw_list, list) {
-		if (gw_node->orig_node != orig_node)
+		if (rcu_dereference(gw_node->orig_node) != orig_node)
 			continue;
 
 		bat_dbg(DBG_BATMAN, bat_priv,
 			"Gateway class of originator %pM changed from "
-			"%i to %i\n",
-			orig_node->orig, gw_node->orig_node->gw_flags,
+			"%i to %i\n", orig_node->orig,
+			rcu_dereference(gw_node->orig_node)->gw_flags,
 			new_gwflags);
 
 		gw_node->deleted = 0;
@@ -283,7 +311,7 @@ void gw_node_update(struct bat_priv *bat_priv,
 				"Gateway %pM removed from gateway list\n",
 				orig_node->orig);
 
-			if (gw_node == bat_priv->curr_gw) {
+			if (gw_node == rcu_dereference(bat_priv->curr_gw)) {
 				rcu_read_unlock();
 				gw_deselect(bat_priv);
 				return;
@@ -321,11 +349,11 @@ void gw_node_purge(struct bat_priv *bat_priv)
 		    atomic_read(&bat_priv->mesh_state) == MESH_ACTIVE)
 			continue;
 
-		if (bat_priv->curr_gw == gw_node)
+		if (rcu_dereference(bat_priv->curr_gw) == gw_node)
 			gw_deselect(bat_priv);
 
 		hlist_del_rcu(&gw_node->list);
-		call_rcu(&gw_node->rcu, gw_node_free_rcu);
+		gw_node_free_ref(gw_node);
 	}
 
 
@@ -335,21 +363,29 @@ void gw_node_purge(struct bat_priv *bat_priv)
 static int _write_buffer_text(struct bat_priv *bat_priv,
 			      struct seq_file *seq, struct gw_node *gw_node)
 {
-	int down, up;
+	struct gw_node *curr_gw;
+	struct orig_node *orig_node;
+	int down, up, ret;
 
-	gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
+	rcu_read_lock();
+	curr_gw = rcu_dereference(bat_priv->curr_gw);
+	orig_node = rcu_dereference(gw_node->orig_node);
+	gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);
 
-	return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
-		       (bat_priv->curr_gw == gw_node ? "=>" : "  "),
-		       gw_node->orig_node->orig,
-		       gw_node->orig_node->router->tq_avg,
-		       gw_node->orig_node->router->addr,
-		       gw_node->orig_node->router->if_incoming->net_dev->name,
-		       gw_node->orig_node->gw_flags,
+	ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
+		       (curr_gw == gw_node ? "=>" : "  "),
+		       orig_node->orig,
+		       orig_node->router->tq_avg,
+		       orig_node->router->addr,
+		       orig_node->router->if_incoming->net_dev->name,
+		       orig_node->gw_flags,
 		       (down > 2048 ? down / 1024 : down),
 		       (down > 2048 ? "MBit" : "KBit"),
 		       (up > 2048 ? up / 1024 : up),
 		       (up > 2048 ? "MBit" : "KBit"));
+	rcu_read_unlock();
+
+	return ret;
 }
 
 int gw_client_seq_print_text(struct seq_file *seq, void *offset)
@@ -470,8 +506,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb)
 	if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER)
 		return -1;
 
-	if (!bat_priv->curr_gw)
+	rcu_read_lock();
+	if (!rcu_dereference(bat_priv->curr_gw)) {
+		rcu_read_unlock();
 		return 0;
+	}
+	rcu_read_unlock();
 
 	return 1;
 }
diff --git a/batman-adv/main.c b/batman-adv/main.c
index e687e7f..8679260 100644
--- a/batman-adv/main.c
+++ b/batman-adv/main.c
@@ -85,6 +85,7 @@ int mesh_init(struct net_device *soft_iface)
 	spin_lock_init(&bat_priv->hna_lhash_lock);
 	spin_lock_init(&bat_priv->hna_ghash_lock);
 	spin_lock_init(&bat_priv->gw_list_lock);
+	spin_lock_init(&bat_priv->curr_gw_lock);
 	spin_lock_init(&bat_priv->vis_hash_lock);
 	spin_lock_init(&bat_priv->vis_list_lock);
 	spin_lock_init(&bat_priv->softif_neigh_lock);
diff --git a/batman-adv/types.h b/batman-adv/types.h
index e4a0462..30d10c0 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -100,7 +100,7 @@ struct gw_node {
 	struct hlist_node list;
 	struct orig_node *orig_node;
 	unsigned long deleted;
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
 };
 
@@ -163,6 +163,7 @@ struct bat_priv {
 	spinlock_t hna_lhash_lock; /* protects hna_local_hash */
 	spinlock_t hna_ghash_lock; /* protects hna_global_hash */
 	spinlock_t gw_list_lock; /* protects gw_list */
+	spinlock_t curr_gw_lock; /* protects curr_gw updates */
 	spinlock_t vis_hash_lock; /* protects vis_hash */
 	spinlock_t vis_list_lock; /* protects vis_info::recv_list */
 	spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..8816102 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
 		if (!orig_node)
 			goto trans_search;
 
-		kref_get(&orig_node->refcount);
 		goto find_router;
 	} else {
 		rcu_read_lock();
-- 
1.7.2.3

Re: [B.A.T.M.A.N.] [PATCH 1/3] batman-adv: Correct rcu refcounting for gw_node

[Sven Eckelmann]

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

> For another the rcu protected macros rcu_dereference() and
> rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node
> need to be used.

What? Since when is curr_gw::orig_node rcu protected?

> -	return curr_gateway_tmp->orig_node;
> +	orig_node = rcu_dereference(curr_gateway_tmp->orig_node);
> +	if (orig_node) {
> +		rcu_read_unlock();
> +		return NULL;
> +	}
> +
> +	rcu_read_unlock();
> +	return orig_node;
>  }


And you cannot return the (it is not, but when it would be) rcu protected 
element without increasing the refcounter.

> +		orig_node = rcu_dereference(gw_node->orig_node);
> +		if (!orig_node->router)
>  			continue;

Same here

> -	if (bat_priv->curr_gw != curr_gw_tmp) {
> -		if ((bat_priv->curr_gw) && (!curr_gw_tmp))
> +	if (curr_gw != curr_gw_tmp) {
> +		orig_node = rcu_dereference(curr_gw_tmp->orig_node);
> +		if ((curr_gw) && (!curr_gw_tmp))

And here

> -	if (bat_priv->curr_gw != curr_gw_tmp) {
> -		if ((bat_priv->curr_gw) && (!curr_gw_tmp))
> +	if (curr_gw != curr_gw_tmp) {
> +		orig_node = rcu_dereference(curr_gw_tmp->orig_node);
> +		if ((curr_gw) && (!curr_gw_tmp))

Here again

> -	if (bat_priv->curr_gw != curr_gw_tmp) {
> -		if ((bat_priv->curr_gw) && (!curr_gw_tmp))
> +	if (curr_gw != curr_gw_tmp) {
> +		orig_node = rcu_dereference(curr_gw_tmp->orig_node);
> +		if ((curr_gw) && (!curr_gw_tmp))

Here....

> -	gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
> +	rcu_read_lock();
> +	curr_gw = rcu_dereference(bat_priv->curr_gw);
> +	orig_node = rcu_dereference(gw_node->orig_node);
> +	gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);

....

> diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
> index 6a9ab61..8816102 100644
> --- a/batman-adv/unicast.c
> +++ b/batman-adv/unicast.c
> @@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct
> bat_priv *bat_priv) if (!orig_node)
>  			goto trans_search;
> 
> -		kref_get(&orig_node->refcount);
>  		goto find_router;
>  	} else {
>  		rcu_read_lock();

Why? Maybe this should be part of the patch 3/3?

And please to merge the refcounting stuff with the rcu pointer in one patch... 
and maybe it is better to merge patch 2/3 with the rcu pointer patch.

Best regards,
	Sven

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

[B.A.T.M.A.N.] [PATCH 2/3] batman-adv: add __rcu annotations for gw_node

[Linus Lüssing]

Add __rcu annotations for rcu protected pointers in the gateway code
to allow sparse checking.

Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 compat.h |    6 ++++++
 types.h  |    4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/batman-adv/compat.h b/batman-adv/compat.h
index a76d0be..4e89049 100644
--- a/batman-adv/compat.h
+++ b/batman-adv/compat.h
@@ -270,4 +270,10 @@ int bat_seq_printf(struct seq_file *m, const char *f, ...);
 
 #endif /* < KERNEL_VERSION(2, 6, 33) */
 
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 36)
+
+#define __rcu
+
+#endif /* < KERNEL_VERSION(2, 6, 36) */
+
 #endif /* _NET_BATMAN_ADV_COMPAT_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 30d10c0..2cb0c31 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -98,7 +98,7 @@ struct orig_node {
 
 struct gw_node {
 	struct hlist_node list;
-	struct orig_node *orig_node;
+	struct orig_node __rcu *orig_node;	/* rcu protected pointer */
 	unsigned long deleted;
 	atomic_t refcount;
 	struct rcu_head rcu;
@@ -172,7 +172,7 @@ struct bat_priv {
 	struct delayed_work hna_work;
 	struct delayed_work orig_work;
 	struct delayed_work vis_work;
-	struct gw_node *curr_gw;
+	struct gw_node __rcu *curr_gw;	/* rcu protected pointer */
 	struct vis_info *my_vis_info;
 };
 
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH 3/3] batman-adv: Increase orig_node refcount before releasing rcu read lock

[Linus Lüssing]

When unicast_send_skb() is increasing the orig_node's refcount another
thread might have been freeing this orig_node already. We need to
increase the refcount in the rcu read lock protected area to avoid that.

Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 gateway_client.c |    1 +
 unicast.c        |    1 -
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index ee71335..15ea268 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -57,6 +57,7 @@ void *gw_get_selected(struct bat_priv *bat_priv)
 
 	orig_node = rcu_dereference(curr_gateway_tmp->orig_node);
 	if (orig_node) {
+		kref_get(&orig_node->refcount);
 		rcu_read_unlock();
 		return NULL;
 	}
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 8816102..b42e40e 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -310,7 +310,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
 			goto trans_search;
 		}
 
-		kref_get(&orig_node->refcount);
 		rcu_read_unlock();
 		goto find_router;
 	}
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh

[Sven Eckelmann]

<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 batman-adv/soft-interface.c |   34 ++++++++++++++++------------------
 batman-adv/types.h          |    2 +-
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c
index 145e0f7..9fb8a6a 100644
--- a/batman-adv/soft-interface.c
+++ b/batman-adv/soft-interface.c
@@ -79,20 +79,18 @@ int my_skb_head_push(struct sk_buff *skb, unsigned int len)
 	return 0;
 }
 
-static void softif_neigh_free_ref(struct kref *refcount)
-{
-	struct softif_neigh *softif_neigh;
-
-	softif_neigh = container_of(refcount, struct softif_neigh, refcount);
-	kfree(softif_neigh);
-}
-
 static void softif_neigh_free_rcu(struct rcu_head *rcu)
 {
 	struct softif_neigh *softif_neigh;
 
 	softif_neigh = container_of(rcu, struct softif_neigh, rcu);
-	kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+	kfree(softif_neigh);
+}
+
+static void softif_neigh_free_ref(struct softif_neigh *softif_neigh)
+{
+	if (atomic_dec_and_test(&softif_neigh->refcount))
+		call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
 }
 
 void softif_neigh_purge(struct bat_priv *bat_priv)
@@ -119,11 +117,10 @@ void softif_neigh_purge(struct bat_priv *bat_priv)
 				 softif_neigh->addr, softif_neigh->vid);
 			softif_neigh_tmp = bat_priv->softif_neigh;
 			bat_priv->softif_neigh = NULL;
-			kref_put(&softif_neigh_tmp->refcount,
-				 softif_neigh_free_ref);
+			softif_neigh_free_ref(softif_neigh_tmp);
 		}
 
-		call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
+		softif_neigh_free_ref(softif_neigh);
 	}
 
 	spin_unlock_bh(&bat_priv->softif_neigh_lock);
@@ -144,8 +141,11 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
 		if (softif_neigh->vid != vid)
 			continue;
 
+		if (!atomic_inc_not_zero(&softif_neigh->refcount))
+			continue;
+
 		softif_neigh->last_seen = jiffies;
-		goto found;
+		goto out;
 	}
 
 	softif_neigh = kzalloc(sizeof(struct softif_neigh), GFP_ATOMIC);
@@ -155,15 +155,13 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
 	memcpy(softif_neigh->addr, addr, ETH_ALEN);
 	softif_neigh->vid = vid;
 	softif_neigh->last_seen = jiffies;
-	kref_init(&softif_neigh->refcount);
+	atomic_set(&softif_neigh->refcount, 1);
 
 	INIT_HLIST_NODE(&softif_neigh->list);
 	spin_lock_bh(&bat_priv->softif_neigh_lock);
 	hlist_add_head_rcu(&softif_neigh->list, &bat_priv->softif_neigh_list);
 	spin_unlock_bh(&bat_priv->softif_neigh_lock);
 
-found:
-	kref_get(&softif_neigh->refcount);
 out:
 	rcu_read_unlock();
 	return softif_neigh;
@@ -267,7 +265,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
 			 softif_neigh->addr, softif_neigh->vid);
 		softif_neigh_tmp = bat_priv->softif_neigh;
 		bat_priv->softif_neigh = softif_neigh;
-		kref_put(&softif_neigh_tmp->refcount, softif_neigh_free_ref);
+		softif_neigh_free_ref(softif_neigh_tmp);
 		/* we need to hold the additional reference */
 		goto err;
 	}
@@ -285,7 +283,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
 	}
 
 out:
-	kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+	softif_neigh_free_ref(softif_neigh);
 err:
 	kfree_skb(skb);
 	return;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index ca5f20a..8df6d9d 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -270,7 +270,7 @@ struct softif_neigh {
 	uint8_t addr[ETH_ALEN];
 	unsigned long last_seen;
 	short vid;
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
 };
 
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if

[Sven Eckelmann]

<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 batman-adv/bat_sysfs.c      |   12 ++++++------
 batman-adv/hard-interface.c |   40 +++++++++++++++++++---------------------
 batman-adv/hard-interface.h |    9 ++++-----
 batman-adv/types.h          |    2 +-
 4 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/batman-adv/bat_sysfs.c b/batman-adv/bat_sysfs.c
index f7b93a0..08f9251 100644
--- a/batman-adv/bat_sysfs.c
+++ b/batman-adv/bat_sysfs.c
@@ -450,7 +450,7 @@ static ssize_t show_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	length = sprintf(buff, "%s\n", batman_if->if_status == IF_NOT_IN_USE ?
 			 "none" : batman_if->soft_iface->name);
 
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 
 	return length;
 }
@@ -472,7 +472,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	if (strlen(buff) >= IFNAMSIZ) {
 		pr_err("Invalid parameter for 'mesh_iface' setting received: "
 		       "interface name too long '%s'\n", buff);
-		kref_put(&batman_if->refcount, hardif_free_ref);
+		hardif_free_ref(batman_if);
 		return -EINVAL;
 	}
 
@@ -483,7 +483,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 
 	if ((batman_if->if_status == status_tmp) || ((batman_if->soft_iface) &&
 	    (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) {
-		kref_put(&batman_if->refcount, hardif_free_ref);
+		hardif_free_ref(batman_if);
 		return count;
 	}
 
@@ -491,7 +491,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 		rtnl_lock();
 		hardif_disable_interface(batman_if);
 		rtnl_unlock();
-		kref_put(&batman_if->refcount, hardif_free_ref);
+		hardif_free_ref(batman_if);
 		return count;
 	}
 
@@ -503,7 +503,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	}
 
 	ret = hardif_enable_interface(batman_if, buff);
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 
 	return ret;
 }
@@ -537,7 +537,7 @@ static ssize_t show_iface_status(struct kobject *kobj, struct attribute *attr,
 		break;
 	}
 
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 
 	return length;
 }
diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index e2b001a..3f59c52 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -40,13 +40,13 @@ static int batman_skb_recv(struct sk_buff *skb,
 			   struct packet_type *ptype,
 			   struct net_device *orig_dev);
 
-static void hardif_free_rcu(struct rcu_head *rcu)
+void hardif_free_rcu(struct rcu_head *rcu)
 {
 	struct batman_if *batman_if;
 
 	batman_if = container_of(rcu, struct batman_if, rcu);
 	dev_put(batman_if->net_dev);
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	kfree(batman_if);
 }
 
 struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
@@ -55,16 +55,14 @@ struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(batman_if, &if_list, list) {
-		if (batman_if->net_dev == net_dev)
+		if (batman_if->net_dev == net_dev &&
+		    atomic_inc_not_zero(&batman_if->refcount))
 			goto out;
 	}
 
 	batman_if = NULL;
 
 out:
-	if (batman_if)
-		kref_get(&batman_if->refcount);
-
 	rcu_read_unlock();
 	return batman_if;
 }
@@ -105,16 +103,14 @@ static struct batman_if *get_active_batman_if(struct net_device *soft_iface)
 		if (batman_if->soft_iface != soft_iface)
 			continue;
 
-		if (batman_if->if_status == IF_ACTIVE)
+		if (batman_if->if_status == IF_ACTIVE &&
+		    atomic_inc_not_zero(&batman_if->refcount))
 			goto out;
 	}
 
 	batman_if = NULL;
 
 out:
-	if (batman_if)
-		kref_get(&batman_if->refcount);
-
 	rcu_read_unlock();
 	return batman_if;
 }
@@ -137,14 +133,14 @@ static void set_primary_if(struct bat_priv *bat_priv,
 	struct batman_packet *batman_packet;
 	struct batman_if *old_if;
 
-	if (batman_if)
-		kref_get(&batman_if->refcount);
+	if (batman_if && !atomic_inc_not_zero(&batman_if->refcount))
+		batman_if = NULL;
 
 	old_if = bat_priv->primary_if;
 	bat_priv->primary_if = batman_if;
 
 	if (old_if)
-		kref_put(&old_if->refcount, hardif_free_ref);
+		hardif_free_ref(old_if);
 
 	if (!bat_priv->primary_if)
 		return;
@@ -290,6 +286,9 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
 	if (batman_if->if_status != IF_NOT_IN_USE)
 		goto out;
 
+	if (!atomic_inc_not_zero(&batman_if->refcount))
+		goto out;
+
 	batman_if->soft_iface = dev_get_by_name(&init_net, iface_name);
 
 	if (!batman_if->soft_iface) {
@@ -328,7 +327,6 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
 	batman_if->batman_adv_ptype.type = __constant_htons(ETH_P_BATMAN);
 	batman_if->batman_adv_ptype.func = batman_skb_recv;
 	batman_if->batman_adv_ptype.dev = batman_if->net_dev;
-	kref_get(&batman_if->refcount);
 	dev_add_pack(&batman_if->batman_adv_ptype);
 
 	atomic_set(&batman_if->seqno, 1);
@@ -371,6 +369,7 @@ out:
 	return 0;
 
 err:
+	hardif_free_ref(batman_if);
 	return -ENOMEM;
 }
 
@@ -387,7 +386,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
 	bat_info(batman_if->soft_iface, "Removing interface: %s\n",
 		 batman_if->net_dev->name);
 	dev_remove_pack(&batman_if->batman_adv_ptype);
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 
 	bat_priv->num_ifaces--;
 	orig_hash_del_if(batman_if, bat_priv->num_ifaces);
@@ -399,7 +398,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
 		set_primary_if(bat_priv, new_if);
 
 		if (new_if)
-			kref_put(&new_if->refcount, hardif_free_ref);
+			hardif_free_ref(new_if);
 	}
 
 	kfree(batman_if->packet_buff);
@@ -445,7 +444,8 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
 	batman_if->soft_iface = NULL;
 	batman_if->if_status = IF_NOT_IN_USE;
 	INIT_LIST_HEAD(&batman_if->list);
-	kref_init(&batman_if->refcount);
+	/* extra reference for return */
+	atomic_set(&batman_if->refcount, 2);
 
 	check_known_mac_addr(batman_if->net_dev);
 
@@ -453,8 +453,6 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
 	list_add_tail_rcu(&batman_if->list, &if_list);
 	spin_unlock(&if_list_lock);
 
-	/* extra reference for return */
-	kref_get(&batman_if->refcount);
 	return batman_if;
 
 free_if:
@@ -476,7 +474,7 @@ static void hardif_remove_interface(struct batman_if *batman_if)
 
 	batman_if->if_status = IF_TO_BE_REMOVED;
 	sysfs_del_hardif(&batman_if->hardif_obj);
-	call_rcu(&batman_if->rcu, hardif_free_rcu);
+	hardif_free_ref(batman_if);
 }
 
 void hardif_remove_interfaces(void)
@@ -548,7 +546,7 @@ static int hard_if_event(struct notifier_block *this,
 	};
 
 hardif_put:
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 out:
 	return NOTIFY_DONE;
 }
diff --git a/batman-adv/hard-interface.h b/batman-adv/hard-interface.h
index ad19543..e488b90 100644
--- a/batman-adv/hard-interface.h
+++ b/batman-adv/hard-interface.h
@@ -37,13 +37,12 @@ void hardif_disable_interface(struct batman_if *batman_if);
 void hardif_remove_interfaces(void);
 int hardif_min_mtu(struct net_device *soft_iface);
 void update_min_mtu(struct net_device *soft_iface);
+void hardif_free_rcu(struct rcu_head *rcu);
 
-static inline void hardif_free_ref(struct kref *refcount)
+static inline void hardif_free_ref(struct batman_if *batman_if)
 {
-	struct batman_if *batman_if;
-
-	batman_if = container_of(refcount, struct batman_if, refcount);
-	kfree(batman_if);
+	if (atomic_dec_and_test(&batman_if->refcount))
+		call_rcu(&batman_if->rcu, hardif_free_rcu);
 }
 
 #endif /* _NET_BATMAN_ADV_HARD_INTERFACE_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 8df6d9d..317ede8 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -43,7 +43,7 @@ struct batman_if {
 	unsigned char *packet_buff;
 	int packet_len;
 	struct kobject *hardif_obj;
-	struct kref refcount;
+	atomic_t refcount;
 	struct packet_type batman_adv_ptype;
 	struct net_device *soft_iface;
 	struct rcu_head rcu;
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node

[Sven Eckelmann]

<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 batman-adv/icmp_socket.c |    7 ++--
 batman-adv/originator.c  |   26 +++++----------
 batman-adv/originator.h  |    3 +-
 batman-adv/routing.c     |   79 ++++++++++++++++++++++++++++-----------------
 batman-adv/types.h       |    3 +-
 batman-adv/unicast.c     |    2 +-
 batman-adv/vis.c         |    7 ++--
 7 files changed, 68 insertions(+), 59 deletions(-)

diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index 10a969c..046f976 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -230,10 +230,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	if (!neigh_node->if_incoming)
@@ -261,7 +262,7 @@ free_skb:
 	kfree_skb(skb);
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
 	return 0;
 }
 
-void neigh_node_free_ref(struct kref *refcount)
-{
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(refcount, struct neigh_node, refcount);
-	kfree(neigh_node);
-}
-
 static void neigh_node_free_rcu(struct rcu_head *rcu)
 {
 	struct neigh_node *neigh_node;
 
 	neigh_node = container_of(rcu, struct neigh_node, rcu);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	kfree(neigh_node);
 }
 
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
 {
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	if (atomic_dec_and_test(&neigh_node->refcount))
+		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
 }
 
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
 	memcpy(neigh_node->addr, neigh, ETH_ALEN);
 	neigh_node->orig_node = orig_neigh_node;
 	neigh_node->if_incoming = if_incoming;
-	kref_init(&neigh_node->refcount);
+	atomic_set(&neigh_node->refcount, 1);
 
 	spin_lock_bh(&orig_node->neigh_list_lock);
 	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
 	list_for_each_entry_safe(neigh_node, tmp_neigh_node,
 				 &orig_node->bond_list, bonding_list) {
 		list_del_rcu(&neigh_node->bonding_list);
-		call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	/* for all neighbors towards this originator ... */
 	hlist_for_each_entry_safe(neigh_node, node, node_tmp,
 				  &orig_node->neigh_list, list) {
 		hlist_del_rcu(&neigh_node->list);
-		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
 
 			hlist_del_rcu(&neigh_node->list);
 			bonding_candidate_del(orig_node, neigh_node);
-			call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+			neigh_node_free_ref(neigh_node);
 		} else {
 			if ((!*best_neigh_node) ||
 			    (neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
 void originator_free(struct bat_priv *bat_priv);
 void purge_orig_ref(struct bat_priv *bat_priv);
 void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
 struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
 				   struct orig_node *orig_neigh_node,
 				   uint8_t *neigh,
 				   struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
 int orig_seq_print_text(struct seq_file *seq, void *offset);
 int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
 int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index 2861f18..29998cf 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -118,12 +118,12 @@ static void update_route(struct bat_priv *bat_priv,
 			orig_node->router->addr);
 	}
 
-	if (neigh_node)
-		kref_get(&neigh_node->refcount);
+	if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+		neigh_node = NULL;
 	neigh_node_tmp = orig_node->router;
 	orig_node->router = neigh_node;
 	if (neigh_node_tmp)
-		kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node_tmp);
 }
 
 
@@ -172,10 +172,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 						     orig_neigh_node->orig,
 						     if_incoming);
 		/* create_neighbor failed, return 0 */
-		if (!neigh_node)
+		if (!neigh_node ||
+		    !atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
 			goto unlock;
+		}
 
-		kref_get(&neigh_node->refcount);
 		rcu_read_unlock();
 
 		neigh_node->last_valid = jiffies;
@@ -197,10 +199,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 						     orig_neigh_node->orig,
 						     if_incoming);
 		/* create_neighbor failed, return 0 */
-		if (!neigh_node)
+		if (!neigh_node ||
+		    !atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
 			goto unlock;
+		}
 
-		kref_get(&neigh_node->refcount);
 		rcu_read_unlock();
 	}
 
@@ -262,7 +266,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	return ret;
 }
 
@@ -275,7 +279,7 @@ void bonding_candidate_del(struct orig_node *orig_node,
 		goto out;
 
 	list_del_rcu(&neigh_node->bonding_list);
-	call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+	neigh_node_free_ref(neigh_node);
 	INIT_LIST_HEAD(&neigh_node->bonding_list);
 	atomic_dec(&orig_node->bond_candidates);
 
@@ -337,8 +341,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
 	if (!list_empty(&neigh_node->bonding_list))
 		goto out;
 
+	if (!atomic_inc_not_zero(&neigh_node->refcount))
+		goto out;
+
 	list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
-	kref_get(&neigh_node->refcount);
 	atomic_inc(&orig_node->bond_candidates);
 	goto out;
 
@@ -382,7 +388,10 @@ static void update_orig(struct bat_priv *bat_priv,
 	hlist_for_each_entry_rcu(tmp_neigh_node, node,
 				 &orig_node->neigh_list, list) {
 		if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
-		    (tmp_neigh_node->if_incoming == if_incoming)) {
+		    (tmp_neigh_node->if_incoming == if_incoming) &&
+		     atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+			if (neigh_node)
+				neigh_node_free_ref(neigh_node);
 			neigh_node = tmp_neigh_node;
 			continue;
 		}
@@ -407,13 +416,15 @@ static void update_orig(struct bat_priv *bat_priv,
 					     ethhdr->h_source, if_incoming);
 
 		kref_put(&orig_tmp->refcount, orig_node_free_ref);
-		if (!neigh_node)
+		if (!neigh_node ||
+		    !atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
 			goto unlock;
+		}
 	} else
 		bat_dbg(DBG_BATMAN, bat_priv,
 			"Updating existing last-hop neighbor of originator\n");
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	orig_node->flags = batman_packet->flags;
@@ -490,7 +501,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 }
 
 /* checks whether the host restarted and is in the protection time.
@@ -891,10 +902,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -917,7 +929,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -955,10 +967,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -981,7 +994,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1051,10 +1064,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -1075,7 +1089,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1108,7 +1122,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	/* select default router to output */
 	router = orig_node->router;
 	router_orig = orig_node->router->orig_node;
-	if (!router_orig) {
+	if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
 		rcu_read_unlock();
 		return NULL;
 	}
@@ -1146,6 +1160,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	 * is is not on the interface where the packet came
 	 * in. */
 
+	neigh_node_free_ref(router);
 	first_candidate = NULL;
 	router = NULL;
 
@@ -1158,14 +1173,16 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 			if (!first_candidate)
 				first_candidate = tmp_neigh_node;
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if) {
+			if (tmp_neigh_node->if_incoming != recv_if &&
+			    atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
 				router = tmp_neigh_node;
 				break;
 			}
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 
 		/* selected should point to the next element
@@ -1174,6 +1191,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 		/* this is a list_move(), which unfortunately
 		 * does not exist as rcu version */
 		list_del_rcu(&primary_orig_node->bond_list);
+		/* BUG: router can be NULL */
 		list_add_rcu(&primary_orig_node->bond_list,
 				&router->bonding_list);
 		spin_unlock_bh(&primary_orig_node->neigh_list_lock);
@@ -1188,7 +1206,8 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 				first_candidate = tmp_neigh_node;
 
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if)
+			if (tmp_neigh_node->if_incoming != recv_if &&
+			    atomic_inc_not_zero(&tmp_neigh_node->refcount))
 				/* if we don't have a router yet
 				 * or this one is better, choose it. */
 				if ((!router) ||
@@ -1198,11 +1217,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 	}
 return_router:
-	kref_get(&router->refcount);
 	rcu_read_unlock();
 	return router;
 }
@@ -1315,7 +1334,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
 	struct list_head bonding_list;
 	unsigned long last_valid;
 	unsigned long real_bits[NUM_WORDS];
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
-	struct rcu_head rcu_bond;
 	struct orig_node *orig_node;
 	struct batman_if *if_incoming;
 };
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
 
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..fe32de3 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -773,10 +773,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +791,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return;
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCHv2 4/4] batman-adv: Correct rcu refcounting for neigh_node

[Sven Eckelmann]

<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Removed the "BUG" after a small conversation with d0tslash

 batman-adv/icmp_socket.c |    7 ++--
 batman-adv/originator.c  |   26 ++++----------
 batman-adv/originator.h  |    3 +-
 batman-adv/routing.c     |   83 +++++++++++++++++++++++++++++----------------
 batman-adv/types.h       |    3 +-
 batman-adv/unicast.c     |    2 +-
 batman-adv/vis.c         |    7 ++--
 7 files changed, 72 insertions(+), 59 deletions(-)

diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index 10a969c..046f976 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -230,10 +230,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	if (!neigh_node->if_incoming)
@@ -261,7 +262,7 @@ free_skb:
 	kfree_skb(skb);
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
 	return 0;
 }
 
-void neigh_node_free_ref(struct kref *refcount)
-{
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(refcount, struct neigh_node, refcount);
-	kfree(neigh_node);
-}
-
 static void neigh_node_free_rcu(struct rcu_head *rcu)
 {
 	struct neigh_node *neigh_node;
 
 	neigh_node = container_of(rcu, struct neigh_node, rcu);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	kfree(neigh_node);
 }
 
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
 {
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	if (atomic_dec_and_test(&neigh_node->refcount))
+		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
 }
 
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
 	memcpy(neigh_node->addr, neigh, ETH_ALEN);
 	neigh_node->orig_node = orig_neigh_node;
 	neigh_node->if_incoming = if_incoming;
-	kref_init(&neigh_node->refcount);
+	atomic_set(&neigh_node->refcount, 1);
 
 	spin_lock_bh(&orig_node->neigh_list_lock);
 	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
 	list_for_each_entry_safe(neigh_node, tmp_neigh_node,
 				 &orig_node->bond_list, bonding_list) {
 		list_del_rcu(&neigh_node->bonding_list);
-		call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	/* for all neighbors towards this originator ... */
 	hlist_for_each_entry_safe(neigh_node, node, node_tmp,
 				  &orig_node->neigh_list, list) {
 		hlist_del_rcu(&neigh_node->list);
-		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
 
 			hlist_del_rcu(&neigh_node->list);
 			bonding_candidate_del(orig_node, neigh_node);
-			call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+			neigh_node_free_ref(neigh_node);
 		} else {
 			if ((!*best_neigh_node) ||
 			    (neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
 void originator_free(struct bat_priv *bat_priv);
 void purge_orig_ref(struct bat_priv *bat_priv);
 void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
 struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
 				   struct orig_node *orig_neigh_node,
 				   uint8_t *neigh,
 				   struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
 int orig_seq_print_text(struct seq_file *seq, void *offset);
 int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
 int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index 2861f18..94e9c1c 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -118,12 +118,12 @@ static void update_route(struct bat_priv *bat_priv,
 			orig_node->router->addr);
 	}
 
-	if (neigh_node)
-		kref_get(&neigh_node->refcount);
+	if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+		neigh_node = NULL;
 	neigh_node_tmp = orig_node->router;
 	orig_node->router = neigh_node;
 	if (neigh_node_tmp)
-		kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node_tmp);
 }
 
 
@@ -172,10 +172,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 						     orig_neigh_node->orig,
 						     if_incoming);
 		/* create_neighbor failed, return 0 */
-		if (!neigh_node)
+		if (!neigh_node ||
+		    !atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
 			goto unlock;
+		}
 
-		kref_get(&neigh_node->refcount);
 		rcu_read_unlock();
 
 		neigh_node->last_valid = jiffies;
@@ -197,10 +199,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 						     orig_neigh_node->orig,
 						     if_incoming);
 		/* create_neighbor failed, return 0 */
-		if (!neigh_node)
+		if (!neigh_node ||
+		    !atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
 			goto unlock;
+		}
 
-		kref_get(&neigh_node->refcount);
 		rcu_read_unlock();
 	}
 
@@ -262,7 +266,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	return ret;
 }
 
@@ -275,7 +279,7 @@ void bonding_candidate_del(struct orig_node *orig_node,
 		goto out;
 
 	list_del_rcu(&neigh_node->bonding_list);
-	call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+	neigh_node_free_ref(neigh_node);
 	INIT_LIST_HEAD(&neigh_node->bonding_list);
 	atomic_dec(&orig_node->bond_candidates);
 
@@ -337,8 +341,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
 	if (!list_empty(&neigh_node->bonding_list))
 		goto out;
 
+	if (!atomic_inc_not_zero(&neigh_node->refcount))
+		goto out;
+
 	list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
-	kref_get(&neigh_node->refcount);
 	atomic_inc(&orig_node->bond_candidates);
 	goto out;
 
@@ -382,7 +388,10 @@ static void update_orig(struct bat_priv *bat_priv,
 	hlist_for_each_entry_rcu(tmp_neigh_node, node,
 				 &orig_node->neigh_list, list) {
 		if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
-		    (tmp_neigh_node->if_incoming == if_incoming)) {
+		    (tmp_neigh_node->if_incoming == if_incoming) &&
+		     atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+			if (neigh_node)
+				neigh_node_free_ref(neigh_node);
 			neigh_node = tmp_neigh_node;
 			continue;
 		}
@@ -407,13 +416,15 @@ static void update_orig(struct bat_priv *bat_priv,
 					     ethhdr->h_source, if_incoming);
 
 		kref_put(&orig_tmp->refcount, orig_node_free_ref);
-		if (!neigh_node)
+		if (!neigh_node ||
+		    !atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
 			goto unlock;
+		}
 	} else
 		bat_dbg(DBG_BATMAN, bat_priv,
 			"Updating existing last-hop neighbor of originator\n");
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	orig_node->flags = batman_packet->flags;
@@ -490,7 +501,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 }
 
 /* checks whether the host restarted and is in the protection time.
@@ -891,10 +902,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -917,7 +929,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -955,10 +967,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -981,7 +994,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1051,10 +1064,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -1075,7 +1089,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1108,7 +1122,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	/* select default router to output */
 	router = orig_node->router;
 	router_orig = orig_node->router->orig_node;
-	if (!router_orig) {
+	if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
 		rcu_read_unlock();
 		return NULL;
 	}
@@ -1146,6 +1160,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	 * is is not on the interface where the packet came
 	 * in. */
 
+	neigh_node_free_ref(router);
 	first_candidate = NULL;
 	router = NULL;
 
@@ -1158,16 +1173,23 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 			if (!first_candidate)
 				first_candidate = tmp_neigh_node;
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if) {
+			if (tmp_neigh_node->if_incoming != recv_if &&
+			    atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
 				router = tmp_neigh_node;
 				break;
 			}
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 
+		if (!router) {
+			rcu_read_unlock();
+			return NULL;
+		}
+
 		/* selected should point to the next element
 		 * after the current router */
 		spin_lock_bh(&primary_orig_node->neigh_list_lock);
@@ -1188,7 +1210,8 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 				first_candidate = tmp_neigh_node;
 
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if)
+			if (tmp_neigh_node->if_incoming != recv_if &&
+			    atomic_inc_not_zero(&tmp_neigh_node->refcount))
 				/* if we don't have a router yet
 				 * or this one is better, choose it. */
 				if ((!router) ||
@@ -1198,11 +1221,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 	}
 return_router:
-	kref_get(&router->refcount);
 	rcu_read_unlock();
 	return router;
 }
@@ -1315,7 +1338,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
 	struct list_head bonding_list;
 	unsigned long last_valid;
 	unsigned long real_bits[NUM_WORDS];
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
-	struct rcu_head rcu_bond;
 	struct orig_node *orig_node;
 	struct batman_if *if_incoming;
 };
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
 
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..fe32de3 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -773,10 +773,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +791,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return;
-- 
1.7.2.3

Re: [B.A.T.M.A.N.] Reordered rcu refcounting

[Marek Lindner]

Hi,

> I told Linus and Marek that the implementation of some datastructures using
> reference counting and RCUs must be redesigned. The problem is that
> call_rcu should only be used when the reference count is zero because
> multiple call_rcu cannot be initiated at the same time. The next problem
> is that we may try to access an element with the refcount 0 when we try to
> traverse a rcu protected list. This problem cannot be solved by kref -
> thus we have to use atomic_t again to not increase the reference count
> when it was already zero.

I reviewed the posted patches, filled in the missing parts and fixed some bugs 
along the way. Feel free to comment.

@Linus: I believe it would be a good idea if your gateway patch was based on 
this patchset.

Cheers,
Marek

[B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node

[Marek Lindner]

From: Sven Eckelmann <sven@narfation.org>

It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 batman-adv/gateway_client.c |   37 ++++++++++++++++---------------------
 batman-adv/types.h          |    2 +-
 2 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 429a013..517e001 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -28,20 +28,18 @@
 #include <linux/udp.h>
 #include <linux/if_vlan.h>
 
-static void gw_node_free_ref(struct kref *refcount)
+static void gw_node_free_rcu(struct rcu_head *rcu)
 {
 	struct gw_node *gw_node;
 
-	gw_node = container_of(refcount, struct gw_node, refcount);
+	gw_node = container_of(rcu, struct gw_node, rcu);
 	kfree(gw_node);
 }
 
-static void gw_node_free_rcu(struct rcu_head *rcu)
+static void gw_node_free_ref(struct gw_node *gw_node)
 {
-	struct gw_node *gw_node;
-
-	gw_node = container_of(rcu, struct gw_node, rcu);
-	kref_put(&gw_node->refcount, gw_node_free_ref);
+	if (atomic_dec_and_test(&gw_node->refcount))
+		call_rcu(&gw_node->rcu, gw_node_free_rcu);
 }
 
 void *gw_get_selected(struct bat_priv *bat_priv)
@@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv)
 	bat_priv->curr_gw = NULL;
 
 	if (gw_node)
-		kref_put(&gw_node->refcount, gw_node_free_ref);
+		gw_node_free_ref(gw_node);
 }
 
-static struct gw_node *gw_select(struct bat_priv *bat_priv,
-			  struct gw_node *new_gw_node)
+static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
 {
 	struct gw_node *curr_gw_node = bat_priv->curr_gw;
 
-	if (new_gw_node)
-		kref_get(&new_gw_node->refcount);
+	if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+		new_gw_node = NULL;
 
 	bat_priv->curr_gw = new_gw_node;
-	return curr_gw_node;
+
+	if (curr_gw_node)
+		gw_node_free_ref(curr_gw_node);
 }
 
 void gw_election(struct bat_priv *bat_priv)
 {
 	struct hlist_node *node;
-	struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+	struct gw_node *gw_node, *curr_gw_tmp = NULL;
 	uint8_t max_tq = 0;
 	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
 	int down, up;
@@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv)
 				curr_gw_tmp->orig_node->gw_flags,
 				curr_gw_tmp->orig_node->router->tq_avg);
 
-		old_gw_node = gw_select(bat_priv, curr_gw_tmp);
+		gw_select(bat_priv, curr_gw_tmp);
 	}
 
 	rcu_read_unlock();
-
-	/* the kfree() has to be outside of the rcu lock */
-	if (old_gw_node)
-		kref_put(&old_gw_node->refcount, gw_node_free_ref);
 }
 
 void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
@@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
 	memset(gw_node, 0, sizeof(struct gw_node));
 	INIT_HLIST_NODE(&gw_node->list);
 	gw_node->orig_node = orig_node;
-	kref_init(&gw_node->refcount);
+	atomic_set(&gw_node->refcount, 1);
 
 	spin_lock_bh(&bat_priv->gw_list_lock);
 	hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
 			gw_deselect(bat_priv);
 
 		hlist_del_rcu(&gw_node->list);
-		call_rcu(&gw_node->rcu, gw_node_free_rcu);
+		gw_node_free_ref(gw_node);
 	}
 
 
diff --git a/batman-adv/types.h b/batman-adv/types.h
index e4a0462..ca5f20a 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -100,7 +100,7 @@ struct gw_node {
 	struct hlist_node list;
 	struct orig_node *orig_node;
 	unsigned long deleted;
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
 };
 
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh

[Marek Lindner]

From: Sven Eckelmann <sven@narfation.org>

It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 batman-adv/soft-interface.c |   31 +++++++++++++++----------------
 batman-adv/types.h          |    2 +-
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c
index 145e0f7..83dcccd 100644
--- a/batman-adv/soft-interface.c
+++ b/batman-adv/soft-interface.c
@@ -79,20 +79,18 @@ int my_skb_head_push(struct sk_buff *skb, unsigned int len)
 	return 0;
 }
 
-static void softif_neigh_free_ref(struct kref *refcount)
+static void softif_neigh_free_rcu(struct rcu_head *rcu)
 {
 	struct softif_neigh *softif_neigh;
 
-	softif_neigh = container_of(refcount, struct softif_neigh, refcount);
+	softif_neigh = container_of(rcu, struct softif_neigh, rcu);
 	kfree(softif_neigh);
 }
 
-static void softif_neigh_free_rcu(struct rcu_head *rcu)
+static void softif_neigh_free_ref(struct softif_neigh *softif_neigh)
 {
-	struct softif_neigh *softif_neigh;
-
-	softif_neigh = container_of(rcu, struct softif_neigh, rcu);
-	kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+	if (atomic_dec_and_test(&softif_neigh->refcount))
+		call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
 }
 
 void softif_neigh_purge(struct bat_priv *bat_priv)
@@ -119,11 +117,10 @@ void softif_neigh_purge(struct bat_priv *bat_priv)
 				 softif_neigh->addr, softif_neigh->vid);
 			softif_neigh_tmp = bat_priv->softif_neigh;
 			bat_priv->softif_neigh = NULL;
-			kref_put(&softif_neigh_tmp->refcount,
-				 softif_neigh_free_ref);
+			softif_neigh_free_ref(softif_neigh_tmp);
 		}
 
-		call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
+		softif_neigh_free_ref(softif_neigh);
 	}
 
 	spin_unlock_bh(&bat_priv->softif_neigh_lock);
@@ -144,8 +141,11 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
 		if (softif_neigh->vid != vid)
 			continue;
 
+		if (!atomic_inc_not_zero(&softif_neigh->refcount))
+			continue;
+
 		softif_neigh->last_seen = jiffies;
-		goto found;
+		goto out;
 	}
 
 	softif_neigh = kzalloc(sizeof(struct softif_neigh), GFP_ATOMIC);
@@ -155,15 +155,14 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
 	memcpy(softif_neigh->addr, addr, ETH_ALEN);
 	softif_neigh->vid = vid;
 	softif_neigh->last_seen = jiffies;
-	kref_init(&softif_neigh->refcount);
+	/* initialize with 2 - caller decrements counter by one */
+	atomic_set(&softif_neigh->refcount, 2);
 
 	INIT_HLIST_NODE(&softif_neigh->list);
 	spin_lock_bh(&bat_priv->softif_neigh_lock);
 	hlist_add_head_rcu(&softif_neigh->list, &bat_priv->softif_neigh_list);
 	spin_unlock_bh(&bat_priv->softif_neigh_lock);
 
-found:
-	kref_get(&softif_neigh->refcount);
 out:
 	rcu_read_unlock();
 	return softif_neigh;
@@ -267,7 +266,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
 			 softif_neigh->addr, softif_neigh->vid);
 		softif_neigh_tmp = bat_priv->softif_neigh;
 		bat_priv->softif_neigh = softif_neigh;
-		kref_put(&softif_neigh_tmp->refcount, softif_neigh_free_ref);
+		softif_neigh_free_ref(softif_neigh_tmp);
 		/* we need to hold the additional reference */
 		goto err;
 	}
@@ -285,7 +284,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
 	}
 
 out:
-	kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+	softif_neigh_free_ref(softif_neigh);
 err:
 	kfree_skb(skb);
 	return;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index ca5f20a..8df6d9d 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -270,7 +270,7 @@ struct softif_neigh {
 	uint8_t addr[ETH_ALEN];
 	unsigned long last_seen;
 	short vid;
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
 };
 
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if

[Marek Lindner]

From: Sven Eckelmann <sven@narfation.org>

It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 batman-adv/bat_sysfs.c      |   20 +++++++++-----------
 batman-adv/hard-interface.c |   40 +++++++++++++++++++---------------------
 batman-adv/hard-interface.h |    9 ++++-----
 batman-adv/types.h          |    2 +-
 4 files changed, 33 insertions(+), 38 deletions(-)

diff --git a/batman-adv/bat_sysfs.c b/batman-adv/bat_sysfs.c
index f7b93a0..93ae20a 100644
--- a/batman-adv/bat_sysfs.c
+++ b/batman-adv/bat_sysfs.c
@@ -450,7 +450,7 @@ static ssize_t show_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	length = sprintf(buff, "%s\n", batman_if->if_status == IF_NOT_IN_USE ?
 			 "none" : batman_if->soft_iface->name);
 
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 
 	return length;
 }
@@ -461,7 +461,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	struct net_device *net_dev = kobj_to_netdev(kobj);
 	struct batman_if *batman_if = get_batman_if_by_netdev(net_dev);
 	int status_tmp = -1;
-	int ret;
+	int ret = count;
 
 	if (!batman_if)
 		return count;
@@ -472,7 +472,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	if (strlen(buff) >= IFNAMSIZ) {
 		pr_err("Invalid parameter for 'mesh_iface' setting received: "
 		       "interface name too long '%s'\n", buff);
-		kref_put(&batman_if->refcount, hardif_free_ref);
+		hardif_free_ref(batman_if);
 		return -EINVAL;
 	}
 
@@ -482,17 +482,14 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 		status_tmp = IF_I_WANT_YOU;
 
 	if ((batman_if->if_status == status_tmp) || ((batman_if->soft_iface) &&
-	    (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) {
-		kref_put(&batman_if->refcount, hardif_free_ref);
-		return count;
-	}
+	    (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0)))
+		goto out;
 
 	if (status_tmp == IF_NOT_IN_USE) {
 		rtnl_lock();
 		hardif_disable_interface(batman_if);
 		rtnl_unlock();
-		kref_put(&batman_if->refcount, hardif_free_ref);
-		return count;
+		goto out;
 	}
 
 	/* if the interface already is in use */
@@ -503,8 +500,9 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	}
 
 	ret = hardif_enable_interface(batman_if, buff);
-	kref_put(&batman_if->refcount, hardif_free_ref);
 
+out:
+	hardif_free_ref(batman_if);
 	return ret;
 }
 
@@ -537,7 +535,7 @@ static ssize_t show_iface_status(struct kobject *kobj, struct attribute *attr,
 		break;
 	}
 
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 
 	return length;
 }
diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index e2b001a..8982485 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -40,13 +40,13 @@ static int batman_skb_recv(struct sk_buff *skb,
 			   struct packet_type *ptype,
 			   struct net_device *orig_dev);
 
-static void hardif_free_rcu(struct rcu_head *rcu)
+void hardif_free_rcu(struct rcu_head *rcu)
 {
 	struct batman_if *batman_if;
 
 	batman_if = container_of(rcu, struct batman_if, rcu);
 	dev_put(batman_if->net_dev);
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	kfree(batman_if);
 }
 
 struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
@@ -55,16 +55,14 @@ struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(batman_if, &if_list, list) {
-		if (batman_if->net_dev == net_dev)
+		if (batman_if->net_dev == net_dev &&
+		    atomic_inc_not_zero(&batman_if->refcount))
 			goto out;
 	}
 
 	batman_if = NULL;
 
 out:
-	if (batman_if)
-		kref_get(&batman_if->refcount);
-
 	rcu_read_unlock();
 	return batman_if;
 }
@@ -105,16 +103,14 @@ static struct batman_if *get_active_batman_if(struct net_device *soft_iface)
 		if (batman_if->soft_iface != soft_iface)
 			continue;
 
-		if (batman_if->if_status == IF_ACTIVE)
+		if (batman_if->if_status == IF_ACTIVE &&
+		    atomic_inc_not_zero(&batman_if->refcount))
 			goto out;
 	}
 
 	batman_if = NULL;
 
 out:
-	if (batman_if)
-		kref_get(&batman_if->refcount);
-
 	rcu_read_unlock();
 	return batman_if;
 }
@@ -137,14 +133,14 @@ static void set_primary_if(struct bat_priv *bat_priv,
 	struct batman_packet *batman_packet;
 	struct batman_if *old_if;
 
-	if (batman_if)
-		kref_get(&batman_if->refcount);
+	if (batman_if && !atomic_inc_not_zero(&batman_if->refcount))
+		batman_if = NULL;
 
 	old_if = bat_priv->primary_if;
 	bat_priv->primary_if = batman_if;
 
 	if (old_if)
-		kref_put(&old_if->refcount, hardif_free_ref);
+		hardif_free_ref(old_if);
 
 	if (!bat_priv->primary_if)
 		return;
@@ -290,6 +286,9 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
 	if (batman_if->if_status != IF_NOT_IN_USE)
 		goto out;
 
+	if (!atomic_inc_not_zero(&batman_if->refcount))
+		goto out;
+
 	batman_if->soft_iface = dev_get_by_name(&init_net, iface_name);
 
 	if (!batman_if->soft_iface) {
@@ -328,7 +327,6 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
 	batman_if->batman_adv_ptype.type = __constant_htons(ETH_P_BATMAN);
 	batman_if->batman_adv_ptype.func = batman_skb_recv;
 	batman_if->batman_adv_ptype.dev = batman_if->net_dev;
-	kref_get(&batman_if->refcount);
 	dev_add_pack(&batman_if->batman_adv_ptype);
 
 	atomic_set(&batman_if->seqno, 1);
@@ -371,6 +369,7 @@ out:
 	return 0;
 
 err:
+	hardif_free_ref(batman_if);
 	return -ENOMEM;
 }
 
@@ -387,7 +386,6 @@ void hardif_disable_interface(struct batman_if *batman_if)
 	bat_info(batman_if->soft_iface, "Removing interface: %s\n",
 		 batman_if->net_dev->name);
 	dev_remove_pack(&batman_if->batman_adv_ptype);
-	kref_put(&batman_if->refcount, hardif_free_ref);
 
 	bat_priv->num_ifaces--;
 	orig_hash_del_if(batman_if, bat_priv->num_ifaces);
@@ -399,7 +397,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
 		set_primary_if(bat_priv, new_if);
 
 		if (new_if)
-			kref_put(&new_if->refcount, hardif_free_ref);
+			hardif_free_ref(new_if);
 	}
 
 	kfree(batman_if->packet_buff);
@@ -416,6 +414,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
 		softif_destroy(batman_if->soft_iface);
 
 	batman_if->soft_iface = NULL;
+	hardif_free_ref(batman_if);
 }
 
 static struct batman_if *hardif_add_interface(struct net_device *net_dev)
@@ -445,7 +444,8 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
 	batman_if->soft_iface = NULL;
 	batman_if->if_status = IF_NOT_IN_USE;
 	INIT_LIST_HEAD(&batman_if->list);
-	kref_init(&batman_if->refcount);
+	/* extra reference for return */
+	atomic_set(&batman_if->refcount, 2);
 
 	check_known_mac_addr(batman_if->net_dev);
 
@@ -453,8 +453,6 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
 	list_add_tail_rcu(&batman_if->list, &if_list);
 	spin_unlock(&if_list_lock);
 
-	/* extra reference for return */
-	kref_get(&batman_if->refcount);
 	return batman_if;
 
 free_if:
@@ -476,7 +474,7 @@ static void hardif_remove_interface(struct batman_if *batman_if)
 
 	batman_if->if_status = IF_TO_BE_REMOVED;
 	sysfs_del_hardif(&batman_if->hardif_obj);
-	call_rcu(&batman_if->rcu, hardif_free_rcu);
+	hardif_free_ref(batman_if);
 }
 
 void hardif_remove_interfaces(void)
@@ -548,7 +546,7 @@ static int hard_if_event(struct notifier_block *this,
 	};
 
 hardif_put:
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 out:
 	return NOTIFY_DONE;
 }
diff --git a/batman-adv/hard-interface.h b/batman-adv/hard-interface.h
index ad19543..e488b90 100644
--- a/batman-adv/hard-interface.h
+++ b/batman-adv/hard-interface.h
@@ -37,13 +37,12 @@ void hardif_disable_interface(struct batman_if *batman_if);
 void hardif_remove_interfaces(void);
 int hardif_min_mtu(struct net_device *soft_iface);
 void update_min_mtu(struct net_device *soft_iface);
+void hardif_free_rcu(struct rcu_head *rcu);
 
-static inline void hardif_free_ref(struct kref *refcount)
+static inline void hardif_free_ref(struct batman_if *batman_if)
 {
-	struct batman_if *batman_if;
-
-	batman_if = container_of(refcount, struct batman_if, refcount);
-	kfree(batman_if);
+	if (atomic_dec_and_test(&batman_if->refcount))
+		call_rcu(&batman_if->rcu, hardif_free_rcu);
 }
 
 #endif /* _NET_BATMAN_ADV_HARD_INTERFACE_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 8df6d9d..317ede8 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -43,7 +43,7 @@ struct batman_if {
 	unsigned char *packet_buff;
 	int packet_len;
 	struct kobject *hardif_obj;
-	struct kref refcount;
+	atomic_t refcount;
 	struct packet_type batman_adv_ptype;
 	struct net_device *soft_iface;
 	struct rcu_head rcu;
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node

[Marek Lindner]

From: Sven Eckelmann <sven@narfation.org>

It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 batman-adv/icmp_socket.c |    7 ++-
 batman-adv/originator.c  |   26 ++++---------
 batman-adv/originator.h  |    3 +-
 batman-adv/routing.c     |   93 ++++++++++++++++++++++++++++++++-------------
 batman-adv/types.h       |    3 +-
 batman-adv/unicast.c     |    2 +-
 batman-adv/vis.c         |    8 +++-
 7 files changed, 87 insertions(+), 55 deletions(-)

diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index 10a969c..046f976 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -230,10 +230,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	if (!neigh_node->if_incoming)
@@ -261,7 +262,7 @@ free_skb:
 	kfree_skb(skb);
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
 	return 0;
 }
 
-void neigh_node_free_ref(struct kref *refcount)
-{
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(refcount, struct neigh_node, refcount);
-	kfree(neigh_node);
-}
-
 static void neigh_node_free_rcu(struct rcu_head *rcu)
 {
 	struct neigh_node *neigh_node;
 
 	neigh_node = container_of(rcu, struct neigh_node, rcu);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	kfree(neigh_node);
 }
 
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
 {
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	if (atomic_dec_and_test(&neigh_node->refcount))
+		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
 }
 
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
 	memcpy(neigh_node->addr, neigh, ETH_ALEN);
 	neigh_node->orig_node = orig_neigh_node;
 	neigh_node->if_incoming = if_incoming;
-	kref_init(&neigh_node->refcount);
+	atomic_set(&neigh_node->refcount, 1);
 
 	spin_lock_bh(&orig_node->neigh_list_lock);
 	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
 	list_for_each_entry_safe(neigh_node, tmp_neigh_node,
 				 &orig_node->bond_list, bonding_list) {
 		list_del_rcu(&neigh_node->bonding_list);
-		call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	/* for all neighbors towards this originator ... */
 	hlist_for_each_entry_safe(neigh_node, node, node_tmp,
 				  &orig_node->neigh_list, list) {
 		hlist_del_rcu(&neigh_node->list);
-		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
 
 			hlist_del_rcu(&neigh_node->list);
 			bonding_candidate_del(orig_node, neigh_node);
-			call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+			neigh_node_free_ref(neigh_node);
 		} else {
 			if ((!*best_neigh_node) ||
 			    (neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
 void originator_free(struct bat_priv *bat_priv);
 void purge_orig_ref(struct bat_priv *bat_priv);
 void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
 struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
 				   struct orig_node *orig_neigh_node,
 				   uint8_t *neigh,
 				   struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
 int orig_seq_print_text(struct seq_file *seq, void *offset);
 int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
 int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index 2861f18..0d6a629 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -118,12 +118,12 @@ static void update_route(struct bat_priv *bat_priv,
 			orig_node->router->addr);
 	}
 
-	if (neigh_node)
-		kref_get(&neigh_node->refcount);
+	if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+		neigh_node = NULL;
 	neigh_node_tmp = orig_node->router;
 	orig_node->router = neigh_node;
 	if (neigh_node_tmp)
-		kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node_tmp);
 }
 
 
@@ -175,7 +175,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 		if (!neigh_node)
 			goto unlock;
 
-		kref_get(&neigh_node->refcount);
+		if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
+			goto unlock;
+		}
+
 		rcu_read_unlock();
 
 		neigh_node->last_valid = jiffies;
@@ -200,7 +204,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 		if (!neigh_node)
 			goto unlock;
 
-		kref_get(&neigh_node->refcount);
+		if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
+			goto unlock;
+		}
+
 		rcu_read_unlock();
 	}
 
@@ -262,7 +270,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	return ret;
 }
 
@@ -275,8 +283,8 @@ void bonding_candidate_del(struct orig_node *orig_node,
 		goto out;
 
 	list_del_rcu(&neigh_node->bonding_list);
-	call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
 	INIT_LIST_HEAD(&neigh_node->bonding_list);
+	neigh_node_free_ref(neigh_node);
 	atomic_dec(&orig_node->bond_candidates);
 
 out:
@@ -337,8 +345,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
 	if (!list_empty(&neigh_node->bonding_list))
 		goto out;
 
+	if (!atomic_inc_not_zero(&neigh_node->refcount))
+		goto out;
+
 	list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
-	kref_get(&neigh_node->refcount);
 	atomic_inc(&orig_node->bond_candidates);
 	goto out;
 
@@ -382,7 +392,10 @@ static void update_orig(struct bat_priv *bat_priv,
 	hlist_for_each_entry_rcu(tmp_neigh_node, node,
 				 &orig_node->neigh_list, list) {
 		if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
-		    (tmp_neigh_node->if_incoming == if_incoming)) {
+		    (tmp_neigh_node->if_incoming == if_incoming) &&
+		     atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+			if (neigh_node)
+				neigh_node_free_ref(neigh_node);
 			neigh_node = tmp_neigh_node;
 			continue;
 		}
@@ -409,11 +422,15 @@ static void update_orig(struct bat_priv *bat_priv,
 		kref_put(&orig_tmp->refcount, orig_node_free_ref);
 		if (!neigh_node)
 			goto unlock;
+
+		if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
+			goto unlock;
+		}
 	} else
 		bat_dbg(DBG_BATMAN, bat_priv,
 			"Updating existing last-hop neighbor of originator\n");
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	orig_node->flags = batman_packet->flags;
@@ -490,7 +507,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 }
 
 /* checks whether the host restarted and is in the protection time.
@@ -894,7 +911,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -917,7 +938,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -958,7 +979,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -981,7 +1006,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1054,7 +1079,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -1075,7 +1104,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1108,12 +1137,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	/* select default router to output */
 	router = orig_node->router;
 	router_orig = orig_node->router->orig_node;
-	if (!router_orig) {
+	if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
 		rcu_read_unlock();
 		return NULL;
 	}
 
-
 	if ((!recv_if) && (!bonding_enabled))
 		goto return_router;
 
@@ -1146,6 +1174,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	 * is is not on the interface where the packet came
 	 * in. */
 
+	neigh_node_free_ref(router);
 	first_candidate = NULL;
 	router = NULL;
 
@@ -1158,16 +1187,23 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 			if (!first_candidate)
 				first_candidate = tmp_neigh_node;
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if) {
+			if (tmp_neigh_node->if_incoming != recv_if &&
+			    atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
 				router = tmp_neigh_node;
 				break;
 			}
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 
+		if (!router) {
+			rcu_read_unlock();
+			return NULL;
+		}
+
 		/* selected should point to the next element
 		 * after the current router */
 		spin_lock_bh(&primary_orig_node->neigh_list_lock);
@@ -1188,21 +1224,24 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 				first_candidate = tmp_neigh_node;
 
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if)
+			if (tmp_neigh_node->if_incoming != recv_if &&
+			    atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
 				/* if we don't have a router yet
 				 * or this one is better, choose it. */
 				if ((!router) ||
-				(tmp_neigh_node->tq_avg > router->tq_avg)) {
+				    (tmp_neigh_node->tq_avg > router->tq_avg))
 					router = tmp_neigh_node;
-				}
+				else
+					neigh_node_free_ref(tmp_neigh_node);
+			}
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 	}
 return_router:
-	kref_get(&router->refcount);
 	rcu_read_unlock();
 	return router;
 }
@@ -1315,7 +1354,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
 	struct list_head bonding_list;
 	unsigned long last_valid;
 	unsigned long real_bits[NUM_WORDS];
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
-	struct rcu_head rcu_bond;
 	struct orig_node *orig_node;
 	struct batman_if *if_incoming;
 };
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
 
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..c1c3258 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -776,7 +776,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +794,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return;
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCHv2 4/4] batman-adv: Correct rcu refcounting for neigh_node

[Marek Lindner]

From: Sven Eckelmann <sven@narfation.org>

It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
Fixed refcounting in find_router().

 batman-adv/icmp_socket.c |    7 ++-
 batman-adv/originator.c  |   26 +++-------
 batman-adv/originator.h  |    3 +-
 batman-adv/routing.c     |  111 +++++++++++++++++++++++++++++++++-------------
 batman-adv/types.h       |    3 +-
 batman-adv/unicast.c     |    2 +-
 batman-adv/vis.c         |    8 +++-
 7 files changed, 101 insertions(+), 59 deletions(-)

diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index 10a969c..046f976 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -230,10 +230,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	if (!neigh_node->if_incoming)
@@ -261,7 +262,7 @@ free_skb:
 	kfree_skb(skb);
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
 	return 0;
 }
 
-void neigh_node_free_ref(struct kref *refcount)
-{
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(refcount, struct neigh_node, refcount);
-	kfree(neigh_node);
-}
-
 static void neigh_node_free_rcu(struct rcu_head *rcu)
 {
 	struct neigh_node *neigh_node;
 
 	neigh_node = container_of(rcu, struct neigh_node, rcu);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	kfree(neigh_node);
 }
 
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
 {
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	if (atomic_dec_and_test(&neigh_node->refcount))
+		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
 }
 
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
 	memcpy(neigh_node->addr, neigh, ETH_ALEN);
 	neigh_node->orig_node = orig_neigh_node;
 	neigh_node->if_incoming = if_incoming;
-	kref_init(&neigh_node->refcount);
+	atomic_set(&neigh_node->refcount, 1);
 
 	spin_lock_bh(&orig_node->neigh_list_lock);
 	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
 	list_for_each_entry_safe(neigh_node, tmp_neigh_node,
 				 &orig_node->bond_list, bonding_list) {
 		list_del_rcu(&neigh_node->bonding_list);
-		call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	/* for all neighbors towards this originator ... */
 	hlist_for_each_entry_safe(neigh_node, node, node_tmp,
 				  &orig_node->neigh_list, list) {
 		hlist_del_rcu(&neigh_node->list);
-		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
 
 			hlist_del_rcu(&neigh_node->list);
 			bonding_candidate_del(orig_node, neigh_node);
-			call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+			neigh_node_free_ref(neigh_node);
 		} else {
 			if ((!*best_neigh_node) ||
 			    (neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
 void originator_free(struct bat_priv *bat_priv);
 void purge_orig_ref(struct bat_priv *bat_priv);
 void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
 struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
 				   struct orig_node *orig_neigh_node,
 				   uint8_t *neigh,
 				   struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
 int orig_seq_print_text(struct seq_file *seq, void *offset);
 int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
 int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index 2861f18..091edd4 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -118,12 +118,12 @@ static void update_route(struct bat_priv *bat_priv,
 			orig_node->router->addr);
 	}
 
-	if (neigh_node)
-		kref_get(&neigh_node->refcount);
+	if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+		neigh_node = NULL;
 	neigh_node_tmp = orig_node->router;
 	orig_node->router = neigh_node;
 	if (neigh_node_tmp)
-		kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node_tmp);
 }
 
 
@@ -175,7 +175,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 		if (!neigh_node)
 			goto unlock;
 
-		kref_get(&neigh_node->refcount);
+		if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
+			goto unlock;
+		}
+
 		rcu_read_unlock();
 
 		neigh_node->last_valid = jiffies;
@@ -200,7 +204,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 		if (!neigh_node)
 			goto unlock;
 
-		kref_get(&neigh_node->refcount);
+		if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
+			goto unlock;
+		}
+
 		rcu_read_unlock();
 	}
 
@@ -262,7 +270,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	return ret;
 }
 
@@ -275,8 +283,8 @@ void bonding_candidate_del(struct orig_node *orig_node,
 		goto out;
 
 	list_del_rcu(&neigh_node->bonding_list);
-	call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
 	INIT_LIST_HEAD(&neigh_node->bonding_list);
+	neigh_node_free_ref(neigh_node);
 	atomic_dec(&orig_node->bond_candidates);
 
 out:
@@ -337,8 +345,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
 	if (!list_empty(&neigh_node->bonding_list))
 		goto out;
 
+	if (!atomic_inc_not_zero(&neigh_node->refcount))
+		goto out;
+
 	list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
-	kref_get(&neigh_node->refcount);
 	atomic_inc(&orig_node->bond_candidates);
 	goto out;
 
@@ -382,7 +392,10 @@ static void update_orig(struct bat_priv *bat_priv,
 	hlist_for_each_entry_rcu(tmp_neigh_node, node,
 				 &orig_node->neigh_list, list) {
 		if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
-		    (tmp_neigh_node->if_incoming == if_incoming)) {
+		    (tmp_neigh_node->if_incoming == if_incoming) &&
+		     atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+			if (neigh_node)
+				neigh_node_free_ref(neigh_node);
 			neigh_node = tmp_neigh_node;
 			continue;
 		}
@@ -409,11 +422,15 @@ static void update_orig(struct bat_priv *bat_priv,
 		kref_put(&orig_tmp->refcount, orig_node_free_ref);
 		if (!neigh_node)
 			goto unlock;
+
+		if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
+			goto unlock;
+		}
 	} else
 		bat_dbg(DBG_BATMAN, bat_priv,
 			"Updating existing last-hop neighbor of originator\n");
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	orig_node->flags = batman_packet->flags;
@@ -490,7 +507,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 }
 
 /* checks whether the host restarted and is in the protection time.
@@ -894,7 +911,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -917,7 +938,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -958,7 +979,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -981,7 +1006,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1054,7 +1079,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -1075,7 +1104,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1108,12 +1137,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	/* select default router to output */
 	router = orig_node->router;
 	router_orig = orig_node->router->orig_node;
-	if (!router_orig) {
+	if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
 		rcu_read_unlock();
 		return NULL;
 	}
 
-
 	if ((!recv_if) && (!bonding_enabled))
 		goto return_router;
 
@@ -1146,6 +1174,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	 * is is not on the interface where the packet came
 	 * in. */
 
+	neigh_node_free_ref(router);
 	first_candidate = NULL;
 	router = NULL;
 
@@ -1158,16 +1187,23 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 			if (!first_candidate)
 				first_candidate = tmp_neigh_node;
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if) {
+			if (tmp_neigh_node->if_incoming != recv_if &&
+			    atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
 				router = tmp_neigh_node;
 				break;
 			}
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 
+		if (!router) {
+			rcu_read_unlock();
+			return NULL;
+		}
+
 		/* selected should point to the next element
 		 * after the current router */
 		spin_lock_bh(&primary_orig_node->neigh_list_lock);
@@ -1188,21 +1224,34 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 				first_candidate = tmp_neigh_node;
 
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if)
-				/* if we don't have a router yet
-				 * or this one is better, choose it. */
-				if ((!router) ||
-				(tmp_neigh_node->tq_avg > router->tq_avg)) {
-					router = tmp_neigh_node;
-				}
+			if (tmp_neigh_node->if_incoming == recv_if)
+				continue;
+
+			if (!atomic_inc_not_zero(&tmp_neigh_node->refcount))
+				continue;
+
+			/* if we don't have a router yet
+			 * or this one is better, choose it. */
+			if ((!router) ||
+			    (tmp_neigh_node->tq_avg > router->tq_avg)) {
+				/* decrement refcount of
+				 * previously selected router */
+				if (router)
+					neigh_node_free_ref(router);
+
+				router = tmp_neigh_node;
+				atomic_inc_not_zero(&router->refcount);
+			}
+
+			neigh_node_free_ref(tmp_neigh_node);
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 	}
 return_router:
-	kref_get(&router->refcount);
 	rcu_read_unlock();
 	return router;
 }
@@ -1315,7 +1364,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
 	struct list_head bonding_list;
 	unsigned long last_valid;
 	unsigned long real_bits[NUM_WORDS];
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
-	struct rcu_head rcu_bond;
 	struct orig_node *orig_node;
 	struct batman_if *if_incoming;
 };
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
 
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..c1c3258 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -776,7 +776,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +794,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return;
-- 
1.7.2.3

[B.A.T.M.A.N.] Reordered rcu refcounting, v3

[Linus Lüssing]

Hi everyone,

Patches [1-4] are untouched, added my stuff on top of it with some fixes:

* Made sure no rcu_read_lock() while gw_deselect() (gw_election(), gw_check_election())
* Removed orig_node null pointer check in gw_get_selected()
* Removed rcu_dereference() for orig_node
* Rebased to marec's last 4 patches
* Made extra patch for bat_priv->curr_gw rcu protection fixes
* Removed the stupid kref_get() removal which was not supposed to be part of this patch [my old 1/3]
* Removed the wrong kref_get() removal [my old 3/3]

It's all based on "batman-adv: Switch order of types.h and compat.h inclusion" (see separate patchset)
which fixes a compile error in my old [2/3] patch.

Note: rcu_dereference() in gw_node_purge() does not seem to be safe yet, there's
no rcu_read_lock(), so another thread, not through gw_node_purge(), might have just
performed a gw_deselect().

Cheers, Linus

[B.A.T.M.A.N.] [PATCH 1/7] batman-adv: Correct rcu refcounting for gw_node

[Linus Lüssing]

From: Sven Eckelmann <sven@narfation.org>

It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 gateway_client.c |   41 ++++++++++++++++++-----------------------
 types.h          |    2 +-
 2 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 429a013..517e001 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -28,20 +28,18 @@
 #include <linux/udp.h>
 #include <linux/if_vlan.h>
 
-static void gw_node_free_ref(struct kref *refcount)
-{
-	struct gw_node *gw_node;
-
-	gw_node = container_of(refcount, struct gw_node, refcount);
-	kfree(gw_node);
-}
-
 static void gw_node_free_rcu(struct rcu_head *rcu)
 {
 	struct gw_node *gw_node;
 
 	gw_node = container_of(rcu, struct gw_node, rcu);
-	kref_put(&gw_node->refcount, gw_node_free_ref);
+	kfree(gw_node);
+}
+
+static void gw_node_free_ref(struct gw_node *gw_node)
+{
+	if (atomic_dec_and_test(&gw_node->refcount))
+		call_rcu(&gw_node->rcu, gw_node_free_rcu);
 }
 
 void *gw_get_selected(struct bat_priv *bat_priv)
@@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv)
 	bat_priv->curr_gw = NULL;
 
 	if (gw_node)
-		kref_put(&gw_node->refcount, gw_node_free_ref);
+		gw_node_free_ref(gw_node);
 }
 
-static struct gw_node *gw_select(struct bat_priv *bat_priv,
-			  struct gw_node *new_gw_node)
+static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
 {
 	struct gw_node *curr_gw_node = bat_priv->curr_gw;
 
-	if (new_gw_node)
-		kref_get(&new_gw_node->refcount);
+	if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+		new_gw_node = NULL;
 
 	bat_priv->curr_gw = new_gw_node;
-	return curr_gw_node;
+
+	if (curr_gw_node)
+		gw_node_free_ref(curr_gw_node);
 }
 
 void gw_election(struct bat_priv *bat_priv)
 {
 	struct hlist_node *node;
-	struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+	struct gw_node *gw_node, *curr_gw_tmp = NULL;
 	uint8_t max_tq = 0;
 	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
 	int down, up;
@@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv)
 				curr_gw_tmp->orig_node->gw_flags,
 				curr_gw_tmp->orig_node->router->tq_avg);
 
-		old_gw_node = gw_select(bat_priv, curr_gw_tmp);
+		gw_select(bat_priv, curr_gw_tmp);
 	}
 
 	rcu_read_unlock();
-
-	/* the kfree() has to be outside of the rcu lock */
-	if (old_gw_node)
-		kref_put(&old_gw_node->refcount, gw_node_free_ref);
 }
 
 void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
@@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
 	memset(gw_node, 0, sizeof(struct gw_node));
 	INIT_HLIST_NODE(&gw_node->list);
 	gw_node->orig_node = orig_node;
-	kref_init(&gw_node->refcount);
+	atomic_set(&gw_node->refcount, 1);
 
 	spin_lock_bh(&bat_priv->gw_list_lock);
 	hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
 			gw_deselect(bat_priv);
 
 		hlist_del_rcu(&gw_node->list);
-		call_rcu(&gw_node->rcu, gw_node_free_rcu);
+		gw_node_free_ref(gw_node);
 	}
 
 
diff --git a/batman-adv/types.h b/batman-adv/types.h
index e4a0462..ca5f20a 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -100,7 +100,7 @@ struct gw_node {
 	struct hlist_node list;
 	struct orig_node *orig_node;
 	unsigned long deleted;
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
 };
 
-- 
1.7.2.3

Re: [B.A.T.M.A.N.] [PATCH 1/7] batman-adv: Correct rcu refcounting for gw_node

[Linus Lüssing]

So, had a second look at the way the gw_node rcu-locking
refcounting stuff is done and looks sane to me with the
rcu_dereference/assign_pointer additions. Just one thing I noticed
in gw_select() which probably is not an issue:

It's not harmful to hold a rcu_read_lock() while calling this as
we are using call_rcu() gw_node_free_ref(), right?
Do we need to increase the new_gw_node refcount? Of course we
cannot call the whole gw_select() without
any rcu locking due to the atomic_inc_not_zero(), but just
wondering if the rcu_read_unlock() should be called earlier.

So if some one can confirm that this is not an issue, then the
gw_node rcu refcount patches get my ok for being applied to the
trunk.

Cheers, Linus

On Fri, Feb 04, 2011 at 04:21:30PM +0100, Linus Lüssing wrote:
> From: Sven Eckelmann <sven@narfation.org>
> 
> It might be possible that 2 threads access the same data in the same
> rcu grace period. The first thread calls call_rcu() to decrement the
> refcount and free the data while the second thread increases the
> refcount to use the data. To avoid this race condition all refcount
> operations have to be atomic.
> 
> Reported-by: Sven Eckelmann <sven@narfation.org>
> Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
> ---
>  gateway_client.c |   41 ++++++++++++++++++-----------------------
>  types.h          |    2 +-
>  2 files changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
> index 429a013..517e001 100644
> --- a/batman-adv/gateway_client.c
> +++ b/batman-adv/gateway_client.c
> @@ -28,20 +28,18 @@
>  #include <linux/udp.h>
>  #include <linux/if_vlan.h>
>  
> -static void gw_node_free_ref(struct kref *refcount)
> -{
> -	struct gw_node *gw_node;
> -
> -	gw_node = container_of(refcount, struct gw_node, refcount);
> -	kfree(gw_node);
> -}
> -
>  static void gw_node_free_rcu(struct rcu_head *rcu)
>  {
>  	struct gw_node *gw_node;
>  
>  	gw_node = container_of(rcu, struct gw_node, rcu);
> -	kref_put(&gw_node->refcount, gw_node_free_ref);
> +	kfree(gw_node);
> +}
> +
> +static void gw_node_free_ref(struct gw_node *gw_node)
> +{
> +	if (atomic_dec_and_test(&gw_node->refcount))
> +		call_rcu(&gw_node->rcu, gw_node_free_rcu);
>  }
>  
>  void *gw_get_selected(struct bat_priv *bat_priv)
> @@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv)
>  	bat_priv->curr_gw = NULL;
>  
>  	if (gw_node)
> -		kref_put(&gw_node->refcount, gw_node_free_ref);
> +		gw_node_free_ref(gw_node);
>  }
>  
> -static struct gw_node *gw_select(struct bat_priv *bat_priv,
> -			  struct gw_node *new_gw_node)
> +static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
>  {
>  	struct gw_node *curr_gw_node = bat_priv->curr_gw;
>  
> -	if (new_gw_node)
> -		kref_get(&new_gw_node->refcount);
> +	if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
> +		new_gw_node = NULL;
>  
>  	bat_priv->curr_gw = new_gw_node;
> -	return curr_gw_node;
> +
> +	if (curr_gw_node)
> +		gw_node_free_ref(curr_gw_node);
>  }
>  
>  void gw_election(struct bat_priv *bat_priv)
>  {
>  	struct hlist_node *node;
> -	struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
> +	struct gw_node *gw_node, *curr_gw_tmp = NULL;
>  	uint8_t max_tq = 0;
>  	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
>  	int down, up;
> @@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv)
>  				curr_gw_tmp->orig_node->gw_flags,
>  				curr_gw_tmp->orig_node->router->tq_avg);
>  
> -		old_gw_node = gw_select(bat_priv, curr_gw_tmp);
> +		gw_select(bat_priv, curr_gw_tmp);
>  	}
>  
>  	rcu_read_unlock();
> -
> -	/* the kfree() has to be outside of the rcu lock */
> -	if (old_gw_node)
> -		kref_put(&old_gw_node->refcount, gw_node_free_ref);
>  }
>  
>  void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
> @@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
>  	memset(gw_node, 0, sizeof(struct gw_node));
>  	INIT_HLIST_NODE(&gw_node->list);
>  	gw_node->orig_node = orig_node;
> -	kref_init(&gw_node->refcount);
> +	atomic_set(&gw_node->refcount, 1);
>  
>  	spin_lock_bh(&bat_priv->gw_list_lock);
>  	hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
> @@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
>  			gw_deselect(bat_priv);
>  
>  		hlist_del_rcu(&gw_node->list);
> -		call_rcu(&gw_node->rcu, gw_node_free_rcu);
> +		gw_node_free_ref(gw_node);
>  	}
>  
>  
> diff --git a/batman-adv/types.h b/batman-adv/types.h
> index e4a0462..ca5f20a 100644
> --- a/batman-adv/types.h
> +++ b/batman-adv/types.h
> @@ -100,7 +100,7 @@ struct gw_node {
>  	struct hlist_node list;
>  	struct orig_node *orig_node;
>  	unsigned long deleted;
> -	struct kref refcount;
> +	atomic_t refcount;
>  	struct rcu_head rcu;
>  };
>  
> -- 
> 1.7.2.3
> 

[B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh

[Linus Lüssing]

From: Sven Eckelmann <sven@narfation.org>

It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 soft-interface.c |   35 +++++++++++++++++------------------
 types.h          |    2 +-
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c
index bd088f8..bd8b539 100644
--- a/batman-adv/soft-interface.c
+++ b/batman-adv/soft-interface.c
@@ -78,20 +78,18 @@ int my_skb_head_push(struct sk_buff *skb, unsigned int len)
 	return 0;
 }
 
-static void softif_neigh_free_ref(struct kref *refcount)
-{
-	struct softif_neigh *softif_neigh;
-
-	softif_neigh = container_of(refcount, struct softif_neigh, refcount);
-	kfree(softif_neigh);
-}
-
 static void softif_neigh_free_rcu(struct rcu_head *rcu)
 {
 	struct softif_neigh *softif_neigh;
 
 	softif_neigh = container_of(rcu, struct softif_neigh, rcu);
-	kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+	kfree(softif_neigh);
+}
+
+static void softif_neigh_free_ref(struct softif_neigh *softif_neigh)
+{
+	if (atomic_dec_and_test(&softif_neigh->refcount))
+		call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
 }
 
 void softif_neigh_purge(struct bat_priv *bat_priv)
@@ -118,11 +116,10 @@ void softif_neigh_purge(struct bat_priv *bat_priv)
 				 softif_neigh->addr, softif_neigh->vid);
 			softif_neigh_tmp = bat_priv->softif_neigh;
 			bat_priv->softif_neigh = NULL;
-			kref_put(&softif_neigh_tmp->refcount,
-				 softif_neigh_free_ref);
+			softif_neigh_free_ref(softif_neigh_tmp);
 		}
 
-		call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
+		softif_neigh_free_ref(softif_neigh);
 	}
 
 	spin_unlock_bh(&bat_priv->softif_neigh_lock);
@@ -143,8 +140,11 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
 		if (softif_neigh->vid != vid)
 			continue;
 
+		if (!atomic_inc_not_zero(&softif_neigh->refcount))
+			continue;
+
 		softif_neigh->last_seen = jiffies;
-		goto found;
+		goto out;
 	}
 
 	softif_neigh = kzalloc(sizeof(struct softif_neigh), GFP_ATOMIC);
@@ -154,15 +154,14 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
 	memcpy(softif_neigh->addr, addr, ETH_ALEN);
 	softif_neigh->vid = vid;
 	softif_neigh->last_seen = jiffies;
-	kref_init(&softif_neigh->refcount);
+	/* initialize with 2 - caller decrements counter by one */
+	atomic_set(&softif_neigh->refcount, 2);
 
 	INIT_HLIST_NODE(&softif_neigh->list);
 	spin_lock_bh(&bat_priv->softif_neigh_lock);
 	hlist_add_head_rcu(&softif_neigh->list, &bat_priv->softif_neigh_list);
 	spin_unlock_bh(&bat_priv->softif_neigh_lock);
 
-found:
-	kref_get(&softif_neigh->refcount);
 out:
 	rcu_read_unlock();
 	return softif_neigh;
@@ -266,7 +265,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
 			 softif_neigh->addr, softif_neigh->vid);
 		softif_neigh_tmp = bat_priv->softif_neigh;
 		bat_priv->softif_neigh = softif_neigh;
-		kref_put(&softif_neigh_tmp->refcount, softif_neigh_free_ref);
+		softif_neigh_free_ref(softif_neigh_tmp);
 		/* we need to hold the additional reference */
 		goto err;
 	}
@@ -284,7 +283,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
 	}
 
 out:
-	kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+	softif_neigh_free_ref(softif_neigh);
 err:
 	kfree_skb(skb);
 	return;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index ca5f20a..8df6d9d 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -270,7 +270,7 @@ struct softif_neigh {
 	uint8_t addr[ETH_ALEN];
 	unsigned long last_seen;
 	short vid;
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
 };
 
-- 
1.7.2.3

Re: [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh

[Linus Lüssing]

On Fri, Feb 04, 2011 at 04:21:31PM +0100, Linus Lüssing wrote:
> @@ -143,8 +140,11 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
>  		if (softif_neigh->vid != vid)
>  			continue;
>  
> +		if (!atomic_inc_not_zero(&softif_neigh->refcount))
> +			continue;
> +
>  		softif_neigh->last_seen = jiffies;
> -		goto found;
> +		goto out;
>  	}
Hmm, we could do a rcu_read_unlock() here, already, I think. Would
it be better to do so, keeping the rcu grace period as small as
possible?
>  
>  	softif_neigh = kzalloc(sizeof(struct softif_neigh), GFP_ATOMIC);
> @@ -154,15 +154,14 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
>  	memcpy(softif_neigh->addr, addr, ETH_ALEN);
>  	softif_neigh->vid = vid;
>  	softif_neigh->last_seen = jiffies;
> -	kref_init(&softif_neigh->refcount);
> +	/* initialize with 2 - caller decrements counter by one */
> +	atomic_set(&softif_neigh->refcount, 2);
>  
>  	INIT_HLIST_NODE(&softif_neigh->list);
>  	spin_lock_bh(&bat_priv->softif_neigh_lock);
>  	hlist_add_head_rcu(&softif_neigh->list, &bat_priv->softif_neigh_list);
>  	spin_unlock_bh(&bat_priv->softif_neigh_lock);
>  
> -found:
> -	kref_get(&softif_neigh->refcount);
>  out:
>  	rcu_read_unlock();
>  	return softif_neigh;

The rest of the new atomic handling seems fine. Just two more
things I noticed while reading the softif_neigh specific code:
bat_priv->softif_neigh needs to be changed to a rcu-protected
pointer.

There's a race condition in softif_neigh_seq_print_text(), between
the rcu_read_unlock() and rcu_read_lock() the number of
softif_neigh's can have increased and there's no check for
accidentally writing outside of the allocated buffer.

Cheers, Linus

Re: [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh

[Marek Lindner]

On Thursday 10 February 2011 13:45:44 Linus Lüssing wrote:
> > -		goto found;
> > +		goto out;
> > 
> >  	}
> 
> Hmm, we could do a rcu_read_unlock() here, already, I think. Would
> it be better to do so, keeping the rcu grace period as small as
> possible?

Can you be more specific where "here" is ? Instead of the "goto out" ?


> The rest of the new atomic handling seems fine. Just two more
> things I noticed while reading the softif_neigh specific code:
> bat_priv->softif_neigh needs to be changed to a rcu-protected
> pointer.

Agreed.


> There's a race condition in softif_neigh_seq_print_text(), between
> the rcu_read_unlock() and rcu_read_lock() the number of
> softif_neigh's can have increased and there's no check for
> accidentally writing outside of the allocated buffer.

Also correct - are you going to send a patch ?

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh

[Linus Lüssing]

> > There's a race condition in softif_neigh_seq_print_text(), between
> > the rcu_read_unlock() and rcu_read_lock() the number of
> > softif_neigh's can have increased and there's no check for
> > accidentally writing outside of the allocated buffer.
> 
> Also correct - are you going to send a patch ?
Yes, will do, wait a sec.

[B.A.T.M.A.N.] [PATCH 3/7] batman-adv: Correct rcu refcounting for batman_if

[Linus Lüssing]

From: Sven Eckelmann <sven@narfation.org>

It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 bat_sysfs.c      |   20 +++++++++-----------
 hard-interface.c |   40 +++++++++++++++++++---------------------
 hard-interface.h |    9 ++++-----
 types.h          |    2 +-
 4 files changed, 33 insertions(+), 38 deletions(-)

diff --git a/batman-adv/bat_sysfs.c b/batman-adv/bat_sysfs.c
index f7b93a0..93ae20a 100644
--- a/batman-adv/bat_sysfs.c
+++ b/batman-adv/bat_sysfs.c
@@ -450,7 +450,7 @@ static ssize_t show_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	length = sprintf(buff, "%s\n", batman_if->if_status == IF_NOT_IN_USE ?
 			 "none" : batman_if->soft_iface->name);
 
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 
 	return length;
 }
@@ -461,7 +461,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	struct net_device *net_dev = kobj_to_netdev(kobj);
 	struct batman_if *batman_if = get_batman_if_by_netdev(net_dev);
 	int status_tmp = -1;
-	int ret;
+	int ret = count;
 
 	if (!batman_if)
 		return count;
@@ -472,7 +472,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	if (strlen(buff) >= IFNAMSIZ) {
 		pr_err("Invalid parameter for 'mesh_iface' setting received: "
 		       "interface name too long '%s'\n", buff);
-		kref_put(&batman_if->refcount, hardif_free_ref);
+		hardif_free_ref(batman_if);
 		return -EINVAL;
 	}
 
@@ -482,17 +482,14 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 		status_tmp = IF_I_WANT_YOU;
 
 	if ((batman_if->if_status == status_tmp) || ((batman_if->soft_iface) &&
-	    (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) {
-		kref_put(&batman_if->refcount, hardif_free_ref);
-		return count;
-	}
+	    (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0)))
+		goto out;
 
 	if (status_tmp == IF_NOT_IN_USE) {
 		rtnl_lock();
 		hardif_disable_interface(batman_if);
 		rtnl_unlock();
-		kref_put(&batman_if->refcount, hardif_free_ref);
-		return count;
+		goto out;
 	}
 
 	/* if the interface already is in use */
@@ -503,8 +500,9 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
 	}
 
 	ret = hardif_enable_interface(batman_if, buff);
-	kref_put(&batman_if->refcount, hardif_free_ref);
 
+out:
+	hardif_free_ref(batman_if);
 	return ret;
 }
 
@@ -537,7 +535,7 @@ static ssize_t show_iface_status(struct kobject *kobj, struct attribute *attr,
 		break;
 	}
 
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 
 	return length;
 }
diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index e2b001a..8982485 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -40,13 +40,13 @@ static int batman_skb_recv(struct sk_buff *skb,
 			   struct packet_type *ptype,
 			   struct net_device *orig_dev);
 
-static void hardif_free_rcu(struct rcu_head *rcu)
+void hardif_free_rcu(struct rcu_head *rcu)
 {
 	struct batman_if *batman_if;
 
 	batman_if = container_of(rcu, struct batman_if, rcu);
 	dev_put(batman_if->net_dev);
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	kfree(batman_if);
 }
 
 struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
@@ -55,16 +55,14 @@ struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(batman_if, &if_list, list) {
-		if (batman_if->net_dev == net_dev)
+		if (batman_if->net_dev == net_dev &&
+		    atomic_inc_not_zero(&batman_if->refcount))
 			goto out;
 	}
 
 	batman_if = NULL;
 
 out:
-	if (batman_if)
-		kref_get(&batman_if->refcount);
-
 	rcu_read_unlock();
 	return batman_if;
 }
@@ -105,16 +103,14 @@ static struct batman_if *get_active_batman_if(struct net_device *soft_iface)
 		if (batman_if->soft_iface != soft_iface)
 			continue;
 
-		if (batman_if->if_status == IF_ACTIVE)
+		if (batman_if->if_status == IF_ACTIVE &&
+		    atomic_inc_not_zero(&batman_if->refcount))
 			goto out;
 	}
 
 	batman_if = NULL;
 
 out:
-	if (batman_if)
-		kref_get(&batman_if->refcount);
-
 	rcu_read_unlock();
 	return batman_if;
 }
@@ -137,14 +133,14 @@ static void set_primary_if(struct bat_priv *bat_priv,
 	struct batman_packet *batman_packet;
 	struct batman_if *old_if;
 
-	if (batman_if)
-		kref_get(&batman_if->refcount);
+	if (batman_if && !atomic_inc_not_zero(&batman_if->refcount))
+		batman_if = NULL;
 
 	old_if = bat_priv->primary_if;
 	bat_priv->primary_if = batman_if;
 
 	if (old_if)
-		kref_put(&old_if->refcount, hardif_free_ref);
+		hardif_free_ref(old_if);
 
 	if (!bat_priv->primary_if)
 		return;
@@ -290,6 +286,9 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
 	if (batman_if->if_status != IF_NOT_IN_USE)
 		goto out;
 
+	if (!atomic_inc_not_zero(&batman_if->refcount))
+		goto out;
+
 	batman_if->soft_iface = dev_get_by_name(&init_net, iface_name);
 
 	if (!batman_if->soft_iface) {
@@ -328,7 +327,6 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
 	batman_if->batman_adv_ptype.type = __constant_htons(ETH_P_BATMAN);
 	batman_if->batman_adv_ptype.func = batman_skb_recv;
 	batman_if->batman_adv_ptype.dev = batman_if->net_dev;
-	kref_get(&batman_if->refcount);
 	dev_add_pack(&batman_if->batman_adv_ptype);
 
 	atomic_set(&batman_if->seqno, 1);
@@ -371,6 +369,7 @@ out:
 	return 0;
 
 err:
+	hardif_free_ref(batman_if);
 	return -ENOMEM;
 }
 
@@ -387,7 +386,6 @@ void hardif_disable_interface(struct batman_if *batman_if)
 	bat_info(batman_if->soft_iface, "Removing interface: %s\n",
 		 batman_if->net_dev->name);
 	dev_remove_pack(&batman_if->batman_adv_ptype);
-	kref_put(&batman_if->refcount, hardif_free_ref);
 
 	bat_priv->num_ifaces--;
 	orig_hash_del_if(batman_if, bat_priv->num_ifaces);
@@ -399,7 +397,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
 		set_primary_if(bat_priv, new_if);
 
 		if (new_if)
-			kref_put(&new_if->refcount, hardif_free_ref);
+			hardif_free_ref(new_if);
 	}
 
 	kfree(batman_if->packet_buff);
@@ -416,6 +414,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
 		softif_destroy(batman_if->soft_iface);
 
 	batman_if->soft_iface = NULL;
+	hardif_free_ref(batman_if);
 }
 
 static struct batman_if *hardif_add_interface(struct net_device *net_dev)
@@ -445,7 +444,8 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
 	batman_if->soft_iface = NULL;
 	batman_if->if_status = IF_NOT_IN_USE;
 	INIT_LIST_HEAD(&batman_if->list);
-	kref_init(&batman_if->refcount);
+	/* extra reference for return */
+	atomic_set(&batman_if->refcount, 2);
 
 	check_known_mac_addr(batman_if->net_dev);
 
@@ -453,8 +453,6 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
 	list_add_tail_rcu(&batman_if->list, &if_list);
 	spin_unlock(&if_list_lock);
 
-	/* extra reference for return */
-	kref_get(&batman_if->refcount);
 	return batman_if;
 
 free_if:
@@ -476,7 +474,7 @@ static void hardif_remove_interface(struct batman_if *batman_if)
 
 	batman_if->if_status = IF_TO_BE_REMOVED;
 	sysfs_del_hardif(&batman_if->hardif_obj);
-	call_rcu(&batman_if->rcu, hardif_free_rcu);
+	hardif_free_ref(batman_if);
 }
 
 void hardif_remove_interfaces(void)
@@ -548,7 +546,7 @@ static int hard_if_event(struct notifier_block *this,
 	};
 
 hardif_put:
-	kref_put(&batman_if->refcount, hardif_free_ref);
+	hardif_free_ref(batman_if);
 out:
 	return NOTIFY_DONE;
 }
diff --git a/batman-adv/hard-interface.h b/batman-adv/hard-interface.h
index ad19543..e488b90 100644
--- a/batman-adv/hard-interface.h
+++ b/batman-adv/hard-interface.h
@@ -37,13 +37,12 @@ void hardif_disable_interface(struct batman_if *batman_if);
 void hardif_remove_interfaces(void);
 int hardif_min_mtu(struct net_device *soft_iface);
 void update_min_mtu(struct net_device *soft_iface);
+void hardif_free_rcu(struct rcu_head *rcu);
 
-static inline void hardif_free_ref(struct kref *refcount)
+static inline void hardif_free_ref(struct batman_if *batman_if)
 {
-	struct batman_if *batman_if;
-
-	batman_if = container_of(refcount, struct batman_if, refcount);
-	kfree(batman_if);
+	if (atomic_dec_and_test(&batman_if->refcount))
+		call_rcu(&batman_if->rcu, hardif_free_rcu);
 }
 
 #endif /* _NET_BATMAN_ADV_HARD_INTERFACE_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 8df6d9d..317ede8 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -43,7 +43,7 @@ struct batman_if {
 	unsigned char *packet_buff;
 	int packet_len;
 	struct kobject *hardif_obj;
-	struct kref refcount;
+	atomic_t refcount;
 	struct packet_type batman_adv_ptype;
 	struct net_device *soft_iface;
 	struct rcu_head rcu;
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH 4/7] batman-adv: Correct rcu refcounting for neigh_node

[Linus Lüssing]

From: Sven Eckelmann <sven@narfation.org>

It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 icmp_socket.c |    7 ++--
 originator.c  |   26 ++++---------
 originator.h  |    3 +-
 routing.c     |  111 +++++++++++++++++++++++++++++++++++++++++----------------
 types.h       |    3 +-
 unicast.c     |    2 +-
 vis.c         |    8 +++-
 7 files changed, 101 insertions(+), 59 deletions(-)

diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index fc812e1..4465a1a 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -229,10 +229,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
 	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
-	if (!neigh_node)
+	if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
 		goto unlock;
+	}
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	if (!neigh_node->if_incoming)
@@ -260,7 +261,7 @@ free_skb:
 	kfree_skb(skb);
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
 	return 0;
 }
 
-void neigh_node_free_ref(struct kref *refcount)
-{
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(refcount, struct neigh_node, refcount);
-	kfree(neigh_node);
-}
-
 static void neigh_node_free_rcu(struct rcu_head *rcu)
 {
 	struct neigh_node *neigh_node;
 
 	neigh_node = container_of(rcu, struct neigh_node, rcu);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	kfree(neigh_node);
 }
 
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
 {
-	struct neigh_node *neigh_node;
-
-	neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
-	kref_put(&neigh_node->refcount, neigh_node_free_ref);
+	if (atomic_dec_and_test(&neigh_node->refcount))
+		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
 }
 
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
 	memcpy(neigh_node->addr, neigh, ETH_ALEN);
 	neigh_node->orig_node = orig_neigh_node;
 	neigh_node->if_incoming = if_incoming;
-	kref_init(&neigh_node->refcount);
+	atomic_set(&neigh_node->refcount, 1);
 
 	spin_lock_bh(&orig_node->neigh_list_lock);
 	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
 	list_for_each_entry_safe(neigh_node, tmp_neigh_node,
 				 &orig_node->bond_list, bonding_list) {
 		list_del_rcu(&neigh_node->bonding_list);
-		call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	/* for all neighbors towards this originator ... */
 	hlist_for_each_entry_safe(neigh_node, node, node_tmp,
 				  &orig_node->neigh_list, list) {
 		hlist_del_rcu(&neigh_node->list);
-		call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+		neigh_node_free_ref(neigh_node);
 	}
 
 	spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
 
 			hlist_del_rcu(&neigh_node->list);
 			bonding_candidate_del(orig_node, neigh_node);
-			call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+			neigh_node_free_ref(neigh_node);
 		} else {
 			if ((!*best_neigh_node) ||
 			    (neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
 void originator_free(struct bat_priv *bat_priv);
 void purge_orig_ref(struct bat_priv *bat_priv);
 void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
 struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
 				   struct orig_node *orig_neigh_node,
 				   uint8_t *neigh,
 				   struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
 int orig_seq_print_text(struct seq_file *seq, void *offset);
 int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
 int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index b792df4..a2b770a 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -117,12 +117,12 @@ static void update_route(struct bat_priv *bat_priv,
 			orig_node->router->addr);
 	}
 
-	if (neigh_node)
-		kref_get(&neigh_node->refcount);
+	if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+		neigh_node = NULL;
 	neigh_node_tmp = orig_node->router;
 	orig_node->router = neigh_node;
 	if (neigh_node_tmp)
-		kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node_tmp);
 }
 
 
@@ -174,7 +174,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 		if (!neigh_node)
 			goto unlock;
 
-		kref_get(&neigh_node->refcount);
+		if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
+			goto unlock;
+		}
+
 		rcu_read_unlock();
 
 		neigh_node->last_valid = jiffies;
@@ -199,7 +203,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
 		if (!neigh_node)
 			goto unlock;
 
-		kref_get(&neigh_node->refcount);
+		if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
+			goto unlock;
+		}
+
 		rcu_read_unlock();
 	}
 
@@ -261,7 +269,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	return ret;
 }
 
@@ -274,8 +282,8 @@ void bonding_candidate_del(struct orig_node *orig_node,
 		goto out;
 
 	list_del_rcu(&neigh_node->bonding_list);
-	call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
 	INIT_LIST_HEAD(&neigh_node->bonding_list);
+	neigh_node_free_ref(neigh_node);
 	atomic_dec(&orig_node->bond_candidates);
 
 out:
@@ -336,8 +344,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
 	if (!list_empty(&neigh_node->bonding_list))
 		goto out;
 
+	if (!atomic_inc_not_zero(&neigh_node->refcount))
+		goto out;
+
 	list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
-	kref_get(&neigh_node->refcount);
 	atomic_inc(&orig_node->bond_candidates);
 	goto out;
 
@@ -381,7 +391,10 @@ static void update_orig(struct bat_priv *bat_priv,
 	hlist_for_each_entry_rcu(tmp_neigh_node, node,
 				 &orig_node->neigh_list, list) {
 		if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
-		    (tmp_neigh_node->if_incoming == if_incoming)) {
+		    (tmp_neigh_node->if_incoming == if_incoming) &&
+		     atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+			if (neigh_node)
+				neigh_node_free_ref(neigh_node);
 			neigh_node = tmp_neigh_node;
 			continue;
 		}
@@ -408,11 +421,15 @@ static void update_orig(struct bat_priv *bat_priv,
 		kref_put(&orig_tmp->refcount, orig_node_free_ref);
 		if (!neigh_node)
 			goto unlock;
+
+		if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+			neigh_node = NULL;
+			goto unlock;
+		}
 	} else
 		bat_dbg(DBG_BATMAN, bat_priv,
 			"Updating existing last-hop neighbor of originator\n");
 
-	kref_get(&neigh_node->refcount);
 	rcu_read_unlock();
 
 	orig_node->flags = batman_packet->flags;
@@ -489,7 +506,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 }
 
 /* checks whether the host restarted and is in the protection time.
@@ -893,7 +910,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -916,7 +937,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -957,7 +978,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -980,7 +1005,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1053,7 +1078,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -1074,7 +1103,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
@@ -1107,12 +1136,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	/* select default router to output */
 	router = orig_node->router;
 	router_orig = orig_node->router->orig_node;
-	if (!router_orig) {
+	if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
 		rcu_read_unlock();
 		return NULL;
 	}
 
-
 	if ((!recv_if) && (!bonding_enabled))
 		goto return_router;
 
@@ -1145,6 +1173,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 	 * is is not on the interface where the packet came
 	 * in. */
 
+	neigh_node_free_ref(router);
 	first_candidate = NULL;
 	router = NULL;
 
@@ -1157,16 +1186,23 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 			if (!first_candidate)
 				first_candidate = tmp_neigh_node;
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if) {
+			if (tmp_neigh_node->if_incoming != recv_if &&
+			    atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
 				router = tmp_neigh_node;
 				break;
 			}
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 
+		if (!router) {
+			rcu_read_unlock();
+			return NULL;
+		}
+
 		/* selected should point to the next element
 		 * after the current router */
 		spin_lock_bh(&primary_orig_node->neigh_list_lock);
@@ -1187,21 +1223,34 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 				first_candidate = tmp_neigh_node;
 
 			/* recv_if == NULL on the first node. */
-			if (tmp_neigh_node->if_incoming != recv_if)
-				/* if we don't have a router yet
-				 * or this one is better, choose it. */
-				if ((!router) ||
-				(tmp_neigh_node->tq_avg > router->tq_avg)) {
-					router = tmp_neigh_node;
-				}
+			if (tmp_neigh_node->if_incoming == recv_if)
+				continue;
+
+			if (!atomic_inc_not_zero(&tmp_neigh_node->refcount))
+				continue;
+
+			/* if we don't have a router yet
+			 * or this one is better, choose it. */
+			if ((!router) ||
+			    (tmp_neigh_node->tq_avg > router->tq_avg)) {
+				/* decrement refcount of
+				 * previously selected router */
+				if (router)
+					neigh_node_free_ref(router);
+
+				router = tmp_neigh_node;
+				atomic_inc_not_zero(&router->refcount);
+			}
+
+			neigh_node_free_ref(tmp_neigh_node);
 		}
 
 		/* use the first candidate if nothing was found. */
-		if (!router)
+		if (!router && first_candidate &&
+		    atomic_inc_not_zero(&first_candidate->refcount))
 			router = first_candidate;
 	}
 return_router:
-	kref_get(&router->refcount);
 	rcu_read_unlock();
 	return router;
 }
@@ -1314,7 +1363,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
 	struct list_head bonding_list;
 	unsigned long last_valid;
 	unsigned long real_bits[NUM_WORDS];
-	struct kref refcount;
+	atomic_t refcount;
 	struct rcu_head rcu;
-	struct rcu_head rcu_bond;
 	struct orig_node *orig_node;
 	struct batman_if *if_incoming;
 };
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
 
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..c1c3258 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -776,7 +776,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
 	if (!neigh_node)
 		goto unlock;
 
-	kref_get(&neigh_node->refcount);
+	if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+		neigh_node = NULL;
+		goto unlock;
+	}
+
 	rcu_read_unlock();
 
 	skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +794,7 @@ unlock:
 	rcu_read_unlock();
 out:
 	if (neigh_node)
-		kref_put(&neigh_node->refcount, neigh_node_free_ref);
+		neigh_node_free_ref(neigh_node);
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
 	return;
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer

[Linus Lüssing]

The rcu protected macros rcu_dereference() and rcu_assign_pointer()
for the bat_priv->curr_gw need to be used, as well as spin/rcu locking.

Otherwise we might end up using a curr_gw pointer pointing to already
freed memory.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 gateway_client.c |   87 ++++++++++++++++++++++++++++++++++++++---------------
 main.c           |    1 +
 types.h          |    1 +
 3 files changed, 64 insertions(+), 25 deletions(-)

diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 517e001..4624515 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -44,19 +44,30 @@ static void gw_node_free_ref(struct gw_node *gw_node)
 
 void *gw_get_selected(struct bat_priv *bat_priv)
 {
-	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+	struct gw_node *curr_gateway_tmp;
+	struct orig_node *orig_node;
 
-	if (!curr_gateway_tmp)
+	rcu_read_lock();
+	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
+	if (!curr_gateway_tmp) {
+		rcu_read_unlock();
 		return NULL;
+	}
 
-	return curr_gateway_tmp->orig_node;
+	orig_node = curr_gateway_tmp->orig_node;
+	rcu_read_unlock();
+
+	return orig_node;
 }
 
 void gw_deselect(struct bat_priv *bat_priv)
 {
-	struct gw_node *gw_node = bat_priv->curr_gw;
+	struct gw_node *gw_node;
 
-	bat_priv->curr_gw = NULL;
+	spin_lock_bh(&bat_priv->curr_gw_lock);
+	gw_node = rcu_dereference(bat_priv->curr_gw);
+	rcu_assign_pointer(bat_priv->curr_gw, NULL);
+	spin_unlock_bh(&bat_priv->curr_gw_lock);
 
 	if (gw_node)
 		gw_node_free_ref(gw_node);
@@ -64,12 +75,15 @@ void gw_deselect(struct bat_priv *bat_priv)
 
 static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
 {
-	struct gw_node *curr_gw_node = bat_priv->curr_gw;
+	struct gw_node *curr_gw_node;
 
 	if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
 		new_gw_node = NULL;
 
-	bat_priv->curr_gw = new_gw_node;
+	spin_lock_bh(&bat_priv->curr_gw_lock);
+	curr_gw_node = rcu_dereference(bat_priv->curr_gw);
+	rcu_assign_pointer(bat_priv->curr_gw, new_gw_node);
+	spin_unlock_bh(&bat_priv->curr_gw_lock);
 
 	if (curr_gw_node)
 		gw_node_free_ref(curr_gw_node);
@@ -78,7 +92,7 @@ static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
 void gw_election(struct bat_priv *bat_priv)
 {
 	struct hlist_node *node;
-	struct gw_node *gw_node, *curr_gw_tmp = NULL;
+	struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL;
 	uint8_t max_tq = 0;
 	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
 	int down, up;
@@ -92,19 +106,24 @@ void gw_election(struct bat_priv *bat_priv)
 	if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT)
 		return;
 
-	if (bat_priv->curr_gw)
+	rcu_read_lock();
+	curr_gw = rcu_dereference(bat_priv->curr_gw);
+	if (curr_gw) {
+		rcu_read_unlock();
 		return;
+	}
 
-	rcu_read_lock();
 	if (hlist_empty(&bat_priv->gw_list)) {
-		rcu_read_unlock();
 
-		if (bat_priv->curr_gw) {
+		if (curr_gw) {
+			rcu_read_unlock();
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Removing selected gateway - "
 				"no gateway in range\n");
 			gw_deselect(bat_priv);
 		}
+		else
+			rcu_read_unlock();
 
 		return;
 	}
@@ -153,12 +172,12 @@ void gw_election(struct bat_priv *bat_priv)
 			max_gw_factor = tmp_gw_factor;
 	}
 
-	if (bat_priv->curr_gw != curr_gw_tmp) {
-		if ((bat_priv->curr_gw) && (!curr_gw_tmp))
+	if (curr_gw != curr_gw_tmp) {
+		if ((curr_gw) && (!curr_gw_tmp))
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Removing selected gateway - "
 				"no gateway in range\n");
-		else if ((!bat_priv->curr_gw) && (curr_gw_tmp))
+		else if ((!curr_gw) && (curr_gw_tmp))
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Adding route to gateway %pM "
 				"(gw_flags: %i, tq: %i)\n",
@@ -181,11 +200,13 @@ void gw_election(struct bat_priv *bat_priv)
 
 void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 {
-	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+	struct gw_node *curr_gateway_tmp;
 	uint8_t gw_tq_avg, orig_tq_avg;
 
+	rcu_read_lock();
+	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
 	if (!curr_gateway_tmp)
-		return;
+		goto rcu_unlock;
 
 	if (!curr_gateway_tmp->orig_node)
 		goto deselect;
@@ -195,12 +216,14 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 
 	/* this node already is the gateway */
 	if (curr_gateway_tmp->orig_node == orig_node)
-		return;
+		goto rcu_unlock;
 
 	if (!orig_node->router)
-		return;
+		goto rcu_unlock;
 
 	gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg;
+	rcu_read_unlock();
+
 	orig_tq_avg = orig_node->router->tq_avg;
 
 	/* the TQ value has to be better */
@@ -222,6 +245,9 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 
 deselect:
 	gw_deselect(bat_priv);
+	return;
+rcu_unlock:
+	rcu_read_unlock();
 }
 
 static void gw_node_add(struct bat_priv *bat_priv,
@@ -278,7 +304,7 @@ void gw_node_update(struct bat_priv *bat_priv,
 				"Gateway %pM removed from gateway list\n",
 				orig_node->orig);
 
-			if (gw_node == bat_priv->curr_gw) {
+			if (gw_node == rcu_dereference(bat_priv->curr_gw)) {
 				rcu_read_unlock();
 				gw_deselect(bat_priv);
 				return;
@@ -316,7 +342,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
 		    atomic_read(&bat_priv->mesh_state) == MESH_ACTIVE)
 			continue;
 
-		if (bat_priv->curr_gw == gw_node)
+		if (rcu_dereference(bat_priv->curr_gw) == gw_node)
 			gw_deselect(bat_priv);
 
 		hlist_del_rcu(&gw_node->list);
@@ -330,12 +356,16 @@ void gw_node_purge(struct bat_priv *bat_priv)
 static int _write_buffer_text(struct bat_priv *bat_priv,
 			      struct seq_file *seq, struct gw_node *gw_node)
 {
-	int down, up;
+	struct gw_node *curr_gw;
+	int down, up, ret;
 
 	gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
 
-	return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
-		       (bat_priv->curr_gw == gw_node ? "=>" : "  "),
+	rcu_read_lock();
+	curr_gw = rcu_dereference(bat_priv->curr_gw);
+
+	ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
+		       (curr_gw == gw_node ? "=>" : "  "),
 		       gw_node->orig_node->orig,
 		       gw_node->orig_node->router->tq_avg,
 		       gw_node->orig_node->router->addr,
@@ -345,6 +375,9 @@ static int _write_buffer_text(struct bat_priv *bat_priv,
 		       (down > 2048 ? "MBit" : "KBit"),
 		       (up > 2048 ? up / 1024 : up),
 		       (up > 2048 ? "MBit" : "KBit"));
+
+	rcu_read_unlock();
+	return ret;
 }
 
 int gw_client_seq_print_text(struct seq_file *seq, void *offset)
@@ -465,8 +498,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb)
 	if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER)
 		return -1;
 
-	if (!bat_priv->curr_gw)
+	rcu_read_lock();
+	if (!rcu_dereference(bat_priv->curr_gw)) {
+		rcu_read_unlock();
 		return 0;
+	}
+	rcu_read_unlock();
 
 	return 1;
 }
diff --git a/batman-adv/main.c b/batman-adv/main.c
index 658ad5a..6823868 100644
--- a/batman-adv/main.c
+++ b/batman-adv/main.c
@@ -84,6 +84,7 @@ int mesh_init(struct net_device *soft_iface)
 	spin_lock_init(&bat_priv->hna_lhash_lock);
 	spin_lock_init(&bat_priv->hna_ghash_lock);
 	spin_lock_init(&bat_priv->gw_list_lock);
+	spin_lock_init(&bat_priv->curr_gw_lock);
 	spin_lock_init(&bat_priv->vis_hash_lock);
 	spin_lock_init(&bat_priv->vis_list_lock);
 	spin_lock_init(&bat_priv->softif_neigh_lock);
diff --git a/batman-adv/types.h b/batman-adv/types.h
index ee77d48..4ae11c3 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -162,6 +162,7 @@ struct bat_priv {
 	spinlock_t hna_lhash_lock; /* protects hna_local_hash */
 	spinlock_t hna_ghash_lock; /* protects hna_global_hash */
 	spinlock_t gw_list_lock; /* protects gw_list */
+	spinlock_t curr_gw_lock; /* protects curr_gw updates */
 	spinlock_t vis_hash_lock; /* protects vis_hash */
 	spinlock_t vis_list_lock; /* protects vis_info::recv_list */
 	spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */
-- 
1.7.2.3

Re: [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer

[Marek Lindner]

On Friday 04 February 2011 16:21:34 Linus Lüssing wrote:
> +	if (curr_gw) {
> +		rcu_read_unlock();
>  		return;
> +	}
> 
> -	rcu_read_lock();
>  	if (hlist_empty(&bat_priv->gw_list)) {
> -		rcu_read_unlock();
> 
> -		if (bat_priv->curr_gw) {
> +		if (curr_gw) {
> +			rcu_read_unlock();
>  			bat_dbg(DBG_BATMAN, bat_priv,
>  				"Removing selected gateway - "
>  				"no gateway in range\n");
>  			gw_deselect(bat_priv);
>  		}
> +		else
> +			rcu_read_unlock();

This is a bit odd here - we return if curr_gw is not NULL and later we print a 
message if curr_gw is not NULL ? 
The issue existed before your patch came along.


>  void gw_check_election(struct bat_priv *bat_priv, struct orig_node
> *orig_node) {
> -	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
> +	struct gw_node *curr_gateway_tmp;
>  	uint8_t gw_tq_avg, orig_tq_avg;
> 
> +	rcu_read_lock();
> +	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
>  	if (!curr_gateway_tmp)
> -		return;
> +		goto rcu_unlock;
> 
>  	if (!curr_gateway_tmp->orig_node)
>  		goto deselect;

It seems if we jump to "deselect" here the rcu_lock() will never be unlocked.


> @@ -316,7 +342,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
> -		if (bat_priv->curr_gw == gw_node)
> +		if (rcu_dereference(bat_priv->curr_gw) == gw_node)
>  			gw_deselect(bat_priv);

At this point we neither hold a rcu_lock() nor the newly created spinlock ...


>  	spinlock_t gw_list_lock; /* protects gw_list */
> +	spinlock_t curr_gw_lock; /* protects curr_gw updates */

Would speak anything against re-using the gw_list_lock ?

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer

[Linus Lüssing]

On Tue, Feb 08, 2011 at 02:18:37PM +0100, Marek Lindner wrote:
> 
> On Friday 04 February 2011 16:21:34 Linus Lüssing wrote:
> > +	if (curr_gw) {
> > +		rcu_read_unlock();
> >  		return;
> > +	}
> > 
> > -	rcu_read_lock();
> >  	if (hlist_empty(&bat_priv->gw_list)) {
> > -		rcu_read_unlock();
> > 
> > -		if (bat_priv->curr_gw) {
> > +		if (curr_gw) {
> > +			rcu_read_unlock();
> >  			bat_dbg(DBG_BATMAN, bat_priv,
> >  				"Removing selected gateway - "
> >  				"no gateway in range\n");
> >  			gw_deselect(bat_priv);
> >  		}
> > +		else
> > +			rcu_read_unlock();
> 
> This is a bit odd here - we return if curr_gw is not NULL and later we print a 
> message if curr_gw is not NULL ? 
> The issue existed before your patch came along.
That's right. What do you think, is it worth an extra patch as it
is not a problem which patch 5/7 does not really try to address?
> 
> 
> >  void gw_check_election(struct bat_priv *bat_priv, struct orig_node
> > *orig_node) {
> > -	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
> > +	struct gw_node *curr_gateway_tmp;
> >  	uint8_t gw_tq_avg, orig_tq_avg;
> > 
> > +	rcu_read_lock();
> > +	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
> >  	if (!curr_gateway_tmp)
> > -		return;
> > +		goto rcu_unlock;
> > 
> >  	if (!curr_gateway_tmp->orig_node)
> >  		goto deselect;
> 
> It seems if we jump to "deselect" here the rcu_lock() will never be unlocked.
True, will add that rcu_read_unlock() in the deselect jump mark.

> 
> 
> > @@ -316,7 +342,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
> > -		if (bat_priv->curr_gw == gw_node)
> > +		if (rcu_dereference(bat_priv->curr_gw) == gw_node)
> >  			gw_deselect(bat_priv);
> 
> At this point we neither hold a rcu_lock() nor the newly created spinlock ...
True again. I would prefer just adding an rcu-lock here...
> 
> 
> >  	spinlock_t gw_list_lock; /* protects gw_list */
> > +	spinlock_t curr_gw_lock; /* protects curr_gw updates */
> 
> Would speak anything against re-using the gw_list_lock ?
... as we are usually changing the gw_list more often than the
curr_gw, so it's not really necessary to let a gw_list change for
another node wait for a curr_gw_node reassignment to finish.
What is speaking for using the same lock for both, just having
less spinlocks in total in the code?
Does anyone know how it is usually done in the rest of the kernel,
are people tending to reduce the atomic context to the minimum
amount needed? Or is it usually a trade-off between two many locks
and and small atomic contexts?
> 
> Regards,
> Marek
> 
Cheers, Linus

Re: [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer

[Marek Lindner]

On Thursday 10 February 2011 11:42:50 Linus Lüssing wrote:
> > Would speak anything against re-using the gw_list_lock ?
> 
> ... as we are usually changing the gw_list more often than the
> curr_gw, so it's not really necessary to let a gw_list change for
> another node wait for a curr_gw_node reassignment to finish.

Well, we only need the list lock when we are adding/deleting items from the 
list which does not happen that often nor is it time critical.


> What is speaking for using the same lock for both, just having
> less spinlocks in total in the code?

Yes.

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer

[Linus Lüssing]

Hi Marek,

Ok, I now reused the gw_list_lock instead of introducing a new spinlock. I've left the
one issue in the beginning of gw_election() you've been mentioning untouched.
The rest should be fixed.

Cheers, Linus

[B.A.T.M.A.N.] [PATCHv2 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer

[Linus Lüssing]

The rcu protected macros rcu_dereference() and rcu_assign_pointer()
for the bat_priv->curr_gw need to be used, as well as spin/rcu locking.

Otherwise we might end up using a curr_gw pointer pointing to already
freed memory.

Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 gateway_client.c |   99 ++++++++++++++++++++++++++++++++++++++---------------
 types.h          |    2 +-
 2 files changed, 72 insertions(+), 29 deletions(-)

diff --git a/gateway_client.c b/gateway_client.c
index 517e001..b04ee56 100644
--- a/gateway_client.c
+++ b/gateway_client.c
@@ -44,19 +44,30 @@ static void gw_node_free_ref(struct gw_node *gw_node)
 
 void *gw_get_selected(struct bat_priv *bat_priv)
 {
-	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+	struct gw_node *curr_gateway_tmp;
+	struct orig_node *orig_node;
 
-	if (!curr_gateway_tmp)
+	rcu_read_lock();
+	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
+	if (!curr_gateway_tmp) {
+		rcu_read_unlock();
 		return NULL;
+	}
+
+	orig_node = curr_gateway_tmp->orig_node;
+	rcu_read_unlock();
 
-	return curr_gateway_tmp->orig_node;
+	return orig_node;
 }
 
 void gw_deselect(struct bat_priv *bat_priv)
 {
-	struct gw_node *gw_node = bat_priv->curr_gw;
+	struct gw_node *gw_node;
 
-	bat_priv->curr_gw = NULL;
+	spin_lock_bh(&bat_priv->gw_list_lock);
+	gw_node = rcu_dereference(bat_priv->curr_gw);
+	rcu_assign_pointer(bat_priv->curr_gw, NULL);
+	spin_unlock_bh(&bat_priv->gw_list_lock);
 
 	if (gw_node)
 		gw_node_free_ref(gw_node);
@@ -64,12 +75,15 @@ void gw_deselect(struct bat_priv *bat_priv)
 
 static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
 {
-	struct gw_node *curr_gw_node = bat_priv->curr_gw;
+	struct gw_node *curr_gw_node;
 
 	if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
 		new_gw_node = NULL;
 
-	bat_priv->curr_gw = new_gw_node;
+	spin_lock_bh(&bat_priv->gw_list_lock);
+	curr_gw_node = rcu_dereference(bat_priv->curr_gw);
+	rcu_assign_pointer(bat_priv->curr_gw, new_gw_node);
+	spin_unlock_bh(&bat_priv->gw_list_lock);
 
 	if (curr_gw_node)
 		gw_node_free_ref(curr_gw_node);
@@ -78,7 +92,7 @@ static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
 void gw_election(struct bat_priv *bat_priv)
 {
 	struct hlist_node *node;
-	struct gw_node *gw_node, *curr_gw_tmp = NULL;
+	struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL;
 	uint8_t max_tq = 0;
 	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
 	int down, up;
@@ -92,19 +106,24 @@ void gw_election(struct bat_priv *bat_priv)
 	if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT)
 		return;
 
-	if (bat_priv->curr_gw)
+	rcu_read_lock();
+	curr_gw = rcu_dereference(bat_priv->curr_gw);
+	if (curr_gw) {
+		rcu_read_unlock();
 		return;
+	}
 
-	rcu_read_lock();
 	if (hlist_empty(&bat_priv->gw_list)) {
-		rcu_read_unlock();
 
-		if (bat_priv->curr_gw) {
+		if (curr_gw) {
+			rcu_read_unlock();
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Removing selected gateway - "
 				"no gateway in range\n");
 			gw_deselect(bat_priv);
 		}
+		else
+			rcu_read_unlock();
 
 		return;
 	}
@@ -153,12 +172,12 @@ void gw_election(struct bat_priv *bat_priv)
 			max_gw_factor = tmp_gw_factor;
 	}
 
-	if (bat_priv->curr_gw != curr_gw_tmp) {
-		if ((bat_priv->curr_gw) && (!curr_gw_tmp))
+	if (curr_gw != curr_gw_tmp) {
+		if ((curr_gw) && (!curr_gw_tmp))
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Removing selected gateway - "
 				"no gateway in range\n");
-		else if ((!bat_priv->curr_gw) && (curr_gw_tmp))
+		else if ((!curr_gw) && (curr_gw_tmp))
 			bat_dbg(DBG_BATMAN, bat_priv,
 				"Adding route to gateway %pM "
 				"(gw_flags: %i, tq: %i)\n",
@@ -181,26 +200,34 @@ void gw_election(struct bat_priv *bat_priv)
 
 void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 {
-	struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+	struct gw_node *curr_gateway_tmp;
 	uint8_t gw_tq_avg, orig_tq_avg;
 
+	rcu_read_lock();
+	curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
 	if (!curr_gateway_tmp)
-		return;
+		goto rcu_unlock;
 
-	if (!curr_gateway_tmp->orig_node)
+	if (!curr_gateway_tmp->orig_node) {
+		rcu_read_unlock();
 		goto deselect;
+	}
 
-	if (!curr_gateway_tmp->orig_node->router)
+	if (!curr_gateway_tmp->orig_node->router) {
+		rcu_read_unlock();
 		goto deselect;
+	}
 
 	/* this node already is the gateway */
 	if (curr_gateway_tmp->orig_node == orig_node)
-		return;
+		goto rcu_unlock;
 
 	if (!orig_node->router)
-		return;
+		goto rcu_unlock;
 
 	gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg;
+	rcu_read_unlock();
+
 	orig_tq_avg = orig_node->router->tq_avg;
 
 	/* the TQ value has to be better */
@@ -222,6 +249,9 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
 
 deselect:
 	gw_deselect(bat_priv);
+	return;
+rcu_unlock:
+	rcu_read_unlock();
 }
 
 static void gw_node_add(struct bat_priv *bat_priv,
@@ -278,7 +308,7 @@ void gw_node_update(struct bat_priv *bat_priv,
 				"Gateway %pM removed from gateway list\n",
 				orig_node->orig);
 
-			if (gw_node == bat_priv->curr_gw) {
+			if (gw_node == rcu_dereference(bat_priv->curr_gw)) {
 				rcu_read_unlock();
 				gw_deselect(bat_priv);
 				return;
@@ -316,8 +346,10 @@ void gw_node_purge(struct bat_priv *bat_priv)
 		    atomic_read(&bat_priv->mesh_state) == MESH_ACTIVE)
 			continue;
 
-		if (bat_priv->curr_gw == gw_node)
-			gw_deselect(bat_priv);
+		if (rcu_dereference(bat_priv->curr_gw) == gw_node) {
+			rcu_assign_pointer(bat_priv->curr_gw, NULL);
+			gw_node_free_ref(gw_node);
+		}
 
 		hlist_del_rcu(&gw_node->list);
 		gw_node_free_ref(gw_node);
@@ -330,12 +362,16 @@ void gw_node_purge(struct bat_priv *bat_priv)
 static int _write_buffer_text(struct bat_priv *bat_priv,
 			      struct seq_file *seq, struct gw_node *gw_node)
 {
-	int down, up;
+	struct gw_node *curr_gw;
+	int down, up, ret;
 
 	gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
 
-	return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
-		       (bat_priv->curr_gw == gw_node ? "=>" : "  "),
+	rcu_read_lock();
+	curr_gw = rcu_dereference(bat_priv->curr_gw);
+
+	ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
+		       (curr_gw == gw_node ? "=>" : "  "),
 		       gw_node->orig_node->orig,
 		       gw_node->orig_node->router->tq_avg,
 		       gw_node->orig_node->router->addr,
@@ -345,6 +381,9 @@ static int _write_buffer_text(struct bat_priv *bat_priv,
 		       (down > 2048 ? "MBit" : "KBit"),
 		       (up > 2048 ? up / 1024 : up),
 		       (up > 2048 ? "MBit" : "KBit"));
+
+	rcu_read_unlock();
+	return ret;
 }
 
 int gw_client_seq_print_text(struct seq_file *seq, void *offset)
@@ -465,8 +504,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb)
 	if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER)
 		return -1;
 
-	if (!bat_priv->curr_gw)
+	rcu_read_lock();
+	if (!rcu_dereference(bat_priv->curr_gw)) {
+		rcu_read_unlock();
 		return 0;
+	}
+	rcu_read_unlock();
 
 	return 1;
 }
diff --git a/types.h b/types.h
index ee77d48..8447330 100644
--- a/types.h
+++ b/types.h
@@ -161,7 +161,7 @@ struct bat_priv {
 	spinlock_t forw_bcast_list_lock; /* protects  */
 	spinlock_t hna_lhash_lock; /* protects hna_local_hash */
 	spinlock_t hna_ghash_lock; /* protects hna_global_hash */
-	spinlock_t gw_list_lock; /* protects gw_list */
+	spinlock_t gw_list_lock; /* protects gw_list and curr_gw */
 	spinlock_t vis_hash_lock; /* protects vis_hash */
 	spinlock_t vis_list_lock; /* protects vis_info::recv_list */
 	spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */
-- 
1.7.2.3

Re: [B.A.T.M.A.N.] [PATCHv2 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer

[Marek Lindner]

On Saturday 12 February 2011 22:21:38 Linus Lüssing wrote:
> The rcu protected macros rcu_dereference() and rcu_assign_pointer()
> for the bat_priv->curr_gw need to be used, as well as spin/rcu locking.
> 
> Otherwise we might end up using a curr_gw pointer pointing to already
> freed memory.

Applied in revision 1941.

Thanks,
Marek

[B.A.T.M.A.N.] [PATCH 6/7] batman-adv: add __rcu annotations for gw_node

[Linus Lüssing]

Add __rcu annotations for the rcu protected bat_priv::curr_gw pointer to
allow sparse checking.

Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 compat.h |    6 ++++++
 types.h  |    2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/batman-adv/compat.h b/batman-adv/compat.h
index a76d0be..4e89049 100644
--- a/batman-adv/compat.h
+++ b/batman-adv/compat.h
@@ -270,4 +270,10 @@ int bat_seq_printf(struct seq_file *m, const char *f, ...);
 
 #endif /* < KERNEL_VERSION(2, 6, 33) */
 
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 36)
+
+#define __rcu
+
+#endif /* < KERNEL_VERSION(2, 6, 36) */
+
 #endif /* _NET_BATMAN_ADV_COMPAT_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 4ae11c3..4dc5854 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -171,7 +171,7 @@ struct bat_priv {
 	struct delayed_work hna_work;
 	struct delayed_work orig_work;
 	struct delayed_work vis_work;
-	struct gw_node *curr_gw;
+	struct gw_node __rcu *curr_gw;  /* rcu protected pointer */
 	struct vis_info *my_vis_info;
 };
 
-- 
1.7.2.3

Re: [B.A.T.M.A.N.] [PATCH 6/7] batman-adv: add __rcu annotations for gw_node

[Marek Lindner]

On Friday 04 February 2011 16:21:35 Linus Lüssing wrote:
> Add __rcu annotations for the rcu protected bat_priv::curr_gw pointer to
> allow sparse checking.

Applied in revision 1942.

Thanks,
Marek

[B.A.T.M.A.N.] [PATCH 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock

[Linus Lüssing]

When unicast_send_skb() is increasing the orig_node's refcount another
thread might have been freeing this orig_node already. We need to
increase the refcount in the rcu read lock protected area to avoid that.

Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 gateway_client.c |    3 +++
 unicast.c        |    1 -
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 4624515..b3cda22 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -55,6 +55,9 @@ void *gw_get_selected(struct bat_priv *bat_priv)
 	}
 
 	orig_node = curr_gateway_tmp->orig_node;
+	if (orig_node)
+		kref_get(&orig_node->refcount);
+
 	rcu_read_unlock();
 
 	return orig_node;
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 580b547..f4f5115 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
 		if (!orig_node)
 			goto trans_search;
 
-		kref_get(&orig_node->refcount);
 		goto find_router;
 	} else {
 		rcu_read_lock();
-- 
1.7.2.3

[B.A.T.M.A.N.] [PATCHv2 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock

[Linus Lüssing]

When unicast_send_skb() is increasing the orig_node's refcount another
thread might have been freeing this orig_node already. We need to
increase the refcount in the rcu read lock protected area to avoid that.

The same is true for get_orig_node().

Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 gateway_client.c |    3 +++
 originator.c     |    4 ++--
 unicast.c        |    1 -
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 4624515..b3cda22 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -55,6 +55,9 @@ void *gw_get_selected(struct bat_priv *bat_priv)
 	}
 
 	orig_node = curr_gateway_tmp->orig_node;
+	if (orig_node)
+		kref_get(&orig_node->refcount);
+
 	rcu_read_unlock();
 
 	return orig_node;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index bde9778..6fb8393 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -193,12 +193,12 @@ struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr)
 	orig_node = ((struct orig_node *)hash_find(bat_priv->orig_hash,
 						   compare_orig, choose_orig,
 						   addr));
-	rcu_read_unlock();
-
 	if (orig_node) {
 		kref_get(&orig_node->refcount);
+		rcu_read_unlock();
 		return orig_node;
 	}
+	rcu_read_unlock();
 
 	bat_dbg(DBG_BATMAN, bat_priv,
 		"Creating new originator: %pM\n", addr);
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 580b547..f4f5115 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
 		if (!orig_node)
 			goto trans_search;
 
-		kref_get(&orig_node->refcount);
 		goto find_router;
 	} else {
 		rcu_read_lock();
-- 
1.7.2.3

Re: [B.A.T.M.A.N.] [PATCH 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock

[Marek Lindner]

On Friday 04 February 2011 16:21:36 Linus Lüssing wrote:
> When unicast_send_skb() is increasing the orig_node's refcount another
> thread might have been freeing this orig_node already. We need to
> increase the refcount in the rcu read lock protected area to avoid that.

Applied in revision 1943.

Thanks,
Marek

Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node

[jay.busch]

Sent from my HTC PURE™, a Windows® phone from AT&T

-----Original Message-----
From: Marek Lindner <lindner_marek@yahoo.de>
Sent: Wednesday, February 02, 2011 2:49 PM
To: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n@lists.open-mesh.org>
Subject: Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node

On Wednesday 02 February 2011 18:37:18 Linus Lüssing wrote:
> So after some more discussions with Marek and Sven, it looks like we
> have to use the rcu protected macros rcu_dereference() and
> rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node.
> 
> Changes here also include moving the kref_get() from unicast_send_skb()
> into gw_get_selected(). The orig_node could have been freed already at
> the time the kref_get() was called in unicast_send_skb().

I'd suggest you make a standalone patch because the patches address different 
problems.

Thanks,
Marek