On 19/04/14(Sat) 16:10, Claudio Jeker wrote: > On Sat, Apr 19, 2014 at 03:09:40PM +0200, Martin Pieuchot wrote: > > On 18/04/14(Fri) 18:12, Claudio Jeker wrote: > > > Bad stuff happens when the ifa lookup tree gets corrupted. > > > In my case local traffic was suddenly no longer local and was > > > forwarded to lo0 ad infinitum. > > > > Which lookup exactly? > > > > in_ouraddr() in ip_input() started to freak out because the RB tree was > internally corrupt and in the worst case you would acutually panic while > checking the tree.
Whoa! > > > This was caused by the usage of rdomains and destroing pseudo interfaces. > > > The sadl address was still in rdomain 0, was therefor not found in the > > > tree and so you end up with a bad node and a use after free. > > > > I'm really interested in this case, because I think that there's no > > benefit of storing the link-layer addresses in the RB-tree. I believe > > we could even remove them from the tree, so I'd better fix the code > > relying this behavior. > > Yes, I'm not sure why we keep the sadl in the RB lookup tree. I think > nothing is using that (but every time I thought something did not make > sense in the network stack I was proven wrong so I'm more careful now). Agreed. > > > The following diff solves this by removing and readding the sadl to the RB > > > tree when switching rdomain. > > > > If you're doing that because of a call to ifa_ifwithaddr() I'd suggest > > to modify the calling function instead. I'd guess it's ifa_ifwithroute(), > > isn't it? > > This will not help to fix a corrupted tree. One could argue that we remove > the if_add / if_del from if_alloc_sadl / if_free_sadl but then something > else my break. The RB tree uses the ifp->if_rdomain in the lookup function > and if that changes all elements in that tree need to be reinserted. > The SIFRDOMAIN code removes all the IP and IPv6 addresses but not the sadl > one and that address is now corrupting the tree. I have a diff that does that actually, but indeed it needs more testing. > > Since link-layer addresses contain the index of their related interface, > > I don't see the point of doing a lookup. > > I would like to get this in since it is currently badly broken. e.g. it is > easily triggered by running the route regress test. Then we can discuss if > we can remove the sadl from the RB tree. > > > > > > > OK? ok mpi@, but could you please add an explicit comment explaining that we need to do that *because* of the RB-tree. So that when it dies, we don't leave them around. > > > -- > > > :wq Claudio > > > > > > Index: if.c > > > =================================================================== > > > RCS file: /cvs/src/sys/net/if.c,v > > > retrieving revision 1.283 > > > diff -u -p -r1.283 if.c > > > --- if.c 10 Apr 2014 13:47:21 -0000 1.283 > > > +++ if.c 18 Apr 2014 16:03:11 -0000 > > > @@ -1515,6 +1580,8 @@ ifioctl(struct socket *so, u_long cmd, c > > > #ifdef INET > > > in_ifdetach(ifp); > > > #endif > > > + /* remove sadl from ifa RB tree */ > > > + ifa_del(ifp, ifp->if_lladdr); > > > splx(s); > > > } > > > > > > @@ -1525,6 +1592,9 @@ ifioctl(struct socket *so, u_long cmd, c > > > > > > /* Add interface to the specified rdomain */ > > > ifp->if_rdomain = ifr->ifr_rdomainid; > > > + > > > + /* re-add sadl to the ifa RB tree in new rdomain */ > > > + ifa_add(ifp, ifp->if_lladdr); > > > break; > > > > > > case SIOCAIFGROUP: > > > > > > > -- > :wq Claudio >
