From: Sven Eckelmann &lt;sven(a)narfation.org&gt; Was: --- <TODO: write a long monologue about every problem we have or could have or maybe never had and would have when we not have it> Signed-off-by: Sven Eckelmann &lt;sven(a)narfation.org&gt; --- So after some more discussions with Marek and Sven, it looks like we have to use the rcu protected macros rcu_dereference() and rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node. Changes here also include moving the kref_get() from unicast_send_skb() into gw_get_selected(). The orig_node could have been freed already at the time the kref_get() was called in unicast_send_skb(). Some things that are still not that clear to me: gw_election(): * can the if-block before gw_deselect() be ommited, we had a nullpointer check for curr_gw just a couple of lines before during the rcu-lock. gw_deselet(): * is the refcount at this time always 1 for gw_node, can the null pointer check + a rcu_dereference be ommited? (at least that's what it looks like when comparing to the rcuref.txt example) gw_get_selected(): * Probably the orig_node's refcounting has to be made atomic, too? Cheers, Linus Not-Signed-off-by: Linus Lüssing &lt;linus.luessing(a)ascom.ch&gt; --- gateway_client.c | 169 +++++++++++++++++++++++++++++++++--------------------- main.c | 1 + types.h | 7 +- unicast.c | 1 - 4 files changed, 109 insertions(+), 69 deletions(-) diff --git a/gateway_client.c b/gateway_client.c index 429a013..96a67bc 100644 --- a/gateway_client.c +++ b/gateway_client.c @@ -28,40 +28,54 @@ #include <linux/udp.h> #include <linux/if_vlan.h> -static void gw_node_free_ref(struct kref *refcount) +static void gw_node_free_rcu(struct rcu_head *rcu) { struct gw_node *gw_node; - gw_node = container_of(refcount, struct gw_node, refcount); + gw_node = container_of(rcu, struct gw_node, rcu); kfree(gw_node); } -static void gw_node_free_rcu(struct rcu_head *rcu) +static void gw_node_free_ref(struct gw_node *gw_node) { - struct gw_node *gw_node; - - gw_node = container_of(rcu, struct gw_node, rcu); - kref_put(&gw_node->refcount, gw_node_free_ref); + if (atomic_dec_and_test(&gw_node->refcount)) + call_rcu(&gw_node->rcu, gw_node_free_rcu); } +/* increases the returned orig_node's refcount */ void *gw_get_selected(struct bat_priv *bat_priv) { - struct gw_node *curr_gateway_tmp = bat_priv->curr_gw; + struct gw_node *curr_gateway_tmp; + struct orig_node *orig_node; - if (!curr_gateway_tmp) + rcu_read_lock(); + curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw); + if (!curr_gateway_tmp) { + rcu_read_unlock(); return NULL; + } - return curr_gateway_tmp->orig_node; + orig_node = rcu_dereference(curr_gateway_tmp->orig_node); + if (orig_node) { + kref_get(&orig_node->refcount); + rcu_read_unlock(); + return NULL; + } + + rcu_read_unlock(); + return orig_node; } void gw_deselect(struct bat_priv *bat_priv) { - struct gw_node *gw_node = bat_priv->curr_gw; + struct gw_node *gw_node; - bat_priv->curr_gw = NULL; + spin_lock_bh(&bat_priv->curr_gw_lock); + gw_node = bat_priv->curr_gw; + rcu_assign_pointer(bat_priv->curr_gw, NULL); + spin_unlock_bh(&bat_priv->curr_gw_lock); - if (gw_node) - kref_put(&gw_node->refcount, gw_node_free_ref); + gw_node_free_ref(gw_node); } static struct gw_node *gw_select(struct bat_priv *bat_priv, @@ -69,17 +83,21 @@ static struct gw_node *gw_select(struct bat_priv *bat_priv, { struct gw_node *curr_gw_node = bat_priv->curr_gw; - if (new_gw_node) - kref_get(&new_gw_node->refcount); + if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount)) + return NULL; + + spin_lock_bh(&bat_priv->curr_gw_lock); + rcu_assign_pointer(bat_priv->curr_gw, new_gw_node); + spin_unlock_bh(&bat_priv->curr_gw_lock); - bat_priv->curr_gw = new_gw_node; return curr_gw_node; } void gw_election(struct bat_priv *bat_priv) { struct hlist_node *node; - struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL; + struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL, *old_gw_node = NULL; + struct orig_node *orig_node; uint8_t max_tq = 0; uint32_t max_gw_factor = 0, tmp_gw_factor = 0; int down, up; @@ -93,25 +111,28 @@ void gw_election(struct bat_priv *bat_priv) if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT) return; - if (bat_priv->curr_gw) - return; - rcu_read_lock(); - if (hlist_empty(&bat_priv->gw_list)) { + curr_gw = rcu_dereference(bat_priv->curr_gw); + if (curr_gw) { rcu_read_unlock(); + return; + } - if (bat_priv->curr_gw) { + if (hlist_empty(&bat_priv->gw_list)) { + if (curr_gw) { bat_dbg(DBG_BATMAN, bat_priv, "Removing selected gateway - " "no gateway in range\n"); gw_deselect(bat_priv); } + rcu_read_unlock(); return; } hlist_for_each_entry_rcu(gw_node, node, &bat_priv->gw_list, list) { - if (!gw_node->orig_node->router) + orig_node = rcu_dereference(gw_node->orig_node); + if (!orig_node->router) continue; if (gw_node->deleted) @@ -119,18 +140,17 @@ void gw_election(struct bat_priv *bat_priv) switch (atomic_read(&bat_priv->gw_sel_class)) { case 1: /* fast connection */ - gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, - &down, &up); + gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up); - tmp_gw_factor = (gw_node->orig_node->router->tq_avg * - gw_node->orig_node->router->tq_avg * + tmp_gw_factor = (orig_node->router->tq_avg * + orig_node->router->tq_avg * down * 100 * 100) / (TQ_LOCAL_WINDOW_SIZE * TQ_LOCAL_WINDOW_SIZE * 64); if ((tmp_gw_factor > max_gw_factor) || ((tmp_gw_factor == max_gw_factor) && - (gw_node->orig_node->router->tq_avg > max_tq))) + (orig_node->router->tq_avg > max_tq))) curr_gw_tmp = gw_node; break; @@ -142,37 +162,38 @@ void gw_election(struct bat_priv *bat_priv) * soon as a better gateway appears which has * $routing_class more tq points) **/ - if (gw_node->orig_node->router->tq_avg > max_tq) + if (orig_node->router->tq_avg > max_tq) curr_gw_tmp = gw_node; break; } - if (gw_node->orig_node->router->tq_avg > max_tq) - max_tq = gw_node->orig_node->router->tq_avg; + if (orig_node->router->tq_avg > max_tq) + max_tq = orig_node->router->tq_avg; if (tmp_gw_factor > max_gw_factor) max_gw_factor = tmp_gw_factor; } - if (bat_priv->curr_gw != curr_gw_tmp) { - if ((bat_priv->curr_gw) && (!curr_gw_tmp)) + if (curr_gw != curr_gw_tmp) { + orig_node = rcu_dereference(curr_gw_tmp->orig_node); + if ((curr_gw) && (!curr_gw_tmp)) bat_dbg(DBG_BATMAN, bat_priv, "Removing selected gateway - " "no gateway in range\n"); - else if ((!bat_priv->curr_gw) && (curr_gw_tmp)) + else if ((!curr_gw) && (curr_gw_tmp)) bat_dbg(DBG_BATMAN, bat_priv, "Adding route to gateway %pM " "(gw_flags: %i, tq: %i)\n", - curr_gw_tmp->orig_node->orig, - curr_gw_tmp->orig_node->gw_flags, - curr_gw_tmp->orig_node->router->tq_avg); + orig_node->orig, + orig_node->gw_flags, + orig_node->router->tq_avg); else bat_dbg(DBG_BATMAN, bat_priv, "Changing route to gateway %pM " "(gw_flags: %i, tq: %i)\n", - curr_gw_tmp->orig_node->orig, - curr_gw_tmp->orig_node->gw_flags, - curr_gw_tmp->orig_node->router->tq_avg); + orig_node->orig, + orig_node->gw_flags, + orig_node->router->tq_avg); old_gw_node = gw_select(bat_priv, curr_gw_tmp); } @@ -181,36 +202,40 @@ void gw_election(struct bat_priv *bat_priv) /* the kfree() has to be outside of the rcu lock */ if (old_gw_node) - kref_put(&old_gw_node->refcount, gw_node_free_ref); + gw_node_free_ref(old_gw_node); } void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node) { - struct gw_node *curr_gateway_tmp = bat_priv->curr_gw; + struct gw_node *curr_gateway_tmp; + struct orig_node *curr_gw_orig; uint8_t gw_tq_avg, orig_tq_avg; + rcu_read_lock(); + curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw); if (!curr_gateway_tmp) - return; + goto rcu_unlock; - if (!curr_gateway_tmp->orig_node) + curr_gw_orig = rcu_dereference(curr_gateway_tmp->orig_node); + if (!curr_gw_orig) goto deselect; - if (!curr_gateway_tmp->orig_node->router) + if (!curr_gw_orig->router) goto deselect; /* this node already is the gateway */ - if (curr_gateway_tmp->orig_node == orig_node) - return; + if (curr_gw_orig == orig_node) + goto deselect; if (!orig_node->router) - return; + goto rcu_unlock; - gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg; + gw_tq_avg = curr_gw_orig ->router->tq_avg; orig_tq_avg = orig_node->router->tq_avg; /* the TQ value has to be better */ if (orig_tq_avg < gw_tq_avg) - return; + goto rcu_unlock; /** * if the routing class is greater than 3 the value tells us how much @@ -218,7 +243,7 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node) **/ if ((atomic_read(&bat_priv->gw_sel_class) > 3) && (orig_tq_avg - gw_tq_avg < atomic_read(&bat_priv->gw_sel_class))) - return; + goto rcu_unlock; bat_dbg(DBG_BATMAN, bat_priv, "Restarting gateway selection: better gateway found (tq curr: " @@ -227,6 +252,8 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node) deselect: gw_deselect(bat_priv); +rcu_unlock: + rcu_read_unlock(); } static void gw_node_add(struct bat_priv *bat_priv, @@ -242,7 +269,7 @@ static void gw_node_add(struct bat_priv *bat_priv, memset(gw_node, 0, sizeof(struct gw_node)); INIT_HLIST_NODE(&gw_node->list); gw_node->orig_node = orig_node; - kref_init(&gw_node->refcount); + atomic_set(&gw_node->refcount, 1); spin_lock_bh(&bat_priv->gw_list_lock); hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list); @@ -325,7 +352,7 @@ void gw_node_purge(struct bat_priv *bat_priv) gw_deselect(bat_priv); hlist_del_rcu(&gw_node->list); - call_rcu(&gw_node->rcu, gw_node_free_rcu); + gw_node_free_ref(gw_node); } @@ -335,21 +362,29 @@ void gw_node_purge(struct bat_priv *bat_priv) static int _write_buffer_text(struct bat_priv *bat_priv, struct seq_file *seq, struct gw_node *gw_node) { - int down, up; + struct gw_node *curr_gw; + struct orig_node *orig_node; + int down, up, ret; - gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up); - - return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n", - (bat_priv->curr_gw == gw_node ? "=>" : " "), - gw_node->orig_node->orig, - gw_node->orig_node->router->tq_avg, - gw_node->orig_node->router->addr, - gw_node->orig_node->router->if_incoming->net_dev->name, - gw_node->orig_node->gw_flags, + rcu_read_lock(); + curr_gw = rcu_dereference(bat_priv->curr_gw); + orig_node = rcu_dereference(gw_node->orig_node); + gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up); + + ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n", + (curr_gw == gw_node ? "=>" : " "), + orig_node->orig, + orig_node->router->tq_avg, + orig_node->router->addr, + orig_node->router->if_incoming->net_dev->name, + orig_node->gw_flags, (down > 2048 ? down / 1024 : down), (down > 2048 ? "MBit" : "KBit"), (up > 2048 ? up / 1024 : up), (up > 2048 ? "MBit" : "KBit")); + rcu_read_unlock(); + + return ret; } int gw_client_seq_print_text(struct seq_file *seq, void *offset) @@ -470,8 +505,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb) if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER) return -1; - if (!bat_priv->curr_gw) + rcu_read_lock(); + if (!rcu_dereference(bat_priv->curr_gw)) { + rcu_read_unlock(); return 0; + } + rcu_read_unlock(); return 1; } diff --git a/main.c b/main.c index e687e7f..8679260 100644 --- a/main.c +++ b/main.c @@ -85,6 +85,7 @@ int mesh_init(struct net_device *soft_iface) spin_lock_init(&bat_priv->hna_lhash_lock); spin_lock_init(&bat_priv->hna_ghash_lock); spin_lock_init(&bat_priv->gw_list_lock); + spin_lock_init(&bat_priv->curr_gw_lock); spin_lock_init(&bat_priv->vis_hash_lock); spin_lock_init(&bat_priv->vis_list_lock); spin_lock_init(&bat_priv->softif_neigh_lock); diff --git a/types.h b/types.h index e4a0462..b9b20b6 100644 --- a/types.h +++ b/types.h @@ -98,9 +98,9 @@ struct orig_node { struct gw_node { struct hlist_node list; - struct orig_node *orig_node; + struct orig_node *orig_node; /* rcu protected pointer */ unsigned long deleted; - struct kref refcount; + atomic_t refcount; struct rcu_head rcu; }; @@ -163,6 +163,7 @@ struct bat_priv { spinlock_t hna_lhash_lock; /* protects hna_local_hash */ spinlock_t hna_ghash_lock; /* protects hna_global_hash */ spinlock_t gw_list_lock; /* protects gw_list */ + spinlock_t curr_gw_lock; /* protects curr_gw updates */ spinlock_t vis_hash_lock; /* protects vis_hash */ spinlock_t vis_list_lock; /* protects vis_info::recv_list */ spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */ @@ -171,7 +172,7 @@ struct bat_priv { struct delayed_work hna_work; struct delayed_work orig_work; struct delayed_work vis_work; - struct gw_node *curr_gw; + struct gw_node *curr_gw; /* rcu protected pointer */ struct vis_info *my_vis_info; }; diff --git a/unicast.c b/unicast.c index 6a9ab61..8816102 100644 --- a/unicast.c +++ b/unicast.c @@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv) if (!orig_node) goto trans_search; - kref_get(&orig_node->refcount); goto find_router; } else { rcu_read_lock(); -- 1.7.2.3

What? Since when is curr_gw::orig_node rcu protected? And you cannot return the (it is not, but when it would be) rcu protected element without increasing the refcounter. Same here And here Here again Here.... .... Why? Maybe this should be part of the patch 3/3? And please to merge the refcounting stuff with the rcu pointer in one patch... and maybe it is better to merge patch 2/3 with the rcu pointer patch. Best regards, Sven

When unicast_send_skb() is increasing the orig_node's refcount another thread might have been freeing this orig_node already. We need to increase the refcount in the rcu read lock protected area to avoid that. Signed-off-by: Linus Lüssing &lt;linus.luessing(a)ascom.ch&gt; --- gateway_client.c | 1 + unicast.c | 1 - 2 files changed, 1 insertions(+), 1 deletions(-) diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c index ee71335..15ea268 100644 --- a/batman-adv/gateway_client.c +++ b/batman-adv/gateway_client.c @@ -57,6 +57,7 @@ void *gw_get_selected(struct bat_priv *bat_priv) orig_node = rcu_dereference(curr_gateway_tmp->orig_node); if (orig_node) { + kref_get(&orig_node->refcount); rcu_read_unlock(); return NULL; } diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c index 8816102..b42e40e 100644 --- a/batman-adv/unicast.c +++ b/batman-adv/unicast.c @@ -310,7 +310,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv) goto trans_search; } - kref_get(&orig_node->refcount); rcu_read_unlock(); goto find_router; } -- 1.7.2.3

<TODO: write a long monologue about every problem we have or could have or maybe never had and would have when we not have it> Signed-off-by: Sven Eckelmann &lt;sven(a)narfation.org&gt; --- Removed the "BUG" after a small conversation with d0tslash batman-adv/icmp_socket.c | 7 ++-- batman-adv/originator.c | 26 ++++---------- batman-adv/originator.h | 3 +- batman-adv/routing.c | 83 +++++++++++++++++++++++++++++---------------- batman-adv/types.h | 3 +- batman-adv/unicast.c | 2 +- batman-adv/vis.c | 7 ++-- 7 files changed, 72 insertions(+), 59 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..94e9c1c 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); } @@ -172,10 +172,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node, orig_neigh_node->orig, if_incoming); /* create_neighbor failed, return 0 */ - 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(); neigh_node->last_valid = jiffies; @@ -197,10 +199,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node, orig_neigh_node->orig, if_incoming); /* create_neighbor failed, return 0 */ - 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(); } @@ -262,7 +266,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,7 +279,7 @@ 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); + neigh_node_free_ref(neigh_node); INIT_LIST_HEAD(&neigh_node->bonding_list); atomic_dec(&orig_node->bond_candidates); @@ -337,8 +341,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 +388,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; } @@ -407,13 +416,15 @@ static void update_orig(struct bat_priv *bat_priv, ethhdr->h_source, if_incoming); kref_put(&orig_tmp->refcount, orig_node_free_ref); - if (!neigh_node) + if (!neigh_node || + !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 +501,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. @@ -891,10 +902,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv, 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(); /* create a copy of the skb, if needed, to modify it. */ @@ -917,7 +929,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; @@ -955,10 +967,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv, 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(); /* create a copy of the skb, if needed, to modify it. */ @@ -981,7 +994,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; @@ -1051,10 +1064,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if) 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(); /* create a copy of the skb, if needed, to modify it. */ @@ -1075,7 +1089,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,7 +1122,7 @@ 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; } @@ -1146,6 +1160,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 +1173,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,7 +1210,8 @@ 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) || @@ -1198,11 +1221,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv, } /* 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 +1338,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..fe32de3 100644 --- a/batman-adv/vis.c +++ b/batman-adv/vis.c @@ -773,10 +773,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv, 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(); skb = skb_clone(info->skb_packet, GFP_ATOMIC); @@ -790,7 +791,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; -- 1.7.2.3

From: Sven Eckelmann &lt;sven(a)narfation.org&gt; 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 &lt;sven(a)narfation.org&gt; Signed-off-by: Marek Lindner &lt;lindner_marek(a)yahoo.de&gt; --- batman-adv/gateway_client.c | 37 ++++++++++++++++--------------------- batman-adv/types.h | 2 +- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c index 429a013..517e001 100644 --- a/batman-adv/gateway_client.c +++ b/batman-adv/gateway_client.c @@ -28,20 +28,18 @@ #include <linux/udp.h> #include <linux/if_vlan.h> -static void gw_node_free_ref(struct kref *refcount) +static void gw_node_free_rcu(struct rcu_head *rcu) { struct gw_node *gw_node; - gw_node = container_of(refcount, struct gw_node, refcount); + gw_node = container_of(rcu, struct gw_node, rcu); kfree(gw_node); } -static void gw_node_free_rcu(struct rcu_head *rcu) +static void gw_node_free_ref(struct gw_node *gw_node) { - struct gw_node *gw_node; - - gw_node = container_of(rcu, struct gw_node, rcu); - kref_put(&gw_node->refcount, gw_node_free_ref); + if (atomic_dec_and_test(&gw_node->refcount)) + call_rcu(&gw_node->rcu, gw_node_free_rcu); } void *gw_get_selected(struct bat_priv *bat_priv) @@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv) bat_priv->curr_gw = NULL; if (gw_node) - kref_put(&gw_node->refcount, gw_node_free_ref); + gw_node_free_ref(gw_node); } -static struct gw_node *gw_select(struct bat_priv *bat_priv, - struct gw_node *new_gw_node) +static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node) { struct gw_node *curr_gw_node = bat_priv->curr_gw; - if (new_gw_node) - kref_get(&new_gw_node->refcount); + if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount)) + new_gw_node = NULL; bat_priv->curr_gw = new_gw_node; - return curr_gw_node; + + if (curr_gw_node) + gw_node_free_ref(curr_gw_node); } void gw_election(struct bat_priv *bat_priv) { struct hlist_node *node; - struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL; + struct gw_node *gw_node, *curr_gw_tmp = NULL; uint8_t max_tq = 0; uint32_t max_gw_factor = 0, tmp_gw_factor = 0; int down, up; @@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv) curr_gw_tmp->orig_node->gw_flags, curr_gw_tmp->orig_node->router->tq_avg); - old_gw_node = gw_select(bat_priv, curr_gw_tmp); + gw_select(bat_priv, curr_gw_tmp); } rcu_read_unlock(); - - /* the kfree() has to be outside of the rcu lock */ - if (old_gw_node) - kref_put(&old_gw_node->refcount, gw_node_free_ref); } void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node) @@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv, memset(gw_node, 0, sizeof(struct gw_node)); INIT_HLIST_NODE(&gw_node->list); gw_node->orig_node = orig_node; - kref_init(&gw_node->refcount); + atomic_set(&gw_node->refcount, 1); spin_lock_bh(&bat_priv->gw_list_lock); hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list); @@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv) gw_deselect(bat_priv); hlist_del_rcu(&gw_node->list); - call_rcu(&gw_node->rcu, gw_node_free_rcu); + gw_node_free_ref(gw_node); } diff --git a/batman-adv/types.h b/batman-adv/types.h index e4a0462..ca5f20a 100644 --- a/batman-adv/types.h +++ b/batman-adv/types.h @@ -100,7 +100,7 @@ struct gw_node { struct hlist_node list; struct orig_node *orig_node; unsigned long deleted; - struct kref refcount; + atomic_t refcount; struct rcu_head rcu; }; -- 1.7.2.3

From: Sven Eckelmann &lt;sven(a)narfation.org&gt; 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 &lt;sven(a)narfation.org&gt; Signed-off-by: Marek Lindner &lt;lindner_marek(a)yahoo.de&gt; --- batman-adv/bat_sysfs.c | 20 +++++++++----------- batman-adv/hard-interface.c | 40 +++++++++++++++++++--------------------- batman-adv/hard-interface.h | 9 ++++----- batman-adv/types.h | 2 +- 4 files changed, 33 insertions(+), 38 deletions(-) diff --git a/batman-adv/bat_sysfs.c b/batman-adv/bat_sysfs.c index f7b93a0..93ae20a 100644 --- a/batman-adv/bat_sysfs.c +++ b/batman-adv/bat_sysfs.c @@ -450,7 +450,7 @@ static ssize_t show_mesh_iface(struct kobject *kobj, struct attribute *attr, length = sprintf(buff, "%s\n", batman_if->if_status == IF_NOT_IN_USE ? "none" : batman_if->soft_iface->name); - kref_put(&batman_if->refcount, hardif_free_ref); + hardif_free_ref(batman_if); return length; } @@ -461,7 +461,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, struct net_device *net_dev = kobj_to_netdev(kobj); struct batman_if *batman_if = get_batman_if_by_netdev(net_dev); int status_tmp = -1; - int ret; + int ret = count; if (!batman_if) return count; @@ -472,7 +472,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, if (strlen(buff) >= IFNAMSIZ) { pr_err("Invalid parameter for 'mesh_iface' setting received: " "interface name too long '%s'\n", buff); - kref_put(&batman_if->refcount, hardif_free_ref); + hardif_free_ref(batman_if); return -EINVAL; } @@ -482,17 +482,14 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, status_tmp = IF_I_WANT_YOU; if ((batman_if->if_status == status_tmp) || ((batman_if->soft_iface) && - (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) { - kref_put(&batman_if->refcount, hardif_free_ref); - return count; - } + (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) + goto out; if (status_tmp == IF_NOT_IN_USE) { rtnl_lock(); hardif_disable_interface(batman_if); rtnl_unlock(); - kref_put(&batman_if->refcount, hardif_free_ref); - return count; + goto out; } /* if the interface already is in use */ @@ -503,8 +500,9 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, } ret = hardif_enable_interface(batman_if, buff); - kref_put(&batman_if->refcount, hardif_free_ref); +out: + hardif_free_ref(batman_if); return ret; } @@ -537,7 +535,7 @@ static ssize_t show_iface_status(struct kobject *kobj, struct attribute *attr, break; } - kref_put(&batman_if->refcount, hardif_free_ref); + hardif_free_ref(batman_if); return length; } diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c index e2b001a..8982485 100644 --- a/batman-adv/hard-interface.c +++ b/batman-adv/hard-interface.c @@ -40,13 +40,13 @@ static int batman_skb_recv(struct sk_buff *skb, struct packet_type *ptype, struct net_device *orig_dev); -static void hardif_free_rcu(struct rcu_head *rcu) +void hardif_free_rcu(struct rcu_head *rcu) { struct batman_if *batman_if; batman_if = container_of(rcu, struct batman_if, rcu); dev_put(batman_if->net_dev); - kref_put(&batman_if->refcount, hardif_free_ref); + kfree(batman_if); } struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev) @@ -55,16 +55,14 @@ struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev) rcu_read_lock(); list_for_each_entry_rcu(batman_if, &if_list, list) { - if (batman_if->net_dev == net_dev) + if (batman_if->net_dev == net_dev && + atomic_inc_not_zero(&batman_if->refcount)) goto out; } batman_if = NULL; out: - if (batman_if) - kref_get(&batman_if->refcount); - rcu_read_unlock(); return batman_if; } @@ -105,16 +103,14 @@ static struct batman_if *get_active_batman_if(struct net_device *soft_iface) if (batman_if->soft_iface != soft_iface) continue; - if (batman_if->if_status == IF_ACTIVE) + if (batman_if->if_status == IF_ACTIVE && + atomic_inc_not_zero(&batman_if->refcount)) goto out; } batman_if = NULL; out: - if (batman_if) - kref_get(&batman_if->refcount); - rcu_read_unlock(); return batman_if; } @@ -137,14 +133,14 @@ static void set_primary_if(struct bat_priv *bat_priv, struct batman_packet *batman_packet; struct batman_if *old_if; - if (batman_if) - kref_get(&batman_if->refcount); + if (batman_if && !atomic_inc_not_zero(&batman_if->refcount)) + batman_if = NULL; old_if = bat_priv->primary_if; bat_priv->primary_if = batman_if; if (old_if) - kref_put(&old_if->refcount, hardif_free_ref); + hardif_free_ref(old_if); if (!bat_priv->primary_if) return; @@ -290,6 +286,9 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name) if (batman_if->if_status != IF_NOT_IN_USE) goto out; + if (!atomic_inc_not_zero(&batman_if->refcount)) + goto out; + batman_if->soft_iface = dev_get_by_name(&init_net, iface_name); if (!batman_if->soft_iface) { @@ -328,7 +327,6 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name) batman_if->batman_adv_ptype.type = __constant_htons(ETH_P_BATMAN); batman_if->batman_adv_ptype.func = batman_skb_recv; batman_if->batman_adv_ptype.dev = batman_if->net_dev; - kref_get(&batman_if->refcount); dev_add_pack(&batman_if->batman_adv_ptype); atomic_set(&batman_if->seqno, 1); @@ -371,6 +369,7 @@ out: return 0; err: + hardif_free_ref(batman_if); return -ENOMEM; } @@ -387,7 +386,6 @@ void hardif_disable_interface(struct batman_if *batman_if) bat_info(batman_if->soft_iface, "Removing interface: %s\n", batman_if->net_dev->name); dev_remove_pack(&batman_if->batman_adv_ptype); - kref_put(&batman_if->refcount, hardif_free_ref); bat_priv->num_ifaces--; orig_hash_del_if(batman_if, bat_priv->num_ifaces); @@ -399,7 +397,7 @@ void hardif_disable_interface(struct batman_if *batman_if) set_primary_if(bat_priv, new_if); if (new_if) - kref_put(&new_if->refcount, hardif_free_ref); + hardif_free_ref(new_if); } kfree(batman_if->packet_buff); @@ -416,6 +414,7 @@ void hardif_disable_interface(struct batman_if *batman_if) softif_destroy(batman_if->soft_iface); batman_if->soft_iface = NULL; + hardif_free_ref(batman_if); } static struct batman_if *hardif_add_interface(struct net_device *net_dev) @@ -445,7 +444,8 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev) batman_if->soft_iface = NULL; batman_if->if_status = IF_NOT_IN_USE; INIT_LIST_HEAD(&batman_if->list); - kref_init(&batman_if->refcount); + /* extra reference for return */ + atomic_set(&batman_if->refcount, 2); check_known_mac_addr(batman_if->net_dev); @@ -453,8 +453,6 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev) list_add_tail_rcu(&batman_if->list, &if_list); spin_unlock(&if_list_lock); - /* extra reference for return */ - kref_get(&batman_if->refcount); return batman_if; free_if: @@ -476,7 +474,7 @@ static void hardif_remove_interface(struct batman_if *batman_if) batman_if->if_status = IF_TO_BE_REMOVED; sysfs_del_hardif(&batman_if->hardif_obj); - call_rcu(&batman_if->rcu, hardif_free_rcu); + hardif_free_ref(batman_if); } void hardif_remove_interfaces(void) @@ -548,7 +546,7 @@ static int hard_if_event(struct notifier_block *this, }; hardif_put: - kref_put(&batman_if->refcount, hardif_free_ref); + hardif_free_ref(batman_if); out: return NOTIFY_DONE; } diff --git a/batman-adv/hard-interface.h b/batman-adv/hard-interface.h index ad19543..e488b90 100644 --- a/batman-adv/hard-interface.h +++ b/batman-adv/hard-interface.h @@ -37,13 +37,12 @@ void hardif_disable_interface(struct batman_if *batman_if); void hardif_remove_interfaces(void); int hardif_min_mtu(struct net_device *soft_iface); void update_min_mtu(struct net_device *soft_iface); +void hardif_free_rcu(struct rcu_head *rcu); -static inline void hardif_free_ref(struct kref *refcount) +static inline void hardif_free_ref(struct batman_if *batman_if) { - struct batman_if *batman_if; - - batman_if = container_of(refcount, struct batman_if, refcount); - kfree(batman_if); + if (atomic_dec_and_test(&batman_if->refcount)) + call_rcu(&batman_if->rcu, hardif_free_rcu); } #endif /* _NET_BATMAN_ADV_HARD_INTERFACE_H_ */ diff --git a/batman-adv/types.h b/batman-adv/types.h index 8df6d9d..317ede8 100644 --- a/batman-adv/types.h +++ b/batman-adv/types.h @@ -43,7 +43,7 @@ struct batman_if { unsigned char *packet_buff; int packet_len; struct kobject *hardif_obj; - struct kref refcount; + atomic_t refcount; struct packet_type batman_adv_ptype; struct net_device *soft_iface; struct rcu_head rcu; -- 1.7.2.3

From: Sven Eckelmann &lt;sven(a)narfation.org&gt; 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 &lt;sven(a)narfation.org&gt; Signed-off-by: Marek Lindner &lt;lindner_marek(a)yahoo.de&gt; --- Fixed refcounting in find_router(). batman-adv/icmp_socket.c | 7 ++- batman-adv/originator.c | 26 +++------- batman-adv/originator.h | 3 +- batman-adv/routing.c | 111 +++++++++++++++++++++++++++++++++------------- batman-adv/types.h | 3 +- batman-adv/unicast.c | 2 +- batman-adv/vis.c | 8 +++- 7 files changed, 101 insertions(+), 59 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..091edd4 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,34 @@ 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 we don't have a router yet - * or this one is better, choose it. */ - if ((!router) || - (tmp_neigh_node->tq_avg > router->tq_avg)) { - router = tmp_neigh_node; - } + if (tmp_neigh_node->if_incoming == recv_if) + continue; + + if (!atomic_inc_not_zero(&tmp_neigh_node->refcount)) + continue; + + /* 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)) { + /* decrement refcount of + * previously selected router */ + if (router) + neigh_node_free_ref(router); + + router = tmp_neigh_node; + atomic_inc_not_zero(&router->refcount); + } + + 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 +1364,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; -- 1.7.2.3

From: Sven Eckelmann &lt;sven(a)narfation.org&gt; 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 &lt;sven(a)narfation.org&gt; Signed-off-by: Marek Lindner &lt;lindner_marek(a)yahoo.de&gt; --- gateway_client.c | 41 ++++++++++++++++++----------------------- types.h | 2 +- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c index 429a013..517e001 100644 --- a/batman-adv/gateway_client.c +++ b/batman-adv/gateway_client.c @@ -28,20 +28,18 @@ #include <linux/udp.h> #include <linux/if_vlan.h> -static void gw_node_free_ref(struct kref *refcount) -{ - struct gw_node *gw_node; - - gw_node = container_of(refcount, struct gw_node, refcount); - kfree(gw_node); -} - static void gw_node_free_rcu(struct rcu_head *rcu) { struct gw_node *gw_node; gw_node = container_of(rcu, struct gw_node, rcu); - kref_put(&gw_node->refcount, gw_node_free_ref); + kfree(gw_node); +} + +static void gw_node_free_ref(struct gw_node *gw_node) +{ + if (atomic_dec_and_test(&gw_node->refcount)) + call_rcu(&gw_node->rcu, gw_node_free_rcu); } void *gw_get_selected(struct bat_priv *bat_priv) @@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv) bat_priv->curr_gw = NULL; if (gw_node) - kref_put(&gw_node->refcount, gw_node_free_ref); + gw_node_free_ref(gw_node); } -static struct gw_node *gw_select(struct bat_priv *bat_priv, - struct gw_node *new_gw_node) +static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node) { struct gw_node *curr_gw_node = bat_priv->curr_gw; - if (new_gw_node) - kref_get(&new_gw_node->refcount); + if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount)) + new_gw_node = NULL; bat_priv->curr_gw = new_gw_node; - return curr_gw_node; + + if (curr_gw_node) + gw_node_free_ref(curr_gw_node); } void gw_election(struct bat_priv *bat_priv) { struct hlist_node *node; - struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL; + struct gw_node *gw_node, *curr_gw_tmp = NULL; uint8_t max_tq = 0; uint32_t max_gw_factor = 0, tmp_gw_factor = 0; int down, up; @@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv) curr_gw_tmp->orig_node->gw_flags, curr_gw_tmp->orig_node->router->tq_avg); - old_gw_node = gw_select(bat_priv, curr_gw_tmp); + gw_select(bat_priv, curr_gw_tmp); } rcu_read_unlock(); - - /* the kfree() has to be outside of the rcu lock */ - if (old_gw_node) - kref_put(&old_gw_node->refcount, gw_node_free_ref); } void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node) @@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv, memset(gw_node, 0, sizeof(struct gw_node)); INIT_HLIST_NODE(&gw_node->list); gw_node->orig_node = orig_node; - kref_init(&gw_node->refcount); + atomic_set(&gw_node->refcount, 1); spin_lock_bh(&bat_priv->gw_list_lock); hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list); @@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv) gw_deselect(bat_priv); hlist_del_rcu(&gw_node->list); - call_rcu(&gw_node->rcu, gw_node_free_rcu); + gw_node_free_ref(gw_node); } diff --git a/batman-adv/types.h b/batman-adv/types.h index e4a0462..ca5f20a 100644 --- a/batman-adv/types.h +++ b/batman-adv/types.h @@ -100,7 +100,7 @@ struct gw_node { struct hlist_node list; struct orig_node *orig_node; unsigned long deleted; - struct kref refcount; + atomic_t refcount; struct rcu_head rcu; }; -- 1.7.2.3

From: Sven Eckelmann &lt;sven(a)narfation.org&gt; 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 &lt;sven(a)narfation.org&gt; Signed-off-by: Marek Lindner &lt;lindner_marek(a)yahoo.de&gt; --- soft-interface.c | 35 +++++++++++++++++------------------ types.h | 2 +- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c index bd088f8..bd8b539 100644 --- a/batman-adv/soft-interface.c +++ b/batman-adv/soft-interface.c @@ -78,20 +78,18 @@ int my_skb_head_push(struct sk_buff *skb, unsigned int len) return 0; } -static void softif_neigh_free_ref(struct kref *refcount) -{ - struct softif_neigh *softif_neigh; - - softif_neigh = container_of(refcount, struct softif_neigh, refcount); - kfree(softif_neigh); -} - static void softif_neigh_free_rcu(struct rcu_head *rcu) { struct softif_neigh *softif_neigh; softif_neigh = container_of(rcu, struct softif_neigh, rcu); - kref_put(&softif_neigh->refcount, softif_neigh_free_ref); + kfree(softif_neigh); +} + +static void softif_neigh_free_ref(struct softif_neigh *softif_neigh) +{ + if (atomic_dec_and_test(&softif_neigh->refcount)) + call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu); } void softif_neigh_purge(struct bat_priv *bat_priv) @@ -118,11 +116,10 @@ void softif_neigh_purge(struct bat_priv *bat_priv) softif_neigh->addr, softif_neigh->vid); softif_neigh_tmp = bat_priv->softif_neigh; bat_priv->softif_neigh = NULL; - kref_put(&softif_neigh_tmp->refcount, - softif_neigh_free_ref); + softif_neigh_free_ref(softif_neigh_tmp); } - call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu); + softif_neigh_free_ref(softif_neigh); } spin_unlock_bh(&bat_priv->softif_neigh_lock); @@ -143,8 +140,11 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv, if (softif_neigh->vid != vid) continue; + if (!atomic_inc_not_zero(&softif_neigh->refcount)) + continue; + softif_neigh->last_seen = jiffies; - goto found; + goto out; } softif_neigh = kzalloc(sizeof(struct softif_neigh), GFP_ATOMIC); @@ -154,15 +154,14 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv, memcpy(softif_neigh->addr, addr, ETH_ALEN); softif_neigh->vid = vid; softif_neigh->last_seen = jiffies; - kref_init(&softif_neigh->refcount); + /* initialize with 2 - caller decrements counter by one */ + atomic_set(&softif_neigh->refcount, 2); INIT_HLIST_NODE(&softif_neigh->list); spin_lock_bh(&bat_priv->softif_neigh_lock); hlist_add_head_rcu(&softif_neigh->list, &bat_priv->softif_neigh_list); spin_unlock_bh(&bat_priv->softif_neigh_lock); -found: - kref_get(&softif_neigh->refcount); out: rcu_read_unlock(); return softif_neigh; @@ -266,7 +265,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev, softif_neigh->addr, softif_neigh->vid); softif_neigh_tmp = bat_priv->softif_neigh; bat_priv->softif_neigh = softif_neigh; - kref_put(&softif_neigh_tmp->refcount, softif_neigh_free_ref); + softif_neigh_free_ref(softif_neigh_tmp); /* we need to hold the additional reference */ goto err; } @@ -284,7 +283,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev, } out: - kref_put(&softif_neigh->refcount, softif_neigh_free_ref); + softif_neigh_free_ref(softif_neigh); err: kfree_skb(skb); return; diff --git a/batman-adv/types.h b/batman-adv/types.h index ca5f20a..8df6d9d 100644 --- a/batman-adv/types.h +++ b/batman-adv/types.h @@ -270,7 +270,7 @@ struct softif_neigh { uint8_t addr[ETH_ALEN]; unsigned long last_seen; short vid; - struct kref refcount; + atomic_t refcount; struct rcu_head rcu; }; -- 1.7.2.3

From: Sven Eckelmann &lt;sven(a)narfation.org&gt; 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 &lt;sven(a)narfation.org&gt; Signed-off-by: Marek Lindner &lt;lindner_marek(a)yahoo.de&gt; --- bat_sysfs.c | 20 +++++++++----------- hard-interface.c | 40 +++++++++++++++++++--------------------- hard-interface.h | 9 ++++----- types.h | 2 +- 4 files changed, 33 insertions(+), 38 deletions(-) diff --git a/batman-adv/bat_sysfs.c b/batman-adv/bat_sysfs.c index f7b93a0..93ae20a 100644 --- a/batman-adv/bat_sysfs.c +++ b/batman-adv/bat_sysfs.c @@ -450,7 +450,7 @@ static ssize_t show_mesh_iface(struct kobject *kobj, struct attribute *attr, length = sprintf(buff, "%s\n", batman_if->if_status == IF_NOT_IN_USE ? "none" : batman_if->soft_iface->name); - kref_put(&batman_if->refcount, hardif_free_ref); + hardif_free_ref(batman_if); return length; } @@ -461,7 +461,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, struct net_device *net_dev = kobj_to_netdev(kobj); struct batman_if *batman_if = get_batman_if_by_netdev(net_dev); int status_tmp = -1; - int ret; + int ret = count; if (!batman_if) return count; @@ -472,7 +472,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, if (strlen(buff) >= IFNAMSIZ) { pr_err("Invalid parameter for 'mesh_iface' setting received: " "interface name too long '%s'\n", buff); - kref_put(&batman_if->refcount, hardif_free_ref); + hardif_free_ref(batman_if); return -EINVAL; } @@ -482,17 +482,14 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, status_tmp = IF_I_WANT_YOU; if ((batman_if->if_status == status_tmp) || ((batman_if->soft_iface) && - (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) { - kref_put(&batman_if->refcount, hardif_free_ref); - return count; - } + (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) + goto out; if (status_tmp == IF_NOT_IN_USE) { rtnl_lock(); hardif_disable_interface(batman_if); rtnl_unlock(); - kref_put(&batman_if->refcount, hardif_free_ref); - return count; + goto out; } /* if the interface already is in use */ @@ -503,8 +500,9 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, } ret = hardif_enable_interface(batman_if, buff); - kref_put(&batman_if->refcount, hardif_free_ref); +out: + hardif_free_ref(batman_if); return ret; } @@ -537,7 +535,7 @@ static ssize_t show_iface_status(struct kobject *kobj, struct attribute *attr, break; } - kref_put(&batman_if->refcount, hardif_free_ref); + hardif_free_ref(batman_if); return length; } diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c index e2b001a..8982485 100644 --- a/batman-adv/hard-interface.c +++ b/batman-adv/hard-interface.c @@ -40,13 +40,13 @@ static int batman_skb_recv(struct sk_buff *skb, struct packet_type *ptype, struct net_device *orig_dev); -static void hardif_free_rcu(struct rcu_head *rcu) +void hardif_free_rcu(struct rcu_head *rcu) { struct batman_if *batman_if; batman_if = container_of(rcu, struct batman_if, rcu); dev_put(batman_if->net_dev); - kref_put(&batman_if->refcount, hardif_free_ref); + kfree(batman_if); } struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev) @@ -55,16 +55,14 @@ struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev) rcu_read_lock(); list_for_each_entry_rcu(batman_if, &if_list, list) { - if (batman_if->net_dev == net_dev) + if (batman_if->net_dev == net_dev && + atomic_inc_not_zero(&batman_if->refcount)) goto out; } batman_if = NULL; out: - if (batman_if) - kref_get(&batman_if->refcount); - rcu_read_unlock(); return batman_if; } @@ -105,16 +103,14 @@ static struct batman_if *get_active_batman_if(struct net_device *soft_iface) if (batman_if->soft_iface != soft_iface) continue; - if (batman_if->if_status == IF_ACTIVE) + if (batman_if->if_status == IF_ACTIVE && + atomic_inc_not_zero(&batman_if->refcount)) goto out; } batman_if = NULL; out: - if (batman_if) - kref_get(&batman_if->refcount); - rcu_read_unlock(); return batman_if; } @@ -137,14 +133,14 @@ static void set_primary_if(struct bat_priv *bat_priv, struct batman_packet *batman_packet; struct batman_if *old_if; - if (batman_if) - kref_get(&batman_if->refcount); + if (batman_if && !atomic_inc_not_zero(&batman_if->refcount)) + batman_if = NULL; old_if = bat_priv->primary_if; bat_priv->primary_if = batman_if; if (old_if) - kref_put(&old_if->refcount, hardif_free_ref); + hardif_free_ref(old_if); if (!bat_priv->primary_if) return; @@ -290,6 +286,9 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name) if (batman_if->if_status != IF_NOT_IN_USE) goto out; + if (!atomic_inc_not_zero(&batman_if->refcount)) + goto out; + batman_if->soft_iface = dev_get_by_name(&init_net, iface_name); if (!batman_if->soft_iface) { @@ -328,7 +327,6 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name) batman_if->batman_adv_ptype.type = __constant_htons(ETH_P_BATMAN); batman_if->batman_adv_ptype.func = batman_skb_recv; batman_if->batman_adv_ptype.dev = batman_if->net_dev; - kref_get(&batman_if->refcount); dev_add_pack(&batman_if->batman_adv_ptype); atomic_set(&batman_if->seqno, 1); @@ -371,6 +369,7 @@ out: return 0; err: + hardif_free_ref(batman_if); return -ENOMEM; } @@ -387,7 +386,6 @@ void hardif_disable_interface(struct batman_if *batman_if) bat_info(batman_if->soft_iface, "Removing interface: %s\n", batman_if->net_dev->name); dev_remove_pack(&batman_if->batman_adv_ptype); - kref_put(&batman_if->refcount, hardif_free_ref); bat_priv->num_ifaces--; orig_hash_del_if(batman_if, bat_priv->num_ifaces); @@ -399,7 +397,7 @@ void hardif_disable_interface(struct batman_if *batman_if) set_primary_if(bat_priv, new_if); if (new_if) - kref_put(&new_if->refcount, hardif_free_ref); + hardif_free_ref(new_if); } kfree(batman_if->packet_buff); @@ -416,6 +414,7 @@ void hardif_disable_interface(struct batman_if *batman_if) softif_destroy(batman_if->soft_iface); batman_if->soft_iface = NULL; + hardif_free_ref(batman_if); } static struct batman_if *hardif_add_interface(struct net_device *net_dev) @@ -445,7 +444,8 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev) batman_if->soft_iface = NULL; batman_if->if_status = IF_NOT_IN_USE; INIT_LIST_HEAD(&batman_if->list); - kref_init(&batman_if->refcount); + /* extra reference for return */ + atomic_set(&batman_if->refcount, 2); check_known_mac_addr(batman_if->net_dev); @@ -453,8 +453,6 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev) list_add_tail_rcu(&batman_if->list, &if_list); spin_unlock(&if_list_lock); - /* extra reference for return */ - kref_get(&batman_if->refcount); return batman_if; free_if: @@ -476,7 +474,7 @@ static void hardif_remove_interface(struct batman_if *batman_if) batman_if->if_status = IF_TO_BE_REMOVED; sysfs_del_hardif(&batman_if->hardif_obj); - call_rcu(&batman_if->rcu, hardif_free_rcu); + hardif_free_ref(batman_if); } void hardif_remove_interfaces(void) @@ -548,7 +546,7 @@ static int hard_if_event(struct notifier_block *this, }; hardif_put: - kref_put(&batman_if->refcount, hardif_free_ref); + hardif_free_ref(batman_if); out: return NOTIFY_DONE; } diff --git a/batman-adv/hard-interface.h b/batman-adv/hard-interface.h index ad19543..e488b90 100644 --- a/batman-adv/hard-interface.h +++ b/batman-adv/hard-interface.h @@ -37,13 +37,12 @@ void hardif_disable_interface(struct batman_if *batman_if); void hardif_remove_interfaces(void); int hardif_min_mtu(struct net_device *soft_iface); void update_min_mtu(struct net_device *soft_iface); +void hardif_free_rcu(struct rcu_head *rcu); -static inline void hardif_free_ref(struct kref *refcount) +static inline void hardif_free_ref(struct batman_if *batman_if) { - struct batman_if *batman_if; - - batman_if = container_of(refcount, struct batman_if, refcount); - kfree(batman_if); + if (atomic_dec_and_test(&batman_if->refcount)) + call_rcu(&batman_if->rcu, hardif_free_rcu); } #endif /* _NET_BATMAN_ADV_HARD_INTERFACE_H_ */ diff --git a/batman-adv/types.h b/batman-adv/types.h index 8df6d9d..317ede8 100644 --- a/batman-adv/types.h +++ b/batman-adv/types.h @@ -43,7 +43,7 @@ struct batman_if { unsigned char *packet_buff; int packet_len; struct kobject *hardif_obj; - struct kref refcount; + atomic_t refcount; struct packet_type batman_adv_ptype; struct net_device *soft_iface; struct rcu_head rcu; -- 1.7.2.3

The rcu protected macros rcu_dereference() and rcu_assign_pointer() for the bat_priv->curr_gw need to be used, as well as spin/rcu locking. Otherwise we might end up using a curr_gw pointer pointing to already freed memory. Reported-by: Sven Eckelmann &lt;sven(a)narfation.org&gt; Signed-off-by: Linus Lüssing &lt;linus.luessing(a)ascom.ch&gt; --- gateway_client.c | 87 ++++++++++++++++++++++++++++++++++++++--------------- main.c | 1 + types.h | 1 + 3 files changed, 64 insertions(+), 25 deletions(-) diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c index 517e001..4624515 100644 --- a/batman-adv/gateway_client.c +++ b/batman-adv/gateway_client.c @@ -44,19 +44,30 @@ static void gw_node_free_ref(struct gw_node *gw_node) void *gw_get_selected(struct bat_priv *bat_priv) { - struct gw_node *curr_gateway_tmp = bat_priv->curr_gw; + struct gw_node *curr_gateway_tmp; + struct orig_node *orig_node; - if (!curr_gateway_tmp) + rcu_read_lock(); + curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw); + if (!curr_gateway_tmp) { + rcu_read_unlock(); return NULL; + } - return curr_gateway_tmp->orig_node; + orig_node = curr_gateway_tmp->orig_node; + rcu_read_unlock(); + + return orig_node; } void gw_deselect(struct bat_priv *bat_priv) { - struct gw_node *gw_node = bat_priv->curr_gw; + struct gw_node *gw_node; - bat_priv->curr_gw = NULL; + spin_lock_bh(&bat_priv->curr_gw_lock); + gw_node = rcu_dereference(bat_priv->curr_gw); + rcu_assign_pointer(bat_priv->curr_gw, NULL); + spin_unlock_bh(&bat_priv->curr_gw_lock); if (gw_node) gw_node_free_ref(gw_node); @@ -64,12 +75,15 @@ void gw_deselect(struct bat_priv *bat_priv) static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node) { - struct gw_node *curr_gw_node = bat_priv->curr_gw; + struct gw_node *curr_gw_node; if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount)) new_gw_node = NULL; - bat_priv->curr_gw = new_gw_node; + spin_lock_bh(&bat_priv->curr_gw_lock); + curr_gw_node = rcu_dereference(bat_priv->curr_gw); + rcu_assign_pointer(bat_priv->curr_gw, new_gw_node); + spin_unlock_bh(&bat_priv->curr_gw_lock); if (curr_gw_node) gw_node_free_ref(curr_gw_node); @@ -78,7 +92,7 @@ static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node) void gw_election(struct bat_priv *bat_priv) { struct hlist_node *node; - struct gw_node *gw_node, *curr_gw_tmp = NULL; + struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL; uint8_t max_tq = 0; uint32_t max_gw_factor = 0, tmp_gw_factor = 0; int down, up; @@ -92,19 +106,24 @@ void gw_election(struct bat_priv *bat_priv) if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT) return; - if (bat_priv->curr_gw) + rcu_read_lock(); + curr_gw = rcu_dereference(bat_priv->curr_gw); + if (curr_gw) { + rcu_read_unlock(); return; + } - rcu_read_lock(); if (hlist_empty(&bat_priv->gw_list)) { - rcu_read_unlock(); - if (bat_priv->curr_gw) { + if (curr_gw) { + rcu_read_unlock(); bat_dbg(DBG_BATMAN, bat_priv, "Removing selected gateway - " "no gateway in range\n"); gw_deselect(bat_priv); } + else + rcu_read_unlock(); return; } @@ -153,12 +172,12 @@ void gw_election(struct bat_priv *bat_priv) max_gw_factor = tmp_gw_factor; } - if (bat_priv->curr_gw != curr_gw_tmp) { - if ((bat_priv->curr_gw) && (!curr_gw_tmp)) + if (curr_gw != curr_gw_tmp) { + if ((curr_gw) && (!curr_gw_tmp)) bat_dbg(DBG_BATMAN, bat_priv, "Removing selected gateway - " "no gateway in range\n"); - else if ((!bat_priv->curr_gw) && (curr_gw_tmp)) + else if ((!curr_gw) && (curr_gw_tmp)) bat_dbg(DBG_BATMAN, bat_priv, "Adding route to gateway %pM " "(gw_flags: %i, tq: %i)\n", @@ -181,11 +200,13 @@ void gw_election(struct bat_priv *bat_priv) void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node) { - struct gw_node *curr_gateway_tmp = bat_priv->curr_gw; + struct gw_node *curr_gateway_tmp; uint8_t gw_tq_avg, orig_tq_avg; + rcu_read_lock(); + curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw); if (!curr_gateway_tmp) - return; + goto rcu_unlock; if (!curr_gateway_tmp->orig_node) goto deselect; @@ -195,12 +216,14 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node) /* this node already is the gateway */ if (curr_gateway_tmp->orig_node == orig_node) - return; + goto rcu_unlock; if (!orig_node->router) - return; + goto rcu_unlock; gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg; + rcu_read_unlock(); + orig_tq_avg = orig_node->router->tq_avg; /* the TQ value has to be better */ @@ -222,6 +245,9 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node) deselect: gw_deselect(bat_priv); + return; +rcu_unlock: + rcu_read_unlock(); } static void gw_node_add(struct bat_priv *bat_priv, @@ -278,7 +304,7 @@ void gw_node_update(struct bat_priv *bat_priv, "Gateway %pM removed from gateway list\n", orig_node->orig); - if (gw_node == bat_priv->curr_gw) { + if (gw_node == rcu_dereference(bat_priv->curr_gw)) { rcu_read_unlock(); gw_deselect(bat_priv); return; @@ -316,7 +342,7 @@ void gw_node_purge(struct bat_priv *bat_priv) atomic_read(&bat_priv->mesh_state) == MESH_ACTIVE) continue; - if (bat_priv->curr_gw == gw_node) + if (rcu_dereference(bat_priv->curr_gw) == gw_node) gw_deselect(bat_priv); hlist_del_rcu(&gw_node->list); @@ -330,12 +356,16 @@ void gw_node_purge(struct bat_priv *bat_priv) static int _write_buffer_text(struct bat_priv *bat_priv, struct seq_file *seq, struct gw_node *gw_node) { - int down, up; + struct gw_node *curr_gw; + int down, up, ret; gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up); - return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n", - (bat_priv->curr_gw == gw_node ? "=>" : " "), + rcu_read_lock(); + curr_gw = rcu_dereference(bat_priv->curr_gw); + + ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n", + (curr_gw == gw_node ? "=>" : " "), gw_node->orig_node->orig, gw_node->orig_node->router->tq_avg, gw_node->orig_node->router->addr, @@ -345,6 +375,9 @@ static int _write_buffer_text(struct bat_priv *bat_priv, (down > 2048 ? "MBit" : "KBit"), (up > 2048 ? up / 1024 : up), (up > 2048 ? "MBit" : "KBit")); + + rcu_read_unlock(); + return ret; } int gw_client_seq_print_text(struct seq_file *seq, void *offset) @@ -465,8 +498,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb) if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER) return -1; - if (!bat_priv->curr_gw) + rcu_read_lock(); + if (!rcu_dereference(bat_priv->curr_gw)) { + rcu_read_unlock(); return 0; + } + rcu_read_unlock(); return 1; } diff --git a/batman-adv/main.c b/batman-adv/main.c index 658ad5a..6823868 100644 --- a/batman-adv/main.c +++ b/batman-adv/main.c @@ -84,6 +84,7 @@ int mesh_init(struct net_device *soft_iface) spin_lock_init(&bat_priv->hna_lhash_lock); spin_lock_init(&bat_priv->hna_ghash_lock); spin_lock_init(&bat_priv->gw_list_lock); + spin_lock_init(&bat_priv->curr_gw_lock); spin_lock_init(&bat_priv->vis_hash_lock); spin_lock_init(&bat_priv->vis_list_lock); spin_lock_init(&bat_priv->softif_neigh_lock); diff --git a/batman-adv/types.h b/batman-adv/types.h index ee77d48..4ae11c3 100644 --- a/batman-adv/types.h +++ b/batman-adv/types.h @@ -162,6 +162,7 @@ struct bat_priv { spinlock_t hna_lhash_lock; /* protects hna_local_hash */ spinlock_t hna_ghash_lock; /* protects hna_global_hash */ spinlock_t gw_list_lock; /* protects gw_list */ + spinlock_t curr_gw_lock; /* protects curr_gw updates */ spinlock_t vis_hash_lock; /* protects vis_hash */ spinlock_t vis_list_lock; /* protects vis_info::recv_list */ spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */ -- 1.7.2.3

On Tue, Feb 08, 2011 at 02:18:37PM +0100, Marek Lindner wrote: That's right. What do you think, is it worth an extra patch as it is not a problem which patch 5/7 does not really try to address? True, will add that rcu_read_unlock() in the deselect jump mark. True again. I would prefer just adding an rcu-lock here... ... as we are usually changing the gw_list more often than the curr_gw, so it's not really necessary to let a gw_list change for another node wait for a curr_gw_node reassignment to finish. What is speaking for using the same lock for both, just having less spinlocks in total in the code? Does anyone know how it is usually done in the rest of the kernel, are people tending to reduce the atomic context to the minimum amount needed? Or is it usually a trade-off between two many locks and and small atomic contexts? Cheers, Linus

When unicast_send_skb() is increasing the orig_node's refcount another thread might have been freeing this orig_node already. We need to increase the refcount in the rcu read lock protected area to avoid that. The same is true for get_orig_node(). Signed-off-by: Linus Lüssing &lt;linus.luessing(a)ascom.ch&gt; --- gateway_client.c | 3 +++ originator.c | 4 ++-- unicast.c | 1 - 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c index 4624515..b3cda22 100644 --- a/batman-adv/gateway_client.c +++ b/batman-adv/gateway_client.c @@ -55,6 +55,9 @@ void *gw_get_selected(struct bat_priv *bat_priv) } orig_node = curr_gateway_tmp->orig_node; + if (orig_node) + kref_get(&orig_node->refcount); + rcu_read_unlock(); return orig_node; diff --git a/batman-adv/originator.c b/batman-adv/originator.c index bde9778..6fb8393 100644 --- a/batman-adv/originator.c +++ b/batman-adv/originator.c @@ -193,12 +193,12 @@ struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr) orig_node = ((struct orig_node *)hash_find(bat_priv->orig_hash, compare_orig, choose_orig, addr)); - rcu_read_unlock(); - if (orig_node) { kref_get(&orig_node->refcount); + rcu_read_unlock(); return orig_node; } + rcu_read_unlock(); bat_dbg(DBG_BATMAN, bat_priv, "Creating new originator: %pM\n", addr); diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c index 580b547..f4f5115 100644 --- a/batman-adv/unicast.c +++ b/batman-adv/unicast.c @@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv) if (!orig_node) goto trans_search; - kref_get(&orig_node->refcount); goto find_router; } else { rcu_read_lock(); -- 1.7.2.3