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
signature.asc
Description: This is a digitally signed message part.
