zebra: can't improve efficiency for recursive depends

cb86eba3ab3d82f540bdb9ed5f65d361ca301ea8 was causing zebra to crash
when handling a nexthop group that had a nexthop which was recursively resolved.

Steps to recreate:

!
nexthop-group red
 nexthop 1.1.1.1
 nexthop 1.1.1.2
!

sharp install routes 8.8.8.1 nexthop-group red 1

=========================================
==11898== Invalid write of size 8
==11898==    at 0x48E53B4: _nexthop_add_sorted (nexthop_group.c:254)
==11898==    by 0x48E5336: nexthop_group_add_sorted (nexthop_group.c:296)
==11898==    by 0x453593: handle_recursive_depend (zebra_nhg.c:481)
==11898==    by 0x451CA8: zebra_nhg_find (zebra_nhg.c:572)
==11898==    by 0x4530FB: zebra_nhg_find_nexthop (zebra_nhg.c:597)
==11898==    by 0x4536B4: depends_find (zebra_nhg.c:1065)
==11898==    by 0x453526: depends_find_add (zebra_nhg.c:1087)
==11898==    by 0x451C4D: zebra_nhg_find (zebra_nhg.c:567)
==11898==    by 0x4519DE: zebra_nhg_rib_find (zebra_nhg.c:1126)
==11898==    by 0x452268: nexthop_active_update (zebra_nhg.c:1729)
==11898==    by 0x461517: rib_process (zebra_rib.c:1049)
==11898==    by 0x4610C8: process_subq_route (zebra_rib.c:1967)
==11898==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

Zebra crashes because we weren't handling the case of the depend nexthop
being recursive.

For this case, we cannot make the function more efficient. A nexthop
could resolve to a group of any size, thus we need allocs/frees.

To solve this and retain the goal of the original patch, we separate out the
two cases so it will still be more efficient if the nexthop is not recursive.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
(cherry picked from commit 0fff714efa1959f48c8e1d88e88968d15c1ffe78)

diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c
index 2eb3cd9b42f48288759ab050bb35145cc247620d..4d0e1db94bf75580bfb8b62b8806057743232200 100644 (file)
--- a/zebra/zebra_nhg.c
+++ b/zebra/zebra_nhg.c
@@ -1048,18 +1048,41 @@ int zebra_nhg_kernel_del(uint32_t id)
 }
 
 /* Some dependency helper functions */
-static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi)
+static struct nhg_hash_entry *depends_find_recursive(const struct nexthop *nh,
+                                                    afi_t afi)
 {
-       struct nexthop lookup;
-       struct nhg_hash_entry *nhe = NULL;
+       struct nhg_hash_entry *nhe;
+       struct nexthop *lookup = NULL;
 
-       if (!nh)
-               goto done;
+       /*
+        * We need to copy its resolved nexthop if its recursively
+        * resolved so that has to be handled with allocs/frees since
+        * it could resolve to a group of unknown size.
+        */
+       copy_nexthops(&lookup, nh, NULL);
+
+       /* Make it a single, recursive nexthop */
+       nexthops_free(lookup->next);
+       nexthops_free(lookup->prev);
+       lookup->next = NULL;
+       lookup->prev = NULL;
+
+       nhe = zebra_nhg_find_nexthop(0, lookup, afi, 0);
+
+       nexthops_free(lookup);
+
+       return nhe;
+}
+
+static struct nhg_hash_entry *depends_find_singleton(const struct nexthop *nh,
+                                                    afi_t afi)
+{
+       struct nhg_hash_entry *nhe;
+       struct nexthop lookup = {};
 
        /* Capture a snapshot of this single nh; it might be part of a list,
         * so we need to make a standalone copy.
         */
-       memset(&lookup, 0, sizeof(lookup));
        nexthop_copy(&lookup, nh, NULL);
 
        nhe = zebra_nhg_find_nexthop(0, &lookup, afi, 0);
@@ -1067,6 +1090,24 @@ static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi)
        /* The copy may have allocated labels; free them if necessary. */
        nexthop_del_labels(&lookup);
 
+       return nhe;
+}
+
+static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi)
+{
+       struct nhg_hash_entry *nhe = NULL;
+
+       if (!nh)
+               goto done;
+
+       /* We are separating these functions out to increase handling speed
+        * in the non-recursive case (by not alloc/freeing)
+        */
+       if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_RECURSIVE))
+               nhe = depends_find_recursive(nh, afi);
+       else
+               nhe = depends_find_singleton(nh, afi);
+
 done:
        return nhe;
 }