Re: [B.A.T.M.A.N.] [PATCHv2 1/7] batman-adv: tvlv - basic infrastructure

Sven Eckelmann
2015-06-10T08:57:14Z

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

On Tuesday 23 April 2013 21:39:57 Marek Lindner wrote:
> +/**
> + * batadv_tvlv_handler_unregister - unregister tvlv handler based on the
> + * provided type and version (both need to match)
> + * @bat_priv: the bat priv with all the soft interface information
> + * @type: tvlv handler type to be unregistered
> + * @version: tvlv handler version to be unregistered
> + */
> +void batadv_tvlv_handler_unregister(struct batadv_priv *bat_priv,
> + uint8_t type, uint8_t version)
> +{
> + struct batadv_tvlv_handler *tvlv_handler;
> +
> + tvlv_handler = batadv_tvlv_handler_get(bat_priv, type, version);
> + if (!tvlv_handler)
> + return;
> +
> + batadv_tvlv_handler_free_ref(tvlv_handler);
> + spin_lock_bh(&bat_priv->tvlv.handler_list_lock);
> + hlist_del_rcu(&tvlv_handler->list);
> + spin_unlock_bh(&bat_priv->tvlv.handler_list_lock);
> + batadv_tvlv_handler_free_ref(tvlv_handler);
> +}

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