Skip to content

Commit

Permalink
Adding more fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
dgsudharsan committed Jul 24, 2024
1 parent 9a384ef commit 80d9c79
Show file tree
Hide file tree
Showing 25 changed files with 374 additions and 55 deletions.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
From bed7636621589c139ed8d83842df3ce438b8493f Mon Sep 17 00:00:00 2001
From: Chirag Shah <[email protected]>
Date: Mon, 17 Jun 2024 13:58:03 -0700
Subject: [PATCH] bgpd: backpressure - fix evpn route sync to zebra

In scaled EVPN + ipv4/ipv6 uni route sync to zebra,
some of the ipv4/ipv6 routes skipped reinstallation
due to incorrect local variable's stale value.

Once the local variable value reset in each loop
iteration all skipped routes synced to zebra properly.

Ticket: #3948828

Signed-off-by: Rajasekar Raja <[email protected]>
Signed-off-by: Chirag Shah <[email protected]>

diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c
index 5d5525156b..278e228d66 100644
--- a/bgpd/bgp_zebra.c
+++ b/bgpd/bgp_zebra.c
@@ -1801,6 +1801,8 @@ static void bgp_handle_route_announcements_to_zebra(struct thread *e)
bool install;

while (count < ZEBRA_ANNOUNCEMENTS_LIMIT) {
+ is_evpn = false;
+
dest = zebra_announce_pop(&bm->zebra_announce_head);

if (!dest)
--
2.43.2

Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
From 05d2c5b3ba6f83cd42a4dd5f9e40959fc438b0a6 Mon Sep 17 00:00:00 2001
From: Rajasekar Raja <[email protected]>
Date: Wed, 10 Jul 2024 16:46:29 -0700
Subject: [PATCH 1/2] bgpd: backpressure - fix to properly remove dest for bgp
under deletion

In case of imported routes (L3vni/vrf leaks), when a bgp instance is
being deleted, the peer->bgp comparision with the incoming bgp to remove
the dest from the pending fifo is wrong. This can lead to the fifo
having stale entries resulting in crash.

Two changes are done here.
- Instead of pop/push items in list if the struct bgp doesnt match,
simply iterate the list and remove the expected ones.

- Corrected the way bgp is fetched from dest rather than relying on
path_info->peer so that it works for all kinds of routes.

Ticket :#3980988

Signed-off-by: Chirag Shah <chirag @nvidia.com>
Signed-off-by: Rajasekar Raja <[email protected]>

diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c
index 2243ffdc77..b1f8f19594 100644
--- a/bgpd/bgp_evpn.c
+++ b/bgpd/bgp_evpn.c
@@ -6074,16 +6074,16 @@ struct bgpevpn *bgp_evpn_new(struct bgp *bgp, vni_t vni,
void bgp_evpn_free(struct bgp *bgp, struct bgpevpn *vpn)
{
struct bgp_dest *dest = NULL;
- uint32_t ann_count = zebra_announce_count(&bm->zebra_announce_head);
+ struct bgp_dest *dest_next = NULL;

- while (ann_count) {
- dest = zebra_announce_pop(&bm->zebra_announce_head);
- ann_count--;
+ for (dest = zebra_announce_first(&bm->zebra_announce_head); dest;
+ dest = dest_next) {
+ dest_next = zebra_announce_next(&bm->zebra_announce_head, dest);
if (dest->za_vpn == vpn) {
bgp_path_info_unlock(dest->za_bgp_pi);
bgp_dest_unlock_node(dest);
- } else
- zebra_announce_add_tail(&bm->zebra_announce_head, dest);
+ zebra_announce_del(&bm->zebra_announce_head, dest);
+ }
}

bgp_evpn_remote_ip_hash_destroy(vpn);
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 492566f8c8..e16a58b443 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -3689,19 +3689,25 @@ int bgp_delete(struct bgp *bgp)
safi_t safi;
int i;
struct bgp_dest *dest = NULL;
+ struct bgp_dest *dest_next = NULL;
+ struct bgp_table *dest_table = NULL;
struct graceful_restart_info *gr_info;
- uint32_t ann_count = zebra_announce_count(&bm->zebra_announce_head);

assert(bgp);

- while (ann_count) {
- dest = zebra_announce_pop(&bm->zebra_announce_head);
- ann_count--;
- if (dest->za_bgp_pi->peer->bgp == bgp) {
+ /*
+ * Iterate the pending dest list and remove all the dest pertaininig to
+ * the bgp under delete.
+ */
+ for (dest = zebra_announce_first(&bm->zebra_announce_head); dest;
+ dest = dest_next) {
+ dest_next = zebra_announce_next(&bm->zebra_announce_head, dest);
+ dest_table = bgp_dest_table(dest);
+ if (dest_table->bgp == bgp) {
bgp_path_info_unlock(dest->za_bgp_pi);
bgp_dest_unlock_node(dest);
- } else
- zebra_announce_add_tail(&bm->zebra_announce_head, dest);
+ zebra_announce_del(&bm->zebra_announce_head, dest);
+ }
}

bgp_soft_reconfig_table_task_cancel(bgp, NULL, NULL);
--
2.43.2

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
From 0dd44dc0d99b69e6c1853f46dbae4a30fc4b9aed Mon Sep 17 00:00:00 2001
From: Rajasekar Raja <[email protected]>
Date: Wed, 10 Jul 2024 20:17:14 -0700
Subject: [PATCH 2/2] bgpd: backpressure - Improve debuggability

Improve debuggability in backpressure code.

Ticket :#3980988

Signed-off-by: Rajasekar Raja <[email protected]>

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index e16a58b443..2e1c5e555b 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -3692,6 +3692,7 @@ int bgp_delete(struct bgp *bgp)
struct bgp_dest *dest_next = NULL;
struct bgp_table *dest_table = NULL;
struct graceful_restart_info *gr_info;
+ uint32_t cnt_before, cnt_after;

assert(bgp);

@@ -3699,6 +3700,7 @@ int bgp_delete(struct bgp *bgp)
* Iterate the pending dest list and remove all the dest pertaininig to
* the bgp under delete.
*/
+ cnt_before = zebra_announce_count(&bm->zebra_announce_head);
for (dest = zebra_announce_first(&bm->zebra_announce_head); dest;
dest = dest_next) {
dest_next = zebra_announce_next(&bm->zebra_announce_head, dest);
@@ -3710,6 +3712,11 @@ int bgp_delete(struct bgp *bgp)
}
}

+ cnt_after = zebra_announce_count(&bm->zebra_announce_head);
+ if (BGP_DEBUG(zebra, ZEBRA))
+ zlog_debug("Zebra Announce Fifo cleanup count before %u and after %u during BGP %s deletion",
+ cnt_before, cnt_after, bgp->name_pretty);
+
bgp_soft_reconfig_table_task_cancel(bgp, NULL, NULL);

/* make sure we withdraw any exported routes */
--
2.43.2

Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
From df1d28fcc12d4f5541c9335115887d31e6197b80 Mon Sep 17 00:00:00 2001
From: Rajasekar Raja <[email protected]>
Date: Mon, 22 Jul 2024 10:13:19 -0700
Subject: [PATCH] bgpd: backpressure - Avoid use after free

Coverity complains there is a use after free (1598495 and 1598496)
At this point, most likely dest->refcount cannot go 1 and free up
the dest, but there might be some code path where this can happen.

Fixing this with a simple order change (no harm fix).

Ticket :#4001204

Signed-off-by: Rajasekar Raja <[email protected]>

diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c
index b1f8f19594..bb3cd62950 100644
--- a/bgpd/bgp_evpn.c
+++ b/bgpd/bgp_evpn.c
@@ -6080,9 +6080,9 @@ void bgp_evpn_free(struct bgp *bgp, struct bgpevpn *vpn)
dest = dest_next) {
dest_next = zebra_announce_next(&bm->zebra_announce_head, dest);
if (dest->za_vpn == vpn) {
+ zebra_announce_del(&bm->zebra_announce_head, dest);
bgp_path_info_unlock(dest->za_bgp_pi);
bgp_dest_unlock_node(dest);
- zebra_announce_del(&bm->zebra_announce_head, dest);
}
}

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 2e1c5e555b..342982069b 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -3706,9 +3706,9 @@ int bgp_delete(struct bgp *bgp)
dest_next = zebra_announce_next(&bm->zebra_announce_head, dest);
dest_table = bgp_dest_table(dest);
if (dest_table->bgp == bgp) {
+ zebra_announce_del(&bm->zebra_announce_head, dest);
bgp_path_info_unlock(dest->za_bgp_pi);
bgp_dest_unlock_node(dest);
- zebra_announce_del(&bm->zebra_announce_head, dest);
}
}

--
2.43.2

Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
From cc56963da0e8f0ca606bc9b932e9180ad059f8c5 Mon Sep 17 00:00:00 2001
From: Rajasekar Raja <[email protected]>
Date: Tue, 16 Jul 2024 23:34:15 -0700
Subject: [PATCH 1/2] bgpd: backpressure - fix ret value
evpn_route_select_install

The return value of evpn_route_select_install is ignored in all cases
except during vni route table install/uninstall and based on the
returned value, an error is logged. Fixing this.

Ticket :#3992392

Signed-off-by: Rajasekar Raja <[email protected]>

diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c
index bb3cd62950..34128e7c19 100644
--- a/bgpd/bgp_evpn.c
+++ b/bgpd/bgp_evpn.c
@@ -1433,11 +1433,12 @@ int evpn_route_select_install(struct bgp *bgp, struct bgpevpn *vpn,
&& !bgp_addpath_is_addpath_used(&bgp->tx_addpath, afi, safi)) {
if (bgp_zebra_has_route_changed(old_select)) {
if (CHECK_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS))
- evpn_zebra_install(
- bgp, vpn,
- (const struct prefix_evpn *)
- bgp_dest_get_prefix(dest),
- old_select);
+ ret = evpn_zebra_install(bgp, vpn,
+ (const struct prefix_evpn
+ *)
+ bgp_dest_get_prefix(
+ dest),
+ old_select);
else
bgp_zebra_route_install(dest, old_select, bgp,
true, vpn, false);
@@ -1475,10 +1476,11 @@ int evpn_route_select_install(struct bgp *bgp, struct bgpevpn *vpn,
&& (new_select->sub_type == BGP_ROUTE_IMPORTED ||
bgp_evpn_attr_is_sync(new_select->attr))) {
if (CHECK_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS))
- evpn_zebra_install(bgp, vpn,
- (const struct prefix_evpn *)
- bgp_dest_get_prefix(dest),
- new_select);
+ ret = evpn_zebra_install(bgp, vpn,
+ (const struct prefix_evpn *)
+ bgp_dest_get_prefix(
+ dest),
+ new_select);
else
bgp_zebra_route_install(dest, new_select, bgp, true,
vpn, false);
@@ -1503,11 +1505,12 @@ int evpn_route_select_install(struct bgp *bgp, struct bgpevpn *vpn,
if (CHECK_FLAG(bgp->flags,
BGP_FLAG_DELETE_IN_PROGRESS) ||
CHECK_FLAG(bgp->flags, BGP_FLAG_VNI_DOWN))
- evpn_zebra_uninstall(
- bgp, vpn,
- (const struct prefix_evpn *)
- bgp_dest_get_prefix(dest),
- old_select, false);
+ ret = evpn_zebra_uninstall(bgp, vpn,
+ (const struct prefix_evpn
+ *)
+ bgp_dest_get_prefix(
+ dest),
+ old_select, false);
else
bgp_zebra_route_install(dest, old_select, bgp,
false, vpn, false);
--
2.43.2

Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
From dd591de04b0e25c74a9936c854bb6dbe7839bd39 Mon Sep 17 00:00:00 2001
From: Rajasekar Raja <[email protected]>
Date: Thu, 18 Jul 2024 22:23:23 -0700
Subject: [PATCH 2/2] bgpd: backpressure - log error for evpn when route
install to zebra fails.

log error for evpn in case route install to zebra fails.

Ticket :#3992392

Signed-off-by: Rajasekar Raja <[email protected]>

diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c
index 278e228d66..038d328a60 100644
--- a/bgpd/bgp_zebra.c
+++ b/bgpd/bgp_zebra.c
@@ -1799,6 +1799,7 @@ static void bgp_handle_route_announcements_to_zebra(struct thread *e)
struct bgp_table *table = NULL;
enum zclient_send_status status = ZCLIENT_SEND_SUCCESS;
bool install;
+ const struct prefix_evpn *evp = NULL;

while (count < ZEBRA_ANNOUNCEMENTS_LIMIT) {
is_evpn = false;
@@ -1809,10 +1810,12 @@ static void bgp_handle_route_announcements_to_zebra(struct thread *e)
break;

table = bgp_dest_table(dest);
- install =
- CHECK_FLAG(dest->flags, BGP_NODE_SCHEDULE_FOR_INSTALL);
- if (table->afi == AFI_L2VPN && table->safi == SAFI_EVPN)
+ install = CHECK_FLAG(dest->flags, BGP_NODE_SCHEDULE_FOR_INSTALL);
+ if (table->afi == AFI_L2VPN && table->safi == SAFI_EVPN) {
is_evpn = true;
+ evp = (const struct prefix_evpn *)bgp_dest_get_prefix(
+ dest);
+ }

if (BGP_DEBUG(zebra, ZEBRA))
zlog_debug(
@@ -1845,6 +1848,17 @@ static void bgp_handle_route_announcements_to_zebra(struct thread *e)
UNSET_FLAG(dest->flags, BGP_NODE_SCHEDULE_FOR_DELETE);
}

+ if (is_evpn && status == ZCLIENT_SEND_FAILURE)
+ flog_err(EC_BGP_EVPN_FAIL,
+ "%s (%u): Failed to %s EVPN %pFX %s route in VNI %u",
+ vrf_id_to_name(table->bgp->vrf_id),
+ table->bgp->vrf_id,
+ install ? "install" : "uninstall", evp,
+ evp->prefix.route_type == BGP_EVPN_MAC_IP_ROUTE
+ ? "MACIP"
+ : "IMET",
+ dest->za_vpn->vni);
+
bgp_path_info_unlock(dest->za_bgp_pi);
dest->za_bgp_pi = NULL;
dest->za_vpn = NULL;
--
2.43.2

Loading

0 comments on commit 80d9c79

Please sign in to comment.