From 0fff714efa1959f48c8e1d88e88968d15c1ffe78 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 31 Dec 2019 12:10:58 -0500 Subject: [PATCH 1/2] 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 --- zebra/zebra_nhg.c | 53 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 4f41406a5c..2be9238ee8 100644 --- 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; } From 77bf9504bfcdb977cda2addca27254e22be52f2f Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 13 Jan 2020 13:29:58 -0500 Subject: [PATCH 2/2] lib,zebra: tighten up the nexthop_copy/nexthop_dup APIs Make the nexthop_copy/nexthop_dup APIs more consistent by adding a secondary, non-recursive, version of them. Before, it was inconsistent whether the APIs were expected to copy recursive info or not. Make it clear now that the default is recursive info is copied unless the _no_recurse() version is called. These APIs are not heavily used so it is fine to change them for now. Signed-off-by: Stephen Worley --- lib/nexthop.c | 28 ++++++++++++++++++++++++++-- lib/nexthop.h | 7 +++++++ lib/nexthop_group.c | 4 ---- zebra/zebra_nhg.c | 15 ++------------- 4 files changed, 35 insertions(+), 19 deletions(-) diff --git a/lib/nexthop.c b/lib/nexthop.c index d2ab70e209..e23f8b0792 100644 --- a/lib/nexthop.c +++ b/lib/nexthop.c @@ -34,6 +34,7 @@ #include "jhash.h" #include "printfrr.h" #include "vrf.h" +#include "nexthop_group.h" DEFINE_MTYPE_STATIC(LIB, NEXTHOP, "Nexthop") DEFINE_MTYPE_STATIC(LIB, NH_LABEL, "Nexthop label") @@ -565,8 +566,9 @@ uint32_t nexthop_hash(const struct nexthop *nexthop) return key; } -void nexthop_copy(struct nexthop *copy, const struct nexthop *nexthop, - struct nexthop *rparent) +void nexthop_copy_no_recurse(struct nexthop *copy, + const struct nexthop *nexthop, + struct nexthop *rparent) { copy->vrf_id = nexthop->vrf_id; copy->ifindex = nexthop->ifindex; @@ -583,6 +585,28 @@ void nexthop_copy(struct nexthop *copy, const struct nexthop *nexthop, &nexthop->nh_label->label[0]); } +void nexthop_copy(struct nexthop *copy, const struct nexthop *nexthop, + struct nexthop *rparent) +{ + nexthop_copy_no_recurse(copy, nexthop, rparent); + + /* Bit of a special case here, we need to handle the case + * of a nexthop resolving to agroup. Hence, we need to + * use a nexthop_group API. + */ + if (CHECK_FLAG(copy->flags, NEXTHOP_FLAG_RECURSIVE)) + copy_nexthops(©->resolved, nexthop->resolved, copy); +} + +struct nexthop *nexthop_dup_no_recurse(const struct nexthop *nexthop, + struct nexthop *rparent) +{ + struct nexthop *new = nexthop_new(); + + nexthop_copy_no_recurse(new, nexthop, rparent); + return new; +} + struct nexthop *nexthop_dup(const struct nexthop *nexthop, struct nexthop *rparent) { diff --git a/lib/nexthop.h b/lib/nexthop.h index 040b643a84..cb5efe00cc 100644 --- a/lib/nexthop.h +++ b/lib/nexthop.h @@ -187,9 +187,16 @@ extern unsigned int nexthop_level(struct nexthop *nexthop); /* Copies to an already allocated nexthop struct */ extern void nexthop_copy(struct nexthop *copy, const struct nexthop *nexthop, struct nexthop *rparent); +/* Copies to an already allocated nexthop struct, not including recurse info */ +extern void nexthop_copy_no_recurse(struct nexthop *copy, + const struct nexthop *nexthop, + struct nexthop *rparent); /* Duplicates a nexthop and returns the newly allocated nexthop */ extern struct nexthop *nexthop_dup(const struct nexthop *nexthop, struct nexthop *rparent); +/* Duplicates a nexthop and returns the newly allocated nexthop */ +extern struct nexthop *nexthop_dup_no_recurse(const struct nexthop *nexthop, + struct nexthop *rparent); #ifdef __cplusplus } diff --git a/lib/nexthop_group.c b/lib/nexthop_group.c index 0051cba625..0f1a87cf6e 100644 --- a/lib/nexthop_group.c +++ b/lib/nexthop_group.c @@ -363,10 +363,6 @@ void copy_nexthops(struct nexthop **tnh, const struct nexthop *nh, for (nh1 = nh; nh1; nh1 = nh1->next) { nexthop = nexthop_dup(nh1, rparent); _nexthop_add(tnh, nexthop); - - if (CHECK_FLAG(nh1->flags, NEXTHOP_FLAG_RECURSIVE)) - copy_nexthops(&nexthop->resolved, nh1->resolved, - nexthop); } } diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 2be9238ee8..2a76fa61e9 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1054,18 +1054,7 @@ static struct nhg_hash_entry *depends_find_recursive(const struct nexthop *nh, struct nhg_hash_entry *nhe; struct nexthop *lookup = NULL; - /* - * 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; + lookup = nexthop_dup(nh, NULL); nhe = zebra_nhg_find_nexthop(0, lookup, afi, 0); @@ -1083,7 +1072,7 @@ static struct nhg_hash_entry *depends_find_singleton(const struct nexthop *nh, /* Capture a snapshot of this single nh; it might be part of a list, * so we need to make a standalone copy. */ - nexthop_copy(&lookup, nh, NULL); + nexthop_copy_no_recurse(&lookup, nh, NULL); nhe = zebra_nhg_find_nexthop(0, &lookup, afi, 0);