Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: remove obsolete deleted attribute for gateway node

[Marek Lindner <mareklindner@neomailbox.ch>]

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

On Friday, June 26, 2015 13:11:25 Simon Wunderlich wrote:
> @@ -537,14 +530,26 @@ void batadv_gw_node_update(struct batadv_priv
> *bat_priv, /* Note: We don't need a NULL check here, since curr_gw never
>  		 * gets dereferenced.
>  		 */
> +		spin_lock_bh(&bat_priv->gw.list_lock);
> +		hlist_for_each_entry_safe(gw_node_tmp, node_tmp,
> +					  &bat_priv->gw.list, list) {
> +			if (gw_node_tmp != gw_node)
> +				continue;
> +
> +			hlist_del_rcu(&gw_node->list);
> +			batadv_gw_node_free_ref(gw_node);
> +		}
> +		spin_unlock_bh(&bat_priv->gw.list_lock);

An entire hlist_for_each_entry_safe() loop isn't necessary. 
hlist_del_init_rcu() should suffice.


>  		curr_gw = batadv_gw_get_selected_gw_node(bat_priv);
>  		if (gw_node == curr_gw)
>  			batadv_gw_reselect(bat_priv);
> +
> +		if (curr_gw)
> +			batadv_gw_node_free_ref(curr_gw);
>  	}
> 
>  out:
> -	if (curr_gw)
> -		batadv_gw_node_free_ref(curr_gw);
>  	if (gw_node)
>  		batadv_gw_node_free_ref(gw_node);
>  }

After the batadv_gw_node_free_ref() 'bat_priv->gw.curr_gw' points to random 
memory ...



> @@ -562,37 +567,21 @@ void batadv_gw_node_delete(struct batadv_priv
> *bat_priv,
> 
>  void batadv_gw_node_purge(struct batadv_priv *bat_priv)
>  {
> -	struct batadv_gw_node *gw_node, *curr_gw;
> +	struct batadv_gw_node *gw_node;
>  	struct hlist_node *node_tmp;
> -	unsigned long timeout = msecs_to_jiffies(2 * BATADV_PURGE_TIMEOUT);
> -	int do_reselect = 0;
> 
> -	curr_gw = batadv_gw_get_selected_gw_node(bat_priv);
> +	if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_ACTIVE)
> +		return;

Why checking for BATADV_MESH_ACTIVE if the function only ever gets called from 
batadv_mesh_free() which sets BATADV_MESH_DEACTIVATING ?

If you change the meaning of the function you could also rename it to 
batadv_gw_node_free() to be consistent with the other cleanup functions we 
have.



>  	spin_lock_bh(&bat_priv->gw.list_lock);
> -
>  	hlist_for_each_entry_safe(gw_node, node_tmp,
>  				  &bat_priv->gw.list, list) {
> -		if (((!gw_node->deleted) ||
> -		     (time_before(jiffies, gw_node->deleted + timeout))) &&
> -		    atomic_read(&bat_priv->mesh_state) == BATADV_MESH_ACTIVE)
> -			continue;
> -
> -		if (curr_gw == gw_node)
> -			do_reselect = 1;
> 
>  		hlist_del_rcu(&gw_node->list);
>  		batadv_gw_node_free_ref(gw_node);
>  	}
> 
>  	spin_unlock_bh(&bat_priv->gw.list_lock);
> -
> -	/* gw_reselect() needs to acquire the gw_list_lock */
> -	if (do_reselect)
> -		batadv_gw_reselect(bat_priv);
> -
> -	if (curr_gw)
> -		batadv_gw_node_free_ref(curr_gw);
>  }

At this point 'bat_priv->gw.curr_gw' points to random memory if I am not 
mistaken.

Cheers,
Marek

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