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. > > 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). > > 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. > 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? > > -- > > :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
