Hi,
On Sunday 26 July 2015 16:27:31 Marek Lindner wrote:
+static struct batadv_hardif_neigh_node * +batadv_hardif_neigh_get_or_create(struct batadv_hard_iface *hard_iface,
const u8 *neigh_addr)+{
- struct batadv_hardif_neigh_node *hardif_neigh = NULL;
- hardif_neigh = batadv_hardif_neigh_get(hard_iface, neigh_addr);
- if (hardif_neigh)
return hardif_neigh;- if (!atomic_inc_not_zero(&hard_iface->refcount))
return NULL;- hardif_neigh = kzalloc(sizeof(*hardif_neigh), GFP_ATOMIC);
- if (!hardif_neigh) {
batadv_hardif_free_ref(hard_iface);return NULL;- }
- INIT_HLIST_NODE(&hardif_neigh->list);
- ether_addr_copy(hardif_neigh->addr, neigh_addr);
- hardif_neigh->if_incoming = hard_iface;
- hardif_neigh->last_seen = jiffies;
- atomic_set(&hardif_neigh->refcount, 1);
- spin_lock_bh(&hard_iface->neigh_list_lock);
- hlist_add_head(&hardif_neigh->list, &hard_iface->neigh_list);
- spin_unlock_bh(&hard_iface->neigh_list_lock);
- return hardif_neigh;
+}
This can be the cause of inconsistencies [1] when two different contexts call this function at the same time when no member with this mac is stored in the list. Two nodes may then be added to the list. This should not a deal breaker because the second node will time out.
Still, we may use a better check:
create() { hardif_neigh = kmalloc(); if (!hardif_neigh) return NULL;
spin_lock_bh(&hard_iface->neigh_list_lock); hardif_neigh = batadv_hardif_neigh_get(hard_iface, neigh_addr); if (hardif_neigh) goto out;
hardif_neigh = kmalloc(); if (!hardif_neigh) goto out;
hardif_neigh->init = something; list_add_rcu(....);
out: spin_unlock_bh(&hard_iface->neigh_list_lock); return hardif_neigh; }
get_or_create() { hardif_neigh = batadv_hardif_neigh_get(hard_iface, neigh_addr); if (hardif_neigh) return hardif_neigh;
hardif_neigh = create(); return hardif_neigh; }
Kind regards, Sven
[1] https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2015-June/013345.html