On Thu, Aug 20, 2015 at 20:10 +0200, Martin Pieuchot wrote:
> On 20/08/15(Thu) 18:20, Mike Belopuhov wrote:
> > Makes you wonder why the heck it wasn't done in the first place,
> > doesn't it?
>
> If you look at the original CSRG source tree, you'll see how/why
> this happened :)
>
> When karels@ changed rtrequest() to turn it into a frontend for the
> radix tree in r7.8 of net/route.c RTM_DELETE was not returning a rt.
> At this point "rt_refcnt" was not that used.
>
> All this nightmare really started with the introduction of cloning
> routes. Until today you can see the mess in rtalloc(9) to increment
> the cloned vs cloning route depending on what happened. This was
> r7.13 of net/route.c and in this commit a call to rtrequest(RTM_DELETE)
> would first rtfree(9) the route given in argument *then* try to remove
> it from the routing table.
>
> Later in r7.23 rtrequest(RTM_DELETE) stopped to free the given route and
> rtinit() (what's today rt_ifa_del(9)) started to rtfree(9) the route
> itself always before removing it from the tree.
>
> Obviously this couldn't work and in r7.29 sklower@ made rtrequest()
> return the route in the RTM_DELETE case to be able to free it. At this
> point rt_refcnt was *not* incremented prior to rfree(9) a route. Note
> that a valid route in the tree without external reference has a count
> of 0. So in order to actually free the route the following pattern has
> been copy/pasted from rtrequest():
>
> if (rt->rt_refcnt <= 0)
> rtfree(rt);
>
> Finally in r7.31 this idiom has been changed to:
>
> if (rt->rt_refcnt <= 0) {
> rt->rt_refcnt++;
> rtfree(rt);
> }
>
> I guess as an "easy fix" because the panic() in rtfree(9) triggered when
> rtalloc(9) returned a cloning route instead of a cloned one without
> incrementing it's reference counter.
>
> Then this pattern has been copy/pasted everywhere, like most of the good
> and not so good stuff we find in this tree.
>
> All of that happened between 1988 and 1992, because of a bug in the
> reference count of RTF_CLONING routes, obviously we weren't their to
> do peer review ;)
>
> That's why I appreciate as much review as possible in this area, I'm
> sure we will have (or already do) similar issues.
>
Thanks a lot for taking your time to dig through this.
Your diff looks good to me apart from the small bit where
you've moved clearing "NDPRF_ONLINK" after the rt_sendmsg.
Isn't it better to clear it before userland will get the
notification?
