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);}
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