[B.A.T.M.A.N.] [PATCH maint] batman-adv: Reduce refcnt of removed router when updating route
_batadv_update_route rcu_derefences orig_ifinfo->router outside of a spinlock protected region to print some information messages to the debug log. But this pointer is not checked again when the new pointer is assigned in the spinlock protected region. Thus is can happen that the value of orig_ifinfo->router changed in the meantime and thus the reference counter of the wrong router gets reduced after the spinlock protected region.
Just rcu_dereferencing the value of orig_ifinfo->router inside the spinlock protected region (which also set the new pointer) is enough to get the correct old router object.
Fixes: d90ddb94423f ("batman-adv: Make orig_node->router an rcu protected pointer") Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/routing.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c index 1fb1be3..61fdefe 100644 --- 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);
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 ?
Cheers, Marek
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
b.a.t.m.a.n@lists.open-mesh.org
-
Marek Lindner -
Sven Eckelmann