From patchwork Thu Feb 3 17:03:57 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Lindner X-Patchwork-Id: 798 Return-Path: Received: from nm18-vm0.bullet.mail.ukl.yahoo.com (nm18-vm0.bullet.mail.ukl.yahoo.com [217.146.183.95]) by open-mesh.org (Postfix) with SMTP id 9A9531545C8 for ; Thu, 3 Feb 2011 18:08:08 +0100 (CET) Received: from [217.146.183.212] by nm18.bullet.mail.ukl.yahoo.com with NNFMP; 03 Feb 2011 17:08:07 -0000 Received: from [77.238.184.75] by tm5.bullet.mail.ukl.yahoo.com with NNFMP; 03 Feb 2011 17:08:07 -0000 Received: from [127.0.0.1] by smtp144.mail.ukl.yahoo.com with NNFMP; 03 Feb 2011 17:08:07 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.de; s=s1024; t=1296752887; bh=Wp3Zr0HhxzX50dom8N3r9ap1v66uI/7+HM+UQnd+ETU=; h=X-Yahoo-Newman-Id:Received:X-Yahoo-SMTP:X-YMail-OSG:X-Yahoo-Newman-Property:From:To:Cc:Subject:Date:Message-Id:X-Mailer:In-Reply-To:References; b=OxOLtitG547kFjSaSVoTako9+FLNajhNOhYgacexWoA46uxTADRxKZjmrY5j/Ff2ee8K8G/BJE6vyTcMWl7PRwXuWGQXVQDjIMZvObTQa1yvxUVpzoA+h/PGmrv13t1Wz+cCUhN2nN7iYZoBrCRrBz5RDXqocMLt6Rb6HtIfsLc= X-Yahoo-Newman-Id: 727201.39622.bm@smtp144.mail.ukl.yahoo.com Received: from localhost (lindner_marek@81.57.254.118 with plain) by smtp144.mail.ukl.yahoo.com with SMTP; 03 Feb 2011 17:08:03 +0000 GMT X-Yahoo-SMTP: tW.h3tiswBBMXO2coYcbPigGD5Lt6zY_.Zc- X-YMail-OSG: sXgdrL8VM1mXMjuZ_iQUkPVaJn6B9Jm6BSPyXNKjqy6D5jr uhaCIijf2qzUy78IBbjyf3QyBm.Q.I.ip6A7F2s6fp3LyGj.h4eU1pVoULaT lGTcyFyN9kC4OLMzp0UZzPGz1UQ5dXSzfC9T15MFqnq7RLJej761YHGAiec. 3GMiH8prORDb1gd9uUienFCb49rYmqCmqsrft0V6677f3Eq8QIYLwRjRXvg7 IQ2lEe2iPKglxAKvHU7wK7EOGT7mxe6Ni8nzn1Buvif1aGpsED_JxCeumlV4 PgvpJ_em1VlI0.YY- X-Yahoo-Newman-Property: ymail-3 From: Marek Lindner To: b.a.t.m.a.n@lists.open-mesh.org Date: Thu, 3 Feb 2011 18:03:57 +0100 Message-Id: <1296752637-4646-4-git-send-email-lindner_marek@yahoo.de> X-Mailer: git-send-email 1.7.2.3 In-Reply-To: <201102031802.52134.lindner_marek@yahoo.de> References: <201102031802.52134.lindner_marek@yahoo.de> Cc: Marek Lindner Subject: [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node X-BeenThere: b.a.t.m.a.n@lists.open-mesh.org X-Mailman-Version: 2.1.11 Precedence: list Reply-To: The list for a Better Approach To Mobile Ad-hoc Networking List-Id: The list for a Better Approach To Mobile Ad-hoc Networking List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Feb 2011 17:08:08 -0000 From: Sven Eckelmann It might be possible that 2 threads access the same data in the same rcu grace period. The first thread calls call_rcu() to decrement the refcount and free the data while the second thread increases the refcount to use the data. To avoid this race condition all refcount operations have to be atomic. Reported-by: Sven Eckelmann Signed-off-by: Marek Lindner --- batman-adv/icmp_socket.c | 7 ++- batman-adv/originator.c | 26 ++++--------- batman-adv/originator.h | 3 +- batman-adv/routing.c | 93 ++++++++++++++++++++++++++++++++------------- batman-adv/types.h | 3 +- batman-adv/unicast.c | 2 +- batman-adv/vis.c | 8 +++- 7 files changed, 87 insertions(+), 55 deletions(-) diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c index 10a969c..046f976 100644 --- a/batman-adv/icmp_socket.c +++ b/batman-adv/icmp_socket.c @@ -230,10 +230,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff, kref_get(&orig_node->refcount); neigh_node = orig_node->router; - if (!neigh_node) + if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) { + neigh_node = NULL; goto unlock; + } - kref_get(&neigh_node->refcount); rcu_read_unlock(); if (!neigh_node->if_incoming) @@ -261,7 +262,7 @@ free_skb: kfree_skb(skb); out: if (neigh_node) - kref_put(&neigh_node->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node); if (orig_node) kref_put(&orig_node->refcount, orig_node_free_ref); return len; diff --git a/batman-adv/originator.c b/batman-adv/originator.c index e8a8473..bde9778 100644 --- a/batman-adv/originator.c +++ b/batman-adv/originator.c @@ -56,28 +56,18 @@ err: return 0; } -void neigh_node_free_ref(struct kref *refcount) -{ - struct neigh_node *neigh_node; - - neigh_node = container_of(refcount, struct neigh_node, refcount); - kfree(neigh_node); -} - static void neigh_node_free_rcu(struct rcu_head *rcu) { struct neigh_node *neigh_node; neigh_node = container_of(rcu, struct neigh_node, rcu); - kref_put(&neigh_node->refcount, neigh_node_free_ref); + kfree(neigh_node); } -void neigh_node_free_rcu_bond(struct rcu_head *rcu) +void neigh_node_free_ref(struct neigh_node *neigh_node) { - struct neigh_node *neigh_node; - - neigh_node = container_of(rcu, struct neigh_node, rcu_bond); - kref_put(&neigh_node->refcount, neigh_node_free_ref); + if (atomic_dec_and_test(&neigh_node->refcount)) + call_rcu(&neigh_node->rcu, neigh_node_free_rcu); } struct neigh_node *create_neighbor(struct orig_node *orig_node, @@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node, memcpy(neigh_node->addr, neigh, ETH_ALEN); neigh_node->orig_node = orig_neigh_node; neigh_node->if_incoming = if_incoming; - kref_init(&neigh_node->refcount); + atomic_set(&neigh_node->refcount, 1); spin_lock_bh(&orig_node->neigh_list_lock); hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list); @@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount) list_for_each_entry_safe(neigh_node, tmp_neigh_node, &orig_node->bond_list, bonding_list) { list_del_rcu(&neigh_node->bonding_list); - call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond); + neigh_node_free_ref(neigh_node); } /* for all neighbors towards this originator ... */ hlist_for_each_entry_safe(neigh_node, node, node_tmp, &orig_node->neigh_list, list) { hlist_del_rcu(&neigh_node->list); - call_rcu(&neigh_node->rcu, neigh_node_free_rcu); + neigh_node_free_ref(neigh_node); } spin_unlock_bh(&orig_node->neigh_list_lock); @@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv, hlist_del_rcu(&neigh_node->list); bonding_candidate_del(orig_node, neigh_node); - call_rcu(&neigh_node->rcu, neigh_node_free_rcu); + neigh_node_free_ref(neigh_node); } else { if ((!*best_neigh_node) || (neigh_node->tq_avg > (*best_neigh_node)->tq_avg)) diff --git a/batman-adv/originator.h b/batman-adv/originator.h index 360dfd1..84d96e2 100644 --- a/batman-adv/originator.h +++ b/batman-adv/originator.h @@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv); void originator_free(struct bat_priv *bat_priv); void purge_orig_ref(struct bat_priv *bat_priv); void orig_node_free_ref(struct kref *refcount); -void neigh_node_free_rcu_bond(struct rcu_head *rcu); struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr); struct neigh_node *create_neighbor(struct orig_node *orig_node, struct orig_node *orig_neigh_node, uint8_t *neigh, struct batman_if *if_incoming); -void neigh_node_free_ref(struct kref *refcount); +void neigh_node_free_ref(struct neigh_node *neigh_node); int orig_seq_print_text(struct seq_file *seq, void *offset); int orig_hash_add_if(struct batman_if *batman_if, int max_if_num); int orig_hash_del_if(struct batman_if *batman_if, int max_if_num); diff --git a/batman-adv/routing.c b/batman-adv/routing.c index 2861f18..0d6a629 100644 --- a/batman-adv/routing.c +++ b/batman-adv/routing.c @@ -118,12 +118,12 @@ static void update_route(struct bat_priv *bat_priv, orig_node->router->addr); } - if (neigh_node) - kref_get(&neigh_node->refcount); + if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount)) + neigh_node = NULL; neigh_node_tmp = orig_node->router; orig_node->router = neigh_node; if (neigh_node_tmp) - kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node_tmp); } @@ -175,7 +175,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node, if (!neigh_node) goto unlock; - kref_get(&neigh_node->refcount); + if (!atomic_inc_not_zero(&neigh_node->refcount)) { + neigh_node = NULL; + goto unlock; + } + rcu_read_unlock(); neigh_node->last_valid = jiffies; @@ -200,7 +204,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node, if (!neigh_node) goto unlock; - kref_get(&neigh_node->refcount); + if (!atomic_inc_not_zero(&neigh_node->refcount)) { + neigh_node = NULL; + goto unlock; + } + rcu_read_unlock(); } @@ -262,7 +270,7 @@ unlock: rcu_read_unlock(); out: if (neigh_node) - kref_put(&neigh_node->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node); return ret; } @@ -275,8 +283,8 @@ void bonding_candidate_del(struct orig_node *orig_node, goto out; list_del_rcu(&neigh_node->bonding_list); - call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond); INIT_LIST_HEAD(&neigh_node->bonding_list); + neigh_node_free_ref(neigh_node); atomic_dec(&orig_node->bond_candidates); out: @@ -337,8 +345,10 @@ static void bonding_candidate_add(struct orig_node *orig_node, if (!list_empty(&neigh_node->bonding_list)) goto out; + if (!atomic_inc_not_zero(&neigh_node->refcount)) + goto out; + list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list); - kref_get(&neigh_node->refcount); atomic_inc(&orig_node->bond_candidates); goto out; @@ -382,7 +392,10 @@ static void update_orig(struct bat_priv *bat_priv, hlist_for_each_entry_rcu(tmp_neigh_node, node, &orig_node->neigh_list, list) { if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) && - (tmp_neigh_node->if_incoming == if_incoming)) { + (tmp_neigh_node->if_incoming == if_incoming) && + atomic_inc_not_zero(&tmp_neigh_node->refcount)) { + if (neigh_node) + neigh_node_free_ref(neigh_node); neigh_node = tmp_neigh_node; continue; } @@ -409,11 +422,15 @@ static void update_orig(struct bat_priv *bat_priv, kref_put(&orig_tmp->refcount, orig_node_free_ref); if (!neigh_node) goto unlock; + + if (!atomic_inc_not_zero(&neigh_node->refcount)) { + neigh_node = NULL; + goto unlock; + } } else bat_dbg(DBG_BATMAN, bat_priv, "Updating existing last-hop neighbor of originator\n"); - kref_get(&neigh_node->refcount); rcu_read_unlock(); orig_node->flags = batman_packet->flags; @@ -490,7 +507,7 @@ unlock: rcu_read_unlock(); out: if (neigh_node) - kref_put(&neigh_node->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node); } /* checks whether the host restarted and is in the protection time. @@ -894,7 +911,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv, if (!neigh_node) goto unlock; - kref_get(&neigh_node->refcount); + if (!atomic_inc_not_zero(&neigh_node->refcount)) { + neigh_node = NULL; + goto unlock; + } + rcu_read_unlock(); /* create a copy of the skb, if needed, to modify it. */ @@ -917,7 +938,7 @@ unlock: rcu_read_unlock(); out: if (neigh_node) - kref_put(&neigh_node->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node); if (orig_node) kref_put(&orig_node->refcount, orig_node_free_ref); return ret; @@ -958,7 +979,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv, if (!neigh_node) goto unlock; - kref_get(&neigh_node->refcount); + if (!atomic_inc_not_zero(&neigh_node->refcount)) { + neigh_node = NULL; + goto unlock; + } + rcu_read_unlock(); /* create a copy of the skb, if needed, to modify it. */ @@ -981,7 +1006,7 @@ unlock: rcu_read_unlock(); out: if (neigh_node) - kref_put(&neigh_node->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node); if (orig_node) kref_put(&orig_node->refcount, orig_node_free_ref); return ret; @@ -1054,7 +1079,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if) if (!neigh_node) goto unlock; - kref_get(&neigh_node->refcount); + if (!atomic_inc_not_zero(&neigh_node->refcount)) { + neigh_node = NULL; + goto unlock; + } + rcu_read_unlock(); /* create a copy of the skb, if needed, to modify it. */ @@ -1075,7 +1104,7 @@ unlock: rcu_read_unlock(); out: if (neigh_node) - kref_put(&neigh_node->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node); if (orig_node) kref_put(&orig_node->refcount, orig_node_free_ref); return ret; @@ -1108,12 +1137,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv, /* select default router to output */ router = orig_node->router; router_orig = orig_node->router->orig_node; - if (!router_orig) { + if (!router_orig || !atomic_inc_not_zero(&router->refcount)) { rcu_read_unlock(); return NULL; } - if ((!recv_if) && (!bonding_enabled)) goto return_router; @@ -1146,6 +1174,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv, * is is not on the interface where the packet came * in. */ + neigh_node_free_ref(router); first_candidate = NULL; router = NULL; @@ -1158,16 +1187,23 @@ struct neigh_node *find_router(struct bat_priv *bat_priv, if (!first_candidate) first_candidate = tmp_neigh_node; /* recv_if == NULL on the first node. */ - if (tmp_neigh_node->if_incoming != recv_if) { + if (tmp_neigh_node->if_incoming != recv_if && + atomic_inc_not_zero(&tmp_neigh_node->refcount)) { router = tmp_neigh_node; break; } } /* use the first candidate if nothing was found. */ - if (!router) + if (!router && first_candidate && + atomic_inc_not_zero(&first_candidate->refcount)) router = first_candidate; + if (!router) { + rcu_read_unlock(); + return NULL; + } + /* selected should point to the next element * after the current router */ spin_lock_bh(&primary_orig_node->neigh_list_lock); @@ -1188,21 +1224,24 @@ struct neigh_node *find_router(struct bat_priv *bat_priv, first_candidate = tmp_neigh_node; /* recv_if == NULL on the first node. */ - if (tmp_neigh_node->if_incoming != recv_if) + if (tmp_neigh_node->if_incoming != recv_if && + atomic_inc_not_zero(&tmp_neigh_node->refcount)) { /* if we don't have a router yet * or this one is better, choose it. */ if ((!router) || - (tmp_neigh_node->tq_avg > router->tq_avg)) { + (tmp_neigh_node->tq_avg > router->tq_avg)) router = tmp_neigh_node; - } + else + neigh_node_free_ref(tmp_neigh_node); + } } /* use the first candidate if nothing was found. */ - if (!router) + if (!router && first_candidate && + atomic_inc_not_zero(&first_candidate->refcount)) router = first_candidate; } return_router: - kref_get(&router->refcount); rcu_read_unlock(); return router; } @@ -1315,7 +1354,7 @@ unlock: rcu_read_unlock(); out: if (neigh_node) - kref_put(&neigh_node->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node); if (orig_node) kref_put(&orig_node->refcount, orig_node_free_ref); return ret; diff --git a/batman-adv/types.h b/batman-adv/types.h index 317ede8..ee77d48 100644 --- a/batman-adv/types.h +++ b/batman-adv/types.h @@ -119,9 +119,8 @@ struct neigh_node { struct list_head bonding_list; unsigned long last_valid; unsigned long real_bits[NUM_WORDS]; - struct kref refcount; + atomic_t refcount; struct rcu_head rcu; - struct rcu_head rcu_bond; struct orig_node *orig_node; struct batman_if *if_incoming; }; diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c index 6a9ab61..580b547 100644 --- a/batman-adv/unicast.c +++ b/batman-adv/unicast.c @@ -363,7 +363,7 @@ find_router: out: if (neigh_node) - kref_put(&neigh_node->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node); if (orig_node) kref_put(&orig_node->refcount, orig_node_free_ref); if (ret == 1) diff --git a/batman-adv/vis.c b/batman-adv/vis.c index 191401b..c1c3258 100644 --- a/batman-adv/vis.c +++ b/batman-adv/vis.c @@ -776,7 +776,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv, if (!neigh_node) goto unlock; - kref_get(&neigh_node->refcount); + if (!atomic_inc_not_zero(&neigh_node->refcount)) { + neigh_node = NULL; + goto unlock; + } + rcu_read_unlock(); skb = skb_clone(info->skb_packet, GFP_ATOMIC); @@ -790,7 +794,7 @@ unlock: rcu_read_unlock(); out: if (neigh_node) - kref_put(&neigh_node->refcount, neigh_node_free_ref); + neigh_node_free_ref(neigh_node); if (orig_node) kref_put(&orig_node->refcount, orig_node_free_ref); return;