The list_del() call was changed to hlist_del_init() to allow take advantage of the hlist_unhashed() check prior to deletion in batadv_tt_req_node_new().
Signed-off-by: Marek Lindner mareklindner@neomailbox.ch --- v2: removed redundant hlist_unhashed() check & reword commit message
net/batman-adv/main.c | 2 +- net/batman-adv/translation-table.c | 28 ++++++++++++++++------------ net/batman-adv/types.h | 4 ++-- 3 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c index 40750cb..d277ba7 100644 --- a/net/batman-adv/main.c +++ b/net/batman-adv/main.c @@ -148,7 +148,7 @@ int batadv_mesh_init(struct net_device *soft_iface) INIT_HLIST_HEAD(&bat_priv->mcast.want_all_ipv6_list); #endif INIT_LIST_HEAD(&bat_priv->tt.changes_list); - INIT_LIST_HEAD(&bat_priv->tt.req_list); + INIT_HLIST_HEAD(&bat_priv->tt.req_list); INIT_LIST_HEAD(&bat_priv->tt.roam_list); #ifdef CONFIG_BATMAN_ADV_MCAST INIT_HLIST_HEAD(&bat_priv->mcast.mla_list); diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 8ef27bb..7971427 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -2220,12 +2220,13 @@ static u32 batadv_tt_local_crc(struct batadv_priv *bat_priv,
static void batadv_tt_req_list_free(struct batadv_priv *bat_priv) { - struct batadv_tt_req_node *node, *safe; + struct batadv_tt_req_node *node; + struct hlist_node *safe;
spin_lock_bh(&bat_priv->tt.req_list_lock);
- list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { - list_del(&node->list); + hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { + hlist_del_init(&node->list); kfree(node); }
@@ -2255,13 +2256,14 @@ static void batadv_tt_save_orig_buffer(struct batadv_priv *bat_priv,
static void batadv_tt_req_purge(struct batadv_priv *bat_priv) { - struct batadv_tt_req_node *node, *safe; + struct batadv_tt_req_node *node; + struct hlist_node *safe;
spin_lock_bh(&bat_priv->tt.req_list_lock); - list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { + hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { if (batadv_has_timed_out(node->issued_at, BATADV_TT_REQUEST_TIMEOUT)) { - list_del(&node->list); + hlist_del_init(&node->list); kfree(node); } } @@ -2283,7 +2285,7 @@ batadv_tt_req_node_new(struct batadv_priv *bat_priv, struct batadv_tt_req_node *tt_req_node_tmp, *tt_req_node = NULL;
spin_lock_bh(&bat_priv->tt.req_list_lock); - list_for_each_entry(tt_req_node_tmp, &bat_priv->tt.req_list, list) { + hlist_for_each_entry(tt_req_node_tmp, &bat_priv->tt.req_list, list) { if (batadv_compare_eth(tt_req_node_tmp, orig_node) && !batadv_has_timed_out(tt_req_node_tmp->issued_at, BATADV_TT_REQUEST_TIMEOUT)) @@ -2297,7 +2299,7 @@ batadv_tt_req_node_new(struct batadv_priv *bat_priv, ether_addr_copy(tt_req_node->addr, orig_node->orig); tt_req_node->issued_at = jiffies;
- list_add(&tt_req_node->list, &bat_priv->tt.req_list); + hlist_add_head(&tt_req_node->list, &bat_priv->tt.req_list); unlock: spin_unlock_bh(&bat_priv->tt.req_list_lock); return tt_req_node; @@ -2548,7 +2550,8 @@ out: batadv_hardif_free_ref(primary_if); if (ret && tt_req_node) { spin_lock_bh(&bat_priv->tt.req_list_lock); - list_del(&tt_req_node->list); + /* hlist_del_init() verifies tt_req_node still is in the list */ + hlist_del_init(&tt_req_node->list); spin_unlock_bh(&bat_priv->tt.req_list_lock); kfree(tt_req_node); } @@ -2944,7 +2947,8 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv, struct batadv_tvlv_tt_data *tt_data, u8 *resp_src, u16 num_entries) { - struct batadv_tt_req_node *node, *safe; + struct batadv_tt_req_node *node; + struct hlist_node *safe; struct batadv_orig_node *orig_node = NULL; struct batadv_tvlv_tt_change *tt_change; u8 *tvlv_ptr = (u8 *)tt_data; @@ -2982,10 +2986,10 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,
/* Delete the tt_req_node from pending tt_requests list */ spin_lock_bh(&bat_priv->tt.req_list_lock); - list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { + hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { if (!batadv_compare_eth(node->addr, resp_src)) continue; - list_del(&node->list); + hlist_del_init(&node->list); kfree(node); }
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index da4c738..76fe31f 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -537,7 +537,7 @@ struct batadv_priv_tt { struct list_head changes_list; struct batadv_hashtable *local_hash; struct batadv_hashtable *global_hash; - struct list_head req_list; + struct hlist_head req_list; struct list_head roam_list; spinlock_t changes_list_lock; /* protects changes */ spinlock_t req_list_lock; /* protects req_list */ @@ -1006,7 +1006,7 @@ struct batadv_tt_change_node { struct batadv_tt_req_node { u8 addr[ETH_ALEN]; unsigned long issued_at; - struct list_head list; + struct hlist_node list; };
/**
On Friday 19 June 2015 21:50:27 Marek Lindner wrote:
The list_del() call was changed to hlist_del_init() to allow take advantage of the hlist_unhashed() check prior to deletion in batadv_tt_req_node_new().
Signed-off-by: Marek Lindner mareklindner@neomailbox.ch
v2: removed redundant hlist_unhashed() check & reword commit message
list_del_init would also work. It is not necessary to switch to hlists for this feature. It is not possible with RCU but this is not used here.
list_del_init doesn't have a special "list_empty" or (not really existing) "list_unhashed". It is not required because list_del_init on an initialized list_head which is not in a list will only access this list_head and end up with the same list_head.
The problem with the old code is unknown state of the tt_req_node in batadv_send_tt_request. This node could either be in the list (valid state) or already be deleted from it. The deletion state had prev and next pointer of this node in an undefined state (either pointing to nodes in its old list, "random" memory regions or at poisoned addresses). This can be avoided by using list_del_init + locking for this list. All correctly initialized nodes will then either be inside a list or have prev+next pointing to itself. Calling again list_del_init on a node which has prev+next pointing to itself is safe and will neither change the state of the node or other objects.
Kind regards, Sven
[...]
- list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
list_del(&node->list);
- hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
hlist_del_init(&node->list);
This would only need a change to list_del_init
[...]
spin_lock_bh(&bat_priv->tt.req_list_lock);
- list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
- hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { if (batadv_has_timed_out(node->issued_at, BATADV_TT_REQUEST_TIMEOUT)) {
list_del(&node->list);
hlist_del_init(&node->list);
Same here
[...]
@@ -2548,7 +2550,8 @@ out: batadv_hardif_free_ref(primary_if); if (ret && tt_req_node) { spin_lock_bh(&bat_priv->tt.req_list_lock);
list_del(&tt_req_node->list);
/* hlist_del_init() verifies tt_req_node still is in the list */hlist_del_init(&tt_req_node->list);
And here (and this is most likely the only reason why you do all this).
[...]
@@ -2982,10 +2986,10 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,
/* Delete the tt_req_node from pending tt_requests list */ spin_lock_bh(&bat_priv->tt.req_list_lock);
- list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
- hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { if (!batadv_compare_eth(node->addr, resp_src)) continue;
list_del(&node->list);
hlist_del_init(&node->list);
And here
On Saturday, June 20, 2015 10:26:31 Sven Eckelmann wrote:
On Friday 19 June 2015 21:50:27 Marek Lindner wrote:
The list_del() call was changed to hlist_del_init() to allow take advantage of the hlist_unhashed() check prior to deletion in batadv_tt_req_node_new().
Signed-off-by: Marek Lindner mareklindner@neomailbox.ch
v2: removed redundant hlist_unhashed() check & reword commit message
list_del_init would also work. It is not necessary to switch to hlists for this feature. It is not possible with RCU but this is not used here.
I am aware of that fact. Still, I'd like to convert the list to hlist. If you prefer I can do this in 2 steps (first changing to list_del_init() and then changing everything to hlist).
Cheers, Marek
b.a.t.m.a.n@lists.open-mesh.org
-
Marek Lindner -
Sven Eckelmann