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