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.
