Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Reduce refcnt of removed router when updating route

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]