Re: [B.A.T.M.A.N.] [PATCHv2 1/4] batman-adv: add list of unique single hop neighbors per hard-interface

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2113 bytes --]

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

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