Re: [B.A.T.M.A.N.] [PATCH 6/7] batman-adv: add bonding again

On Wednesday 09 October 2013 15:05:37 Simon Wunderlich wrote:

--- a/routing.c +++ b/routing.c @@ -407,16 +407,104 @@ static int batadv_check_unicast_packet(struct batadv_priv *bat_priv, struct batadv_neigh_node * batadv_find_router(struct batadv_priv *bat_priv, struct batadv_orig_node *orig_node,

•    const struct batadv_hard_iface *recv_if)
  

•    struct batadv_hard_iface *recv_if)
  

{

• struct batadv_neigh_node *router;

• struct batadv_algo_ops *bao = bat_priv->bat_algo_ops;

• struct batadv_neigh_node *first_candidate_router = NULL;

• struct batadv_neigh_node *next_candidate_router;

• struct batadv_neigh_node *router, *cand_router;

• struct batadv_orig_node_ifinfo *cand, *first_candidate = NULL;

• struct batadv_orig_node_ifinfo *next_candidate = NULL;

• bool last_candidate_found = false;

  if (!orig_node) return NULL;

  router = batadv_orig_node_get_router(orig_node, recv_if);

• /* TODO: fill this later with new bonding mechanism */

• /* only consider bonding for recv_if == NULL (first hop) and if

• * activated.
  

• */
  

• if (recv_if || !atomic_read(&bat_priv->bonding) || !router)

• return router;
  

• /* bonding: loop through the list of possible routers found

• * for the various outgoing interfaces and find a candidate after
  

• * the last chosen bonding candidate (next_candidate). If no such
  

• * router is found, use the first candidate found (the last chosen
  

• * bonding candidate might have been the last one in the list).
  

• * If this can't be found either, return the previously choosen
  

• * router - obviously there are no other candidates.
  

• */
  

• rcu_read_lock();
• hlist_for_each_entry_rcu(cand, &orig_node->ifinfo_list, list) {

• cand_router = rcu_dereference(cand->router);
  

• if (!cand_router)
  

• 	continue;
  

• if (!atomic_inc_not_zero(&cand_router->refcount))
  

• 	continue;
  

• /* alternative candidate should be good enough to be
  

•  * considered
  

•  */
  

• if (!bao->bat_neigh_is_equiv_or_better(cand_router,
  

• 				       cand->if_outgoing,
  

• 				       router, recv_if))
  

• 	goto next;
  

• /* don't use the same router twice */
  

• if (orig_node->last_bonding_candidate &&
  

•     (orig_node->last_bonding_candidate->router ==
  

•      cand_router))
  

• 		goto next;
  

Again, this if-statement won't pass.

• /* check if already passed the next candidate ... this function
  

•  * should the next candidate AFTER the last used bonding
  

•  * candidate.
  

•  */
  

• if (!orig_node->last_bonding_candidate ||
  

•     last_candidate_found) {
  

• 	next_candidate = cand;
  

• 	next_candidate_router = cand_router;
  

• 	break;
  

• }
  

The comment above would benefit from additional love. :)

• if (next_candidate) {

• /* found a possible candidate after the last chosen bonding
  

•  * candidate, return it.
  

•  */
  

• batadv_neigh_node_free_ref(router);
  

• if (first_candidate)
  

• 	batadv_neigh_node_free_ref(first_candidate_router);
  

• router = next_candidate_router;
  

• orig_node->last_bonding_candidate = next_candidate;
  

• } else if (first_candidate) {

• /* found no possible candidate after the last candidate, return
  

•  * the first candidate if available, or the already selected
  

•  * router otherwise.
  

•  */
  

• batadv_neigh_node_free_ref(router);
  

• router = first_candidate_router;
  

• orig_node->last_bonding_candidate = first_candidate;
  

• } else {

• orig_node->last_bonding_candidate = NULL;
  

• }

It is not safe to hold a pointer to first_candidate or next_candidate without having a refcounter protecting it.

Cheers, Marek