[B.A.T.M.A.N.] [PATCHv2 4/4] batman-adv: Correct rcu refcounting for neigh_node

<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 sven@narfation.org --- 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;