diff mbox

[v2] batman-adv: protect tt request from double deletion

Message ID 1434721827-4935-1-git-send-email-mareklindner@neomailbox.ch (mailing list archive)
State Superseded, archived
Headers

Commit Message

Marek Lindner June 19, 2015, 1:50 p.m. UTC 
  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(-)

Comments

Sven Eckelmann June 20, 2015, 8:26 a.m. UTC | #1 
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);
>  		spin_unlock_bh(&bat_priv->tt.req_list_lock);
>  		kfree(tt_req_node);
>  	}

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
Marek Lindner June 20, 2015, 10:14 a.m. UTC | #2 
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
diff mbox

Patch

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;
 };
 
 /**