Hey Linus,
Thanks for the comments! I applied your proposed changes/fixes and will include them in the next patchset, a few comments below:
@@ -1171,15 +1232,19 @@ batadv_iv_ogm_update_seqnos(const struct ethhdr *ethhdr,
}
rcu_read_lock();
- hlist_for_each_entry_rcu(tmp_neigh_node,
&orig_node->neigh_list, list) {neigh_addr = tmp_neigh_node->addr;is_dup = batadv_test_bit(tmp_neigh_node->bat_iv.real_bits,
- hlist_for_each_entry_rcu(neigh_node, &orig_node->neigh_list, list) {
neigh_ifinfo = batadv_neigh_ifinfo_new(neigh_node,if_outgoing);hm, not quite sure, but would a batadv_neigh_ifinfo_get() be sufficient here?
I don't think so. batadv_iv_ogm_update_seqnos() is called before batadv_iv_ogm_orig_update() which is the other function to call _new(). We need batadv_iv_ogm_update_seqnos() to call _new to create the ifinfo and put the first mark in the bitfield.
+static void batadv_neigh_node_free_rcu(struct rcu_head *rcu) +{
- struct hlist_node *node_tmp;
- struct batadv_neigh_node *neigh_node;
- struct batadv_neigh_ifinfo *neigh_ifinfo;
- neigh_node = container_of(rcu, struct batadv_neigh_node, rcu);
- hlist_for_each_entry_safe(neigh_ifinfo, node_tmp,
&neigh_node->ifinfo_list, list) {batadv_neigh_ifinfo_free_ref_now(neigh_ifinfo);- }
two things are missing here: orig_node and if_incoming refcounters need to be decreased
This appears to be broken in the current code already: * batadv_iv_ogm_neigh_new() calls batadv_neigh_node_new() and increases the if_incoming refcount (shouldn't be that in batadv_neigh_node_new() instead?) * orig_node does not get its refcounter increased in both functions * both functions set hard_iface and orig_node within the neigh_node struct (that seems to be redundant) * the current code does not free_ref neigh->if_incoming
As we have a cyclic dependency here (neigh_node -> orig_node and vice versa), I'm not sure if we add more problems by adding refcounting to the orig_node here (when cleaning up, that is).
I'll therefore add the free_ref for if_incoming and keep the orig_node refcounting as it is.
why not simply using a "break" within the if-case here instead of the "!best" check and continuing to iterate over the list?
because we could have found a better neighbor then the previous "best", but not yet the "best" of the whole list. We need to check the whole list to find the best.
Thanks, Simon