batman-adv: avoid potential race condition when adding a new neighbour
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
@@ -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
@@ -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
@@ -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
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
-1spin_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
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
-1spin_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