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

Sven Eckelmann
2015-06-10T08:52:13Z

[-- 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 --]