Skip to contentSkip to conversation

batman-adv: avoid potential race condition when adding a new neighbour

PATCH Superseded Antonio Quartulli 2 participants 3 messages Jan 24, 2014 – Jan 27, 2014

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 batman-adv: avoid potential race condition when adding a new neighbour

From: Antonio Quartulli <[email protected]>

When adding a new neighbour it is important to atomically perform the following:
- check if the neighbour already exists
- append the neighbour to the proper list

If the two operations are not performed in an atomic context it is possible that two concurrent insertions add the same neighbour twice.

---
 bat_iv_ogm.c | 21 +++++++++++++++------
 originator.c | 36 ++++++++++++++++++++++++++++++++++++
 originator.h |  4 ++++
 3 files changed, 55 insertions(+), 6 deletions(-)
bat_iv_ogm.c +15 -6
index 512159b..c6eac80 100644
@@ -266,7 +266,7 @@ batadv_iv_ogm_neigh_new(struct batadv_hard_iface *hard_iface,
 			struct batadv_orig_node *orig_neigh)
 {
 	struct batadv_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
-	struct batadv_neigh_node *neigh_node;
+	struct batadv_neigh_node *neigh_node = NULL, *tmp_neigh_node;
 
 	neigh_node = batadv_neigh_node_new(hard_iface, neigh_addr, orig_node);
 	if (!neigh_node)
@@ -281,14 +281,23 @@ batadv_iv_ogm_neigh_new(struct batadv_hard_iface *hard_iface,
 	neigh_node->orig_node = orig_neigh;
 	neigh_node->if_incoming = hard_iface;
 
-	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
-		   "Creating new neighbor %pM for orig_node %pM on interface %s\n",
-		   neigh_addr, orig_node->orig, hard_iface->net_dev->name);
-
 	spin_lock_bh(&orig_node->neigh_list_lock);
-	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
+	tmp_neigh_node = batadv_neigh_node_get(orig_node, hard_iface,
+					       neigh_addr);
+	if (!tmp_neigh_node) {
+		hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
+	} else {
+		kfree(neigh_node);
+		neigh_node = tmp_neigh_node;
+	}
 	spin_unlock_bh(&orig_node->neigh_list_lock);
 
+	if (!tmp_neigh_node)
+		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
+			   "Creating new neighbor %pM for orig_node %pM on interface %s\n",
+			   neigh_addr, orig_node->orig,
+			   hard_iface->net_dev->name);
+
 out:
 	return neigh_node;
 }
originator.c +36 -0
index 6df12a2..bb07c92 100644
@@ -458,6 +458,42 @@ out:
 }
 
 /**
+ * batadv_neigh_node_get - retrieve a neighbour from the list
+ * @orig_node: originator which the neighbour belongs to
+ * @hard_iface: the interface where this neighbour is connected to
+ * @addr: the address of the neighbour
+ *
+ * Looks for and possibly return a neighbour belonging to this originator list
+ * which is connected through the provided hard interface.
+ * Returns NULL if the neighbour is not found.
+ */
+struct batadv_neigh_node *
+batadv_neigh_node_get(const struct batadv_orig_node *orig_node,
+		      const struct batadv_hard_iface *hard_iface,
+		      const uint8_t *addr)
+{
+	struct batadv_neigh_node *tmp_neigh_node, *res = NULL;
+
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(tmp_neigh_node, &orig_node->neigh_list, list) {
+		if (!batadv_compare_eth(tmp_neigh_node->addr, addr))
+			continue;
+
+		if (tmp_neigh_node->if_incoming != hard_iface)
+			continue;
+
+		if (!atomic_inc_not_zero(&tmp_neigh_node->refcount))
+			continue;
+
+		res = tmp_neigh_node;
+		break;
+	}
+	rcu_read_unlock();
+
+	return res;
+}
+
+/**
  * batadv_orig_ifinfo_free_rcu - free the orig_ifinfo object
  * @rcu: rcu pointer of the orig_ifinfo object
  */
originator.h +4 -0
index 37be290..db3a9ed 100644
@@ -29,6 +29,10 @@ void batadv_orig_node_free_ref_now(struct batadv_orig_node *orig_node);
 struct batadv_orig_node *batadv_orig_node_new(struct batadv_priv *bat_priv,
 					      const uint8_t *addr);
 struct batadv_neigh_node *
+batadv_neigh_node_get(const struct batadv_orig_node *orig_node,
+		      const struct batadv_hard_iface *hard_iface,
+		      const uint8_t *addr);
+struct batadv_neigh_node *
 batadv_neigh_node_new(struct batadv_hard_iface *hard_iface,
 		      const uint8_t *neigh_addr,
 		      struct batadv_orig_node *orig_node);
Signature · 1 line
1.8.5.3
#1 lore raw
Quoted 8 lines · On Friday 24 January 2014 09:58:03 Antonio Quartulli wrote:
bat_iv_ogm.c +0 -4
@@ -281,14 +281,23 @@ batadv_iv_ogm_neigh_new(struct batadv_hard_iface
 *hard_iface, neigh_node->orig_node = orig_neigh;
 	neigh_node->if_incoming = hard_iface;
 
-	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
-		   "Creating new neighbor %pM for orig_node %pM on interface %s\n",
-		   neigh_addr, orig_node->orig, hard_iface->net_dev->name);
-

bat_iv_ogm.c batadv_iv_ogm_neigh_new(struct batadv_hard_iface · line 292

Quoted 10 lines
bat_iv_ogm.c +8 -1
 	spin_lock_bh(&orig_node->neigh_list_lock);
-	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
+	tmp_neigh_node = batadv_neigh_node_get(orig_node, hard_iface,
+					       neigh_addr);
+	if (!tmp_neigh_node) {
+		hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
+	} else {
+		kfree(neigh_node);
+		neigh_node = tmp_neigh_node;
+	}

What about hard_iface->refcount ?

Quoted 8 lines
originator.c +6 -0
 /**
+ * batadv_neigh_node_get - retrieve a neighbour from the list
+ * @orig_node: originator which the neighbour belongs to
+ * @hard_iface: the interface where this neighbour is connected to
+ * @addr: the address of the neighbour
+ *
+ * Looks for and possibly return a neighbour belonging to this originator

list + * which is connected through the provided hard interface.

'Looks and return' does not work. Either 'look and return' or 'looks and returns'.

Cheers,
Marek

#2 lore raw
Quoted 5 lines · Marek Lindner
Quoted 4 lines · On Friday 24 January 2014 09:58:03 Antonio Quartulli wrote:
bat_iv_ogm.c +0 -0
@@ -281,14 +281,23 @@ batadv_iv_ogm_neigh_new(struct batadv_hard_iface
 *hard_iface, neigh_node->orig_node = orig_neigh;
 	neigh_node->if_incoming = hard_iface;
 
Quoted 16 lines
Quoted 4 lines
bat_iv_ogm.c +0 -4
-	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
-		   "Creating new neighbor %pM for orig_node %pM on interface %s\n",
-		   neigh_addr, orig_node->orig, hard_iface->net_dev->name);
-

bat_iv_ogm.c batadv_iv_ogm_neigh_new(struct batadv_hard_iface *hard_iface, · line 292

Quoted 10 lines
bat_iv_ogm.c +8 -1
 	spin_lock_bh(&orig_node->neigh_list_lock);
-	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
+	tmp_neigh_node = batadv_neigh_node_get(orig_node, hard_iface,
+					       neigh_addr);
+	if (!tmp_neigh_node) {
+		hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
+	} else {
+		kfree(neigh_node);
+		neigh_node = tmp_neigh_node;
+	}

What about hard_iface->refcount ?

True.
hard_iface must be released if we found an already existent neighbour. I'll put a batadv_hardif_free_ref(hard_iface); right after the kfree(neigh_node);.

Quoted 11 lines
Quoted 10 lines
originator.c +6 -0
 /**
+ * batadv_neigh_node_get - retrieve a neighbour from the list
+ * @orig_node: originator which the neighbour belongs to
+ * @hard_iface: the interface where this neighbour is connected to
+ * @addr: the address of the neighbour
+ *
+ * Looks for and possibly return a neighbour belonging to this originator

list + * which is connected through the provided hard interface.

Quoted 2 lines

'Looks and return' does not work. Either 'look and return' or 'looks and returns'.

Yap, thanks!

I'll send v2 soon!

Signature · 1 line
Antonio Quartulli