Re: [B.A.T.M.A.N.] [PATCHv14 5/6] batman-adv: Add IPv4 link-local/IPv6-ll-all-nodes multicast support

[Sven Eckelmann <sven@narfation.org>]

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

On Saturday 15 February 2014 17:47:53 Linus Lüssing wrote:
> +static void batadv_mcast_want_unsnoop_update(struct batadv_priv *bat_priv,
> +					     struct batadv_orig_node *orig,
> +					     uint8_t mcast_flags)
> +{
> +	/* switched from flag unset to set */
> +	if (mcast_flags & BATADV_MCAST_WANT_ALL_UNSNOOPABLES &&
> +	    !(orig->mcast_flags & BATADV_MCAST_WANT_ALL_UNSNOOPABLES)) {
> +		atomic_inc(&bat_priv->mcast.num_want_all_unsnoopables);
> +
> +		spin_lock_bh(&bat_priv->mcast.want_lists_lock);
> +		hlist_add_head_rcu(&orig->mcast_want_all_unsnoopables_node,
> +				   &bat_priv->mcast.want_all_unsnoopables_list);
> +		spin_unlock_bh(&bat_priv->mcast.want_lists_lock);
> +	/* switched from flag set to unset */
> +	} else if (!(mcast_flags & BATADV_MCAST_WANT_ALL_UNSNOOPABLES) &&
> +		   orig->mcast_flags & BATADV_MCAST_WANT_ALL_UNSNOOPABLES) {
> +		atomic_dec(&bat_priv->mcast.num_want_all_unsnoopables);
> +
> +		spin_lock_bh(&bat_priv->mcast.want_lists_lock);
> +		hlist_del_rcu(&orig->mcast_want_all_unsnoopables_node);
> +		spin_unlock_bh(&bat_priv->mcast.want_lists_lock);
> +	}

Looks unsafe. How do you make sure that it is still in the list and not 
already already removed in the meantime? hlist_del_rcu does not re-initialize 
the pointers (only POISON them when CONFIG_DEBUG_LIST is activated). Thus 
calling hlist_del_rcu again on an object not in the list either kills other 
data structures or causes an Oops.

There are more *list_del* related problems of that type in your code. This is 
just one example I've picked.

See http://www.open-mesh.org/issues/217#note-5

Kind regards,
	Sven

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