On Sunday 20 March 2016 18:45:29 Marek Lindner wrote:
On Saturday, March 05, 2016 15:53:47 Sven Eckelmann wrote:
--- a/net/batman-adv/routing.c +++ b/net/batman-adv/routing.c @@ -104,6 +104,8 @@ static void _batadv_update_route(struct batadv_priv *bat_priv, neigh_node = NULL;
spin_lock_bh(&orig_node->neigh_list_lock);
curr_router = rcu_dereference_protected(orig_ifinfo->router,true);
rcu_assign_pointer(orig_ifinfo->router, neigh_node); spin_unlock_bh(&orig_node->neigh_list_lock); batadv_orig_ifinfo_free_ref(orig_ifinfo);Don't we also need to check for curr_router->refcount > 0 to mimic the check above ? Maybe a negative refcount does not hurt or is it unsigned ?
If this one gets negative then we would have a bug in a different place. The assignment only happens in this neigh_list_lock protected block. So the neigh_node behind orig_ifinfo->router must at least have a reference count of 1 or there was no valid reference (as in reference counter) for the pointer.
The the kref_get_unless_zero before was only necessary because the curr_router was aquired inside a rcu_read_lock protected region which is not perfectly in sync with its writers. So it could happen that rcu_dereference returned a pointer to a neigh_node but this neigh_node will be free'd (reference counter == 0). And we cannot get a valid reference for an object which has refcount of 0. This function avoids this problem by assuming that orig_ifinfo->router is NULL. This is not perfectly correct but better than having a pointer to free'd memory.
Kind regards, Sven