From baf6f62e12190f88f79dc10578aabfefc0cc69f6 Mon Sep 17 00:00:00 2001 From: mssonicbld <79238446+mssonicbld@users.noreply.github.com> Date: Thu, 18 Apr 2024 16:01:12 +0800 Subject: [PATCH 1/5] [submodule] Update submodule sonic-platform-daemons to the latest HEAD automatically (#18706) #### Why I did it src/sonic-platform-daemons ``` * d45b982 - (HEAD -> 202311, origin/202311) [chassis][linecard] Fix Module LINECARD<> went off-line message for empty slot issue (#462) (4 hours ago) [Marty Y. Lok] ``` #### How I did it #### How to verify it #### Description for the changelog --- src/sonic-platform-daemons | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-platform-daemons b/src/sonic-platform-daemons index 4ed4fa885a70..d45b982eef17 160000 --- a/src/sonic-platform-daemons +++ b/src/sonic-platform-daemons @@ -1 +1 @@ -Subproject commit 4ed4fa885a70fc34a5411187bd9fcdae401eb5f8 +Subproject commit d45b982eef176f815eb2d80b56348c17b84e0616 From 2f9bc000166e30a5ffbb66eaba6b76def6103a73 Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Thu, 18 Apr 2024 02:29:23 -0700 Subject: [PATCH 2/5] [FRR]Adding fix for memory leak seen with BGP community (#18606) Porting PR #18272 to 202311. Why I did it Porting fix for FRRouting/frr#15459 Adding patch for PR FRRouting/frr#15466 Work item tracking Microsoft ADO (number only): How I did it Ported the fix FRRouting/frr#15466 to 8.5.1 How to verify it Running the test_stress_route and ensure no memory leak --- .../0037-bgp-community-memory-leak-fix.patch | 303 ++++++++++++++++++ src/sonic-frr/patch/series | 1 + 2 files changed, 304 insertions(+) create mode 100644 src/sonic-frr/patch/0037-bgp-community-memory-leak-fix.patch diff --git a/src/sonic-frr/patch/0037-bgp-community-memory-leak-fix.patch b/src/sonic-frr/patch/0037-bgp-community-memory-leak-fix.patch new file mode 100644 index 000000000000..8d1ff02d46bf --- /dev/null +++ b/src/sonic-frr/patch/0037-bgp-community-memory-leak-fix.patch @@ -0,0 +1,303 @@ +From 2b8e4e4b93a78e5884e2e5b97050b4ea3843e2e0 Mon Sep 17 00:00:00 2001 +From: Donald Sharp +Date: Thu, 7 Mar 2024 22:18:18 -0500 +Subject: [PATCH 1/2] bgpd: Combined patch to clean up filter leaks + +Customer has this valgrind trace: + +Direct leak of 2829120 byte(s) in 70728 object(s) allocated from: + 0 in community_new ../bgpd/bgp_community.c:39 + 1 in community_uniq_sort ../bgpd/bgp_community.c:170 + 2 in route_set_community ../bgpd/bgp_routemap.c:2342 + 3 in route_map_apply_ext ../lib/routemap.c:2673 + 4 in subgroup_announce_check ../bgpd/bgp_route.c:2367 + 5 in subgroup_process_announce_selected ../bgpd/bgp_route.c:2914 + 6 in group_announce_route_walkcb ../bgpd/bgp_updgrp_adv.c:199 + 7 in hash_walk ../lib/hash.c:285 + 8 in update_group_af_walk ../bgpd/bgp_updgrp.c:2061 + 9 in group_announce_route ../bgpd/bgp_updgrp_adv.c:1059 + 10 in bgp_process_main_one ../bgpd/bgp_route.c:3221 + 11 in bgp_process_wq ../bgpd/bgp_route.c:3221 + 12 in work_queue_run ../lib/workqueue.c:282 + +The above leak detected by valgrind was from a screenshot so I copied it +by hand. Any mistakes in line numbers are purely from my transcription. +Additionally this is against a slightly modified 8.5.1 version of FRR. +Code inspection of 8.5.1 -vs- latest master shows the same problem +exists. Code should be able to be followed from there to here. + +What is happening: + +There is a route-map being applied that modifes the outgoing community +to a peer. This is saved in the attr copy created in +subgroup_process_announce_selected. This community pointer is not +interned. So the community->refcount is still 0. Normally when +a prefix is announced, the attr and the prefix are placed on a +adjency out structure where the attribute is interned. This will +cause the community to be saved in the community hash list as well. +In a non-normal operation when the decision to send is aborted after +the route-map application, the attribute is just dropped and the +pointer to the community is just dropped too, leading to situations +where the memory is leaked. The usage of bgp suppress-fib would +would be a case where the community is caused to be leaked. +Additionally the previous commit where an unsuppress-map is used +to modify the outgoing attribute but since unsuppress-map was +not considered part of outgoing policy the attribute would be dropped as +well. This pointer drop also extends to any dynamically allocated +memory saved by the attribute pointer that was not interned yet as well. + +So let's modify the return case where the decision is made to +not send the prefix to the peer to always just flush the attribute +to ensure memory is not leaked. + +Fixes: #15459 +Signed-off-by: Donald Sharp +--- + bgpd/bgp_conditional_adv.c | 5 +++-- + bgpd/bgp_route.c | 19 +++++++++++------ + bgpd/bgp_updgrp.h | 2 +- + bgpd/bgp_updgrp_adv.c | 53 ++++++++++++++++++++++++++-------------------- + 4 files changed, 47 insertions(+), 32 deletions(-) + +diff --git a/bgpd/bgp_conditional_adv.c b/bgpd/bgp_conditional_adv.c +index 4ad00ed121..89ea85ff46 100644 +--- a/bgpd/bgp_conditional_adv.c ++++ b/bgpd/bgp_conditional_adv.c +@@ -134,8 +134,9 @@ static void bgp_conditional_adv_routes(struct peer *peer, afi_t afi, + if (update_type == UPDATE_TYPE_ADVERTISE && + subgroup_announce_check(dest, pi, subgrp, dest_p, + &attr, &advmap_attr)) { +- bgp_adj_out_set_subgroup(dest, subgrp, &attr, +- pi); ++ if (!bgp_adj_out_set_subgroup(dest, subgrp, ++ &attr, pi)) ++ bgp_attr_flush(&attr); + } else { + /* If default originate is enabled for + * the peer, do not send explicit +diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c +index a7a5c9849a..157bfa8a2b 100644 +--- a/bgpd/bgp_route.c ++++ b/bgpd/bgp_route.c +@@ -2917,16 +2917,23 @@ void subgroup_process_announce_selected(struct update_subgroup *subgrp, + * in FIB, then it is advertised + */ + if (advertise) { +- if (!bgp_check_withdrawal(bgp, dest)) +- bgp_adj_out_set_subgroup( +- dest, subgrp, &attr, selected); +- else ++ if (!bgp_check_withdrawal(bgp, dest)) { ++ if (!bgp_adj_out_set_subgroup( ++ dest, subgrp, &attr, ++ selected)) ++ bgp_attr_flush(&attr); ++ } else { + bgp_adj_out_unset_subgroup( + dest, subgrp, 1, addpath_tx_id); +- } +- } else ++ bgp_attr_flush(&attr); ++ } ++ } else ++ bgp_attr_flush(&attr); ++ } else { + bgp_adj_out_unset_subgroup(dest, subgrp, 1, + addpath_tx_id); ++ bgp_attr_flush(&attr); ++ } + } + + /* If selected is NULL we must withdraw the path using addpath_tx_id */ +diff --git a/bgpd/bgp_updgrp.h b/bgpd/bgp_updgrp.h +index e27c1e7b67..b7b6aa07e9 100644 +--- a/bgpd/bgp_updgrp.h ++++ b/bgpd/bgp_updgrp.h +@@ -458,7 +458,7 @@ extern struct bgp_adj_out *bgp_adj_out_alloc(struct update_subgroup *subgrp, + extern void bgp_adj_out_remove_subgroup(struct bgp_dest *dest, + struct bgp_adj_out *adj, + struct update_subgroup *subgrp); +-extern void bgp_adj_out_set_subgroup(struct bgp_dest *dest, ++extern bool bgp_adj_out_set_subgroup(struct bgp_dest *dest, + struct update_subgroup *subgrp, + struct attr *attr, + struct bgp_path_info *path); +diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c +index af8ef751da..301a8b267e 100644 +--- a/bgpd/bgp_updgrp_adv.c ++++ b/bgpd/bgp_updgrp_adv.c +@@ -454,7 +454,7 @@ bgp_advertise_clean_subgroup(struct update_subgroup *subgrp, + return next; + } + +-void bgp_adj_out_set_subgroup(struct bgp_dest *dest, ++bool bgp_adj_out_set_subgroup(struct bgp_dest *dest, + struct update_subgroup *subgrp, struct attr *attr, + struct bgp_path_info *path) + { +@@ -474,7 +474,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, + bgp = SUBGRP_INST(subgrp); + + if (DISABLE_BGP_ANNOUNCE) +- return; ++ return false; + + /* Look for adjacency information. */ + adj = adj_lookup( +@@ -490,7 +490,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, + bgp_addpath_id_for_peer(peer, afi, safi, + &path->tx_addpath)); + if (!adj) +- return; ++ return false; + + subgrp->pscount++; + } +@@ -529,7 +529,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, + * will never be able to coalesce the 3rd peer down + */ + subgrp->version = MAX(subgrp->version, dest->version); +- return; ++ return false; + } + + if (adj->adv) +@@ -576,6 +576,8 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, + bgp_adv_fifo_add_tail(&subgrp->sync->update, adv); + + subgrp->version = MAX(subgrp->version, dest->version); ++ ++ return true; + } + + /* The only time 'withdraw' will be false is if we are sending +@@ -668,7 +670,7 @@ void subgroup_announce_table(struct update_subgroup *subgrp, + { + struct bgp_dest *dest; + struct bgp_path_info *ri; +- struct attr attr; ++ struct attr attr = {0}; + struct peer *peer; + afi_t afi; + safi_t safi; +@@ -715,19 +717,24 @@ void subgroup_announce_table(struct update_subgroup *subgrp, + &attr, NULL)) { + /* Check if route can be advertised */ + if (advertise) { +- if (!bgp_check_withdrawal(bgp, dest)) +- bgp_adj_out_set_subgroup( +- dest, subgrp, &attr, +- ri); +- else ++ if (!bgp_check_withdrawal(bgp, dest)) { ++ if (!bgp_adj_out_set_subgroup( ++ dest, subgrp, &attr, ++ ri)) ++ bgp_attr_flush(&attr); ++ } else { ++ bgp_attr_flush(&attr); + bgp_adj_out_unset_subgroup( + dest, subgrp, 1, + bgp_addpath_id_for_peer( + peer, afi, + safi_rib, + &ri->tx_addpath)); +- } ++ } ++ } else ++ bgp_attr_flush(&attr); + } else { ++ bgp_attr_flush(&attr); + /* If default originate is enabled for + * the peer, do not send explicit + * withdraw. This will prevent deletion +@@ -947,18 +954,18 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) + if (dest) { + for (pi = bgp_dest_get_bgp_path_info(dest); pi; + pi = pi->next) { +- if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) +- if (subgroup_announce_check( +- dest, pi, subgrp, +- bgp_dest_get_prefix(dest), +- &attr, NULL)) { +- struct attr *default_attr = +- bgp_attr_intern(&attr); +- +- bgp_adj_out_set_subgroup( +- dest, subgrp, +- default_attr, pi); +- } ++ if (!CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) ++ continue; ++ ++ if (subgroup_announce_check( ++ dest, pi, subgrp, ++ bgp_dest_get_prefix(dest), &attr, ++ NULL)) { ++ if (!bgp_adj_out_set_subgroup( ++ dest, subgrp, &attr, pi)) ++ bgp_attr_flush(&attr); ++ } else ++ bgp_attr_flush(&attr); + } + bgp_dest_unlock_node(dest); + } +-- +2.14.1 + + +From 761907075520aa3fae70a8d18fa717a24d3cbd65 Mon Sep 17 00:00:00 2001 +From: Donald Sharp +Date: Wed, 13 Mar 2024 10:26:58 -0400 +Subject: [PATCH 2/2] bgpd: Ensure that the correct aspath is free'd + +Currently in subgroup_default_originate the attr.aspath +is set in bgp_attr_default_set, which hashs the aspath +and creates a refcount for it. If this is a withdraw +the subgroup_announce_check and bgp_adj_out_set_subgroup +is called which will intern the attribute. This will +cause the the attr.aspath to be set to a new value +finally at the bottom of the function it intentionally +uninterns the aspath which is not the one that was +created for this function. This reduces the other +aspath's refcount by 1 and if a clear bgp * is issued +fast enough the aspath for that will be removed +and the system will crash. + +Signed-off-by: Donald Sharp +--- + bgpd/bgp_updgrp_adv.c | 4 +++- + 1 file changed, 3 insertions(+), 1 deletion(-) + +diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c +index 301a8b267e..75e377f9a1 100644 +--- a/bgpd/bgp_updgrp_adv.c ++++ b/bgpd/bgp_updgrp_adv.c +@@ -817,6 +817,7 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) + struct bgp *bgp; + struct attr attr; + struct attr *new_attr = &attr; ++ struct aspath *aspath; + struct prefix p; + struct peer *from; + struct bgp_dest *dest; +@@ -854,6 +855,7 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) + /* make coverity happy */ + assert(attr.aspath); + ++ aspath = attr.aspath; + attr.med = 0; + attr.flag |= ATTR_FLAG_BIT(BGP_ATTR_MULTI_EXIT_DISC); + +@@ -1009,7 +1011,7 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) + } + } + +- aspath_unintern(&attr.aspath); ++ aspath_unintern(&aspath); + } + + /* +-- +2.14.1 + diff --git a/src/sonic-frr/patch/series b/src/sonic-frr/patch/series index 3c9c6e6acf25..fcc15bac8ffc 100644 --- a/src/sonic-frr/patch/series +++ b/src/sonic-frr/patch/series @@ -33,3 +33,4 @@ cross-compile-changes.patch 0034-fpm-Use-vrf_id-for-vrf-not-tabled_id.patch 0035-fpm-ignore-route-from-default-table.patch 0036-Add-support-of-bgp-l3vni-evpn.patch +0037-bgp-community-memory-leak-fix.patch From 1cb9dd5190fbca525e5f4e238dca4afa1cb1b052 Mon Sep 17 00:00:00 2001 From: Tejaswini Chadaga <85581939+tjchadaga@users.noreply.github.com> Date: Tue, 16 Apr 2024 21:08:28 -0700 Subject: [PATCH 3/5] Add dependency for tsa_enabled flag before populating peer config and bringing up BGP (#18556) Why I did it Ensure BGP peer bring-up and route advertisements are done only after checking TSA status during reload Work item tracking Microsoft ADO (number only): 27171112 How I did it Add dependency on tsa_enabled flag before peer configuration How to verify it Validate that the BGP session bring up is not complete until tsa_enabled flag is populated. Ensure no traffic is drawn to the device when in TSA and reboot with BGP unshut. --- src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py index b15f841f6166..306b33e32cc4 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py @@ -107,6 +107,7 @@ def __init__(self, common_objs, db_name, table_name, peer_type, check_neig_meta) deps = [ ("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"), ("CONFIG_DB", swsscommon.CFG_LOOPBACK_INTERFACE_TABLE_NAME, "Loopback0"), + ("CONFIG_DB", swsscommon.CFG_BGP_DEVICE_GLOBAL_TABLE_NAME, "tsa_enabled"), ("LOCAL", "local_addresses", ""), ("LOCAL", "interfaces", ""), ] From 09b0423cccfdf3a1c293cfb16a0466d071a5c1cc Mon Sep 17 00:00:00 2001 From: Kebo Liu Date: Sun, 14 Apr 2024 21:35:26 +0800 Subject: [PATCH 4/5] Support to fetch pmon_daemon_control.json file from both platform and HWSKU folders (#18441) - Why I did it The pmon_daemon_control.json file currently has a platform-specific scope, resulting in all SKUs of the same platform sharing the same pmon daemon configuration. Consequently, this restricts the ability to have distinct pmon daemon configurations for different SKUs. The proposed change aims to enhance flexibility by allowing PMON to load this file from both the platform folder and the HWSKU folder. In case a pmon_daemon_control.json file exists in the HWSKU folder, it will take precedence over the one in the platform folder. - How I did it The pmon docker init script will be updated to prioritize the HWSKU folder when searching for the pmon_daemon_control.json file. If the file is present in the HWSKU folder, it will be utilized. However, if the file does not exist in the HWSKU folder, the script will then fallback to utilizing the one located in the platform folder. - How to verify it Build image with this change, put the file in 1. platform folder, 2. in HWSKU folder 3. in both folders make sure the pmon daemons started according to the configuration. Signed-off-by: Kebo Liu --- dockers/docker-platform-monitor/docker_init.j2 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dockers/docker-platform-monitor/docker_init.j2 b/dockers/docker-platform-monitor/docker_init.j2 index d8667296efae..46766b5fd171 100755 --- a/dockers/docker-platform-monitor/docker_init.j2 +++ b/dockers/docker-platform-monitor/docker_init.j2 @@ -8,7 +8,6 @@ FANCONTROL_CONF_FILE="/usr/share/sonic/platform/fancontrol" SUPERVISOR_CONF_TEMPLATE="/usr/share/sonic/templates/docker-pmon.supervisord.conf.j2" SUPERVISOR_CONF_FILE="/etc/supervisor/conf.d/supervisord.conf" -PMON_DAEMON_CONTROL_FILE="/usr/share/sonic/platform/pmon_daemon_control.json" MODULAR_CHASSISDB_CONF_FILE="/usr/share/sonic/platform/chassisdb.conf" HAVE_SENSORS_CONF=0 @@ -17,6 +16,13 @@ IS_MODULAR_CHASSIS=0 # Default use python2 version SONIC_PLATFORM_API_PYTHON_VERSION=2 +if [ -e /usr/share/sonic/hwsku/pmon_daemon_control.json ]; +then + PMON_DAEMON_CONTROL_FILE="/usr/share/sonic/hwsku/pmon_daemon_control.json" +else + PMON_DAEMON_CONTROL_FILE="/usr/share/sonic/platform/pmon_daemon_control.json" +fi + declare -r EXIT_SUCCESS="0" if [ "${RUNTIME_OWNER}" == "" ]; then From 29aabd0a7c841f37c653750d1da2bb711944f05a Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Thu, 18 Apr 2024 02:44:08 +0000 Subject: [PATCH 5/5] [202311][FRR]Fix EVPN route type not matching route map --- ...rt-EVPN-prefixes-into-IPv4-IPv6-if-n.patch | 278 ++++++++++++++++++ src/sonic-frr/patch/series | 1 + 2 files changed, 279 insertions(+) create mode 100644 src/sonic-frr/patch/0038-lib-Do-not-convert-EVPN-prefixes-into-IPv4-IPv6-if-n.patch diff --git a/src/sonic-frr/patch/0038-lib-Do-not-convert-EVPN-prefixes-into-IPv4-IPv6-if-n.patch b/src/sonic-frr/patch/0038-lib-Do-not-convert-EVPN-prefixes-into-IPv4-IPv6-if-n.patch new file mode 100644 index 000000000000..361aac9e2db6 --- /dev/null +++ b/src/sonic-frr/patch/0038-lib-Do-not-convert-EVPN-prefixes-into-IPv4-IPv6-if-n.patch @@ -0,0 +1,278 @@ +From 799c131b6a41262322a68252e44624e052b23cfa Mon Sep 17 00:00:00 2001 +From: Trey Aspelund +Date: Fri, 17 Feb 2023 21:47:09 +0000 +Subject: [PATCH 1/2] lib: skip route-map optimization if !AF_INET(6) + +Currently we unconditionally send a prefix through the optimized +route-map codepath if the v4 and v6 LPM tables have been allocated and +optimization has not been disabled. +However prefixes from address-families that are not IPv4/IPv6 unicast +always fail the optimized route-map index lookup, because they occur on +an LPM tree that is IPv4 or IPv6 specific. +e.g. +Even if you have an empty permit route-map clause, Type-3 EVPN routes +are always denied: +``` +--config +route-map soo-foo permit 10 + +--logs +2023/02/17 19:38:42 BGP: [KZK58-6T4Y6] No best match sequence for pfx: [3]:[0]:[32]:[2.2.2.2] in route-map: soo-foo, result: no match +2023/02/17 19:38:42 BGP: [H5AW4-JFYQC] Route-map: soo-foo, prefix: [3]:[0]:[32]:[2.2.2.2], result: deny +``` + +There is some existing code that creates an AF_INET/AF_INET6 prefix +using the IP/prefix information from a Type-2/5 EVPN route, which +allowed only these two route-types to successfully attempt an LPM lookup +in the route-map optimization trees via the converted prefix. + +This commit does 3 things: +1) Reverts to non-optimized route-map lookup for prefixes that are not + AF_INET or AF_INET6. +2) Cleans up the route-map code so that the AF check is part of the + index lookup + the EVPN RT-2/5 -> AF_INET/6 prefix conversion occurs + outside the index lookup. +3) Adds "debug route-map detail" logs to indicate when we attempt to + convert an AF_EVPN prefix into an AF_INET/6 prefix + when we fallback + to a non-optimized lookup. + +Additional functionality for optimized lookups of prefixes from other +address-families can be added prior to the index lookup, similar to how +the existing EVPN conversion works today. + +New behavior: +``` +2023/02/17 21:44:27 BGP: [WYP1M-NE4SY] Converted EVPN prefix [5]:[0]:[32]:[192.0.2.7] into 192.0.2.7/32 for optimized route-map lookup +2023/02/17 21:44:27 BGP: [MT1SJ-WEJQ1] Best match route-map: soo-foo, sequence: 10 for pfx: 192.0.2.7/32, result: match +2023/02/17 21:44:27 BGP: [H5AW4-JFYQC] Route-map: soo-foo, prefix: 192.0.2.7/32, result: permit + +2023/02/17 21:44:27 BGP: [WYP1M-NE4SY] Converted EVPN prefix [2]:[0]:[48]:[aa:bb:cc:00:22:22]:[32]:[20.0.0.2] into 20.0.0.2/32 for optimized route-map lookup +2023/02/17 21:44:27 BGP: [MT1SJ-WEJQ1] Best match route-map: soo-foo, sequence: 10 for pfx: 20.0.0.2/32, result: match +2023/02/17 21:44:27 BGP: [H5AW4-JFYQC] Route-map: soo-foo, prefix: 20.0.0.2/32, result: permit + +2023/02/17 21:44:27 BGP: [KHG7H-RH4PN] Unable to convert EVPN prefix [3]:[0]:[32]:[2.2.2.2] into IPv4/IPv6 prefix. Falling back to non-optimized route-map lookup +2023/02/17 21:44:27 BGP: [MT1SJ-WEJQ1] Best match route-map: soo-foo, sequence: 10 for pfx: [3]:[0]:[32]:[2.2.2.2], result: match +2023/02/17 21:44:27 BGP: [H5AW4-JFYQC] Route-map: soo-foo, prefix: [3]:[0]:[32]:[2.2.2.2], result: permit +``` + +Signed-off-by: Trey Aspelund +--- + lib/routemap.c | 99 ++++++++++++++++++++++++++++---------------------- + 1 file changed, 56 insertions(+), 43 deletions(-) + +diff --git a/lib/routemap.c b/lib/routemap.c +index 210027105d..010d4bff0b 100644 +--- a/lib/routemap.c ++++ b/lib/routemap.c +@@ -1817,26 +1817,24 @@ route_map_get_index(struct route_map *map, const struct prefix *prefix, + struct route_map_index *index = NULL, *best_index = NULL; + struct route_map_index *head_index = NULL; + struct route_table *table = NULL; +- struct prefix conv; +- unsigned char family; + +- /* +- * Handling for matching evpn_routes in the prefix table. +- * +- * We convert type2/5 prefix to ipv4/6 prefix to do longest +- * prefix matching on. ++ /* Route-map optimization relies on LPM lookups of the prefix to reduce ++ * the amount of route-map clauses a given prefix needs to be processed ++ * against. These LPM trees are IPv4/IPv6-specific and prefix->family ++ * must be AF_INET or AF_INET6 in order for the lookup to succeed. So if ++ * the AF doesn't line up with the LPM trees, skip the optimization. + */ +- if (prefix->family == AF_EVPN) { +- if (evpn_prefix2prefix(prefix, &conv) != 0) +- return NULL; +- +- prefix = &conv; ++ if (map->optimization_disabled || ++ (prefix->family == AF_INET && !map->ipv4_prefix_table) || ++ (prefix->family == AF_INET6 && !map->ipv6_prefix_table)) { ++ if (rmap_debug) ++ zlog_debug( ++ "Skipping route-map optimization for route-map: %s, pfx: %pFX, family: %d", ++ map->name, prefix, prefix->family); ++ return map->head; + } + +- +- family = prefix->family; +- +- if (family == AF_INET) ++ if (prefix->family == AF_INET) + table = map->ipv4_prefix_table; + else + table = map->ipv6_prefix_table; +@@ -2558,6 +2556,7 @@ route_map_result_t route_map_apply_ext(struct route_map *map, + struct route_map_index *index = NULL; + struct route_map_rule *set = NULL; + bool skip_match_clause = false; ++ struct prefix conv; + + if (recursion > RMAP_RECURSION_LIMIT) { + flog_warn( +@@ -2575,37 +2574,51 @@ route_map_result_t route_map_apply_ext(struct route_map *map, + + map->applied++; + +- if ((!map->optimization_disabled) +- && (map->ipv4_prefix_table || map->ipv6_prefix_table)) { +- index = route_map_get_index(map, prefix, match_object, +- &match_ret); +- if (index) { +- index->applied++; +- if (rmap_debug) +- zlog_debug( +- "Best match route-map: %s, sequence: %d for pfx: %pFX, result: %s", +- map->name, index->pref, prefix, +- route_map_cmd_result_str(match_ret)); ++ /* ++ * Handling for matching evpn_routes in the prefix table. ++ * ++ * We convert type2/5 prefix to ipv4/6 prefix to do longest ++ * prefix matching on. ++ */ ++ if (prefix->family == AF_EVPN) { ++ if (evpn_prefix2prefix(prefix, &conv) != 0) { ++ zlog_debug( ++ "Unable to convert EVPN prefix %pFX into IPv4/IPv6 prefix. Falling back to non-optimized route-map lookup", ++ prefix); + } else { +- if (rmap_debug) +- zlog_debug( +- "No best match sequence for pfx: %pFX in route-map: %s, result: %s", +- prefix, map->name, +- route_map_cmd_result_str(match_ret)); +- /* +- * No index matches this prefix. Return deny unless, +- * match_ret = RMAP_NOOP. +- */ +- if (match_ret == RMAP_NOOP) +- ret = RMAP_PERMITMATCH; +- else +- ret = RMAP_DENYMATCH; +- goto route_map_apply_end; ++ zlog_debug( ++ "Converted EVPN prefix %pFX into %pFX for optimized route-map lookup", ++ prefix, &conv); ++ ++ prefix = &conv; + } +- skip_match_clause = true; ++ } ++ ++ index = route_map_get_index(map, prefix, match_object, &match_ret); ++ if (index) { ++ index->applied++; ++ if (rmap_debug) ++ zlog_debug( ++ "Best match route-map: %s, sequence: %d for pfx: %pFX, result: %s", ++ map->name, index->pref, prefix, ++ route_map_cmd_result_str(match_ret)); + } else { +- index = map->head; ++ if (rmap_debug) ++ zlog_debug( ++ "No best match sequence for pfx: %pFX in route-map: %s, result: %s", ++ prefix, map->name, ++ route_map_cmd_result_str(match_ret)); ++ /* ++ * No index matches this prefix. Return deny unless, ++ * match_ret = RMAP_NOOP. ++ */ ++ if (match_ret == RMAP_NOOP) ++ ret = RMAP_PERMITMATCH; ++ else ++ ret = RMAP_DENYMATCH; ++ goto route_map_apply_end; + } ++ skip_match_clause = true; + + for (; index; index = index->next) { + if (!skip_match_clause) { +-- +2.17.1 + + +From 950cf63054fa36be57f4aa751243f5425793584b Mon Sep 17 00:00:00 2001 +From: Donatas Abraitis +Date: Thu, 15 Feb 2024 12:07:43 +0200 +Subject: [PATCH 2/2] lib: Do not convert EVPN prefixes into IPv4/IPv6 if not + needed + +Convert only when this is really needed, e.g. `match ip address prefix-list ...`. + +Otherwise, we can't have mixed match clauses, like: + +``` +match ip address prefix-list p1 +match evpn route-type prefix +``` + +This won't work, because the prefix is already converted, and we can't extract +route type, vni, etc. from the original EVPN prefix. + +Signed-off-by: Donatas Abraitis +(cherry picked from commit 439b739495e86912c8b9ec36b84e55311c549ba0) +--- + lib/routemap.c | 25 +++++-------------------- + 1 file changed, 5 insertions(+), 20 deletions(-) + +diff --git a/lib/routemap.c b/lib/routemap.c +index 010d4bff0b..408faae49e 100644 +--- a/lib/routemap.c ++++ b/lib/routemap.c +@@ -2556,7 +2556,6 @@ route_map_result_t route_map_apply_ext(struct route_map *map, + struct route_map_index *index = NULL; + struct route_map_rule *set = NULL; + bool skip_match_clause = false; +- struct prefix conv; + + if (recursion > RMAP_RECURSION_LIMIT) { + flog_warn( +@@ -2574,27 +2573,14 @@ route_map_result_t route_map_apply_ext(struct route_map *map, + + map->applied++; + +- /* +- * Handling for matching evpn_routes in the prefix table. +- * +- * We convert type2/5 prefix to ipv4/6 prefix to do longest +- * prefix matching on. +- */ + if (prefix->family == AF_EVPN) { +- if (evpn_prefix2prefix(prefix, &conv) != 0) { +- zlog_debug( +- "Unable to convert EVPN prefix %pFX into IPv4/IPv6 prefix. Falling back to non-optimized route-map lookup", +- prefix); +- } else { +- zlog_debug( +- "Converted EVPN prefix %pFX into %pFX for optimized route-map lookup", +- prefix, &conv); +- +- prefix = &conv; +- } ++ index = map->head; ++ } else { ++ skip_match_clause = true; ++ index = route_map_get_index(map, prefix, match_object, ++ &match_ret); + } + +- index = route_map_get_index(map, prefix, match_object, &match_ret); + if (index) { + index->applied++; + if (rmap_debug) +@@ -2618,7 +2604,6 @@ route_map_result_t route_map_apply_ext(struct route_map *map, + ret = RMAP_DENYMATCH; + goto route_map_apply_end; + } +- skip_match_clause = true; + + for (; index; index = index->next) { + if (!skip_match_clause) { +-- +2.17.1 + diff --git a/src/sonic-frr/patch/series b/src/sonic-frr/patch/series index fcc15bac8ffc..e0a16cd6eba2 100644 --- a/src/sonic-frr/patch/series +++ b/src/sonic-frr/patch/series @@ -34,3 +34,4 @@ cross-compile-changes.patch 0035-fpm-ignore-route-from-default-table.patch 0036-Add-support-of-bgp-l3vni-evpn.patch 0037-bgp-community-memory-leak-fix.patch +0038-lib-Do-not-convert-EVPN-prefixes-into-IPv4-IPv6-if-n.patch