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

[Sven Eckelmann <sven@narfation.org>]

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