[quagga-dev 7394] zebra still deletes the wrong routes in some cases.

Lennart Sorensen lsorense at csclub.uwaterloo.ca
Fri Nov 20 20:48:33 GMT 2009


I just noticed while going through my local patches that the current
version of quagga still will match and delete the wrong routes from the
kernel's routing table in some cases.  I know I sent a patch for this
quite a while ago.

The problem is here:

  /* Lookup same type route. */
  for (rib = rn->info; rib; rib = rib->next)
    {
      if (CHECK_FLAG (rib->status, RIB_ENTRY_REMOVED))
        continue;

      if (CHECK_FLAG (rib->flags, ZEBRA_FLAG_SELECTED))
        fib = rib;

      if (rib->type != type)
        continue;
      if (rib->type == ZEBRA_ROUTE_CONNECT && (nexthop = rib->nexthop) &&
          nexthop->type == NEXTHOP_TYPE_IFINDEX && nexthop->ifindex == ifindex)
        {
          if (rib->refcnt)
            {
              rib->refcnt--;
              route_unlock_node (rn);
              route_unlock_node (rn);
              return 0;
            }
          same = rib;
          break;
        }
      /* Make sure that the route found has the same gateway. */
      else if (gate == NULL ||
               ((nexthop = rib->nexthop) &&
                (IPV4_ADDR_SAME (&nexthop->gate.ipv4, gate) ||
                 IPV4_ADDR_SAME (&nexthop->rgate.ipv4, gate))))
        {
          same = rib;
          break;
        }
    }

The idea here is to find a route matching the arguments passed in,
and if one is found we return it and then go delete it.  The bug is that
if gate == NULL, then we will match the first thing we find and delete
it, even though this isn't a match at all.

This is the part that is simply wrong:
      else if (gate == NULL ||
               ((nexthop = rib->nexthop) &&
                (IPV4_ADDR_SAME (&nexthop->gate.ipv4, gate) ||
                 IPV4_ADDR_SAME (&nexthop->rgate.ipv4, gate))))

This should be
      else if (gate != NULL &&
               ((nexthop = rib->nexthop) &&
                (IPV4_ADDR_SAME (&nexthop->gate.ipv4, gate) ||
                 IPV4_ADDR_SAME (&nexthop->rgate.ipv4, gate))))

Now it will only match routes that in fact match the request.

Clearly someone had a segfault at some point becuse gate=NULL and put
in the current code to avoid the segfault, but unfortunately got the
check backwards.  Elliminated the segfault, broke the behaviour.  The idea
was to only do the nexthop matching and the gate check was intended to
prevent a segfault, not to be a "sure grab this route no matter what it
is because gate==NULL".

This affects anyone with multiple identical routes (which happens if
you run ipsec using klips, so it is actually quite common).

So the patch is:

          break;
        }
       /* Make sure that the route found has the same gateway. */
-      else if (gate == NULL ||
+      else if (gate != NULL &&
               ((nexthop = rib->nexthop) &&
                (IPV4_ADDR_SAME (&nexthop->gate.ipv4, gate) ||
                 IPV4_ADDR_SAME (&nexthop->rgate.ipv4, gate))))

The other part right above in my patch set is:


       if (rib->type != type)
        continue;
-      if (rib->type == ZEBRA_ROUTE_CONNECT && (nexthop = rib->nexthop) &&
+      if ((rib->type == ZEBRA_ROUTE_CONNECT || (rib->type == ZEBRA_ROUTE_KERNEL && gate == NULL)) && (nexthop = rib->nexthop) &&
          nexthop->type == NEXTHOP_TYPE_IFINDEX && nexthop->ifindex == ifindex)
        {
          if (rib->refcnt)

Unfortunately I can't quite remember why that part was necesary, but I
am pretty sure it was related to the problem too.  Certainly the first
part is essential.

Can we please get this in before any 1.0 release ideas happen?

-- 
Len Sorensen



More information about the Quagga-dev mailing list