From e6b00e3fb9d40e33e4d756e5f5ef35b4b4cd07b4 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 7 Jan 2021 15:28:28 -0500 Subject: [PATCH 1/5] pbrd: nht only handle if updates if IPV*_IFINDEX nh Only handle an interface update in the nexthop tracking code if the nexthop in question was set with an interface to point out of. If the nexthop is GW only, the interface update could be unrelated but have overlapping address space. Let that be handled elsewhere. Ex) ``` 5.5.5.0/30 dev dummyDoof proto kernel scope link src 5.5.5.1 5.5.5.0/24 dev goofDummy proto kernel scope link src 5.5.5.1 [root@alfred frr-2]# ip ro show table 10000 default via 5.5.5.2 dev dummyDoof proto pbr metric 20 [root@alfred frr-2]# ip link set goofDummy down [root@alfred frr-2]# ip ro show table 10000 [root@alfred frr-2]# ip link set goofDummy up [root@alfred frr-2]# ip ro show table 10000 ``` Signed-off-by: Stephen Worley --- pbrd/pbr_nht.c | 37 +++++++------------------------------ 1 file changed, 7 insertions(+), 30 deletions(-) diff --git a/pbrd/pbr_nht.c b/pbrd/pbr_nht.c index 723374d9d6..5b79a3d211 100644 --- a/pbrd/pbr_nht.c +++ b/pbrd/pbr_nht.c @@ -711,7 +711,6 @@ pbr_nht_individual_nexthop_gw_update(struct pbr_nexthop_cache *pnhc, struct pbr_nht_individual *pnhi) { bool is_valid = pnhc->valid; - bool all_done = false; /* * If we have an interface down event, let's note that @@ -723,43 +722,21 @@ pbr_nht_individual_nexthop_gw_update(struct pbr_nexthop_cache *pnhc, * interface event. */ if (!pnhi->nhr && pnhi->ifp) { - struct connected *connected; - struct listnode *node; - struct prefix p; - switch (pnhc->nexthop.type) { case NEXTHOP_TYPE_BLACKHOLE: - all_done = true; + case NEXTHOP_TYPE_IPV4: + case NEXTHOP_TYPE_IPV6: + goto done; break; case NEXTHOP_TYPE_IFINDEX: case NEXTHOP_TYPE_IPV4_IFINDEX: case NEXTHOP_TYPE_IPV6_IFINDEX: - is_valid = if_is_up(pnhi->ifp); - all_done = true; - break; - case NEXTHOP_TYPE_IPV4: - p.family = AF_INET; - p.prefixlen = IPV4_MAX_BITLEN; - p.u.prefix4 = pnhc->nexthop.gate.ipv4; - break; - case NEXTHOP_TYPE_IPV6: - p.family = AF_INET6; - p.prefixlen = IPV6_MAX_BITLEN; - memcpy(&p.u.prefix6, &pnhc->nexthop.gate.ipv6, - sizeof(struct in6_addr)); - break; - } - - /* Early exit in a couple of cases. */ - if (all_done) - goto done; - - FOR_ALL_INTERFACES_ADDRESSES (pnhi->ifp, connected, node) { - if (prefix_match(connected->address, &p)) { + if (pnhc->nexthop.ifindex == pnhi->ifp->ifindex) is_valid = if_is_up(pnhi->ifp); - break; - } + goto done; + break; } + goto done; } From 8eeca5a201fce3930d1ede9b7385bf6325f255ce Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 11 Jan 2021 17:28:39 -0500 Subject: [PATCH 2/5] zebra: add some debugging for PBR events in zebra Add some debugging for PBR events internal to zebra, specifically ADD/UPDATE/DELETE of pbr rules. Signed-off-by: Stephen Worley --- zebra/debug.c | 38 ++++++++++++++++++++++++++++++++++++++ zebra/debug.h | 5 +++++ zebra/zebra_pbr.c | 21 ++++++++++++++++++++- 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/zebra/debug.c b/zebra/debug.c index 87a10ea65d..21fa765c63 100644 --- a/zebra/debug.c +++ b/zebra/debug.c @@ -41,6 +41,7 @@ unsigned long zebra_debug_dplane; unsigned long zebra_debug_mlag; unsigned long zebra_debug_nexthop; unsigned long zebra_debug_evpn_mh; +unsigned long zebra_debug_pbr; DEFINE_HOOK(zebra_debug_show_debugging, (struct vty *vty), (vty)); @@ -122,6 +123,9 @@ DEFUN_NOSH (show_debugging_zebra, if (IS_ZEBRA_DEBUG_EVPN_MH_NEIGH) vty_out(vty, " Zebra EVPN-MH Neigh debugging is on\n"); + if (IS_ZEBRA_DEBUG_PBR) + vty_out(vty, " Zebra PBR debugging is on\n"); + hook_call(zebra_debug_show_debugging, vty); return CMD_SUCCESS; } @@ -318,6 +322,17 @@ DEFUN (debug_zebra_dplane, return CMD_SUCCESS; } +DEFUN (debug_zebra_pbr, + debug_zebra_pbr_cmd, + "debug zebra pbr", + DEBUG_STR + "Zebra configuration\n" + "Debug zebra pbr events\n") +{ + SET_FLAG(zebra_debug_pbr, ZEBRA_DEBUG_PBR); + return CMD_SUCCESS; +} + DEFPY (debug_zebra_mlag, debug_zebra_mlag_cmd, "[no$no] debug zebra mlag", @@ -508,6 +523,18 @@ DEFUN (no_debug_zebra_dplane, return CMD_SUCCESS; } +DEFUN (no_debug_zebra_pbr, + no_debug_zebra_pbr_cmd, + "no debug zebra pbr", + NO_STR + DEBUG_STR + "Zebra configuration\n" + "Debug zebra pbr events\n") +{ + zebra_debug_pbr = 0; + return CMD_SUCCESS; +} + DEFPY (debug_zebra_nexthop, debug_zebra_nexthop_cmd, "[no$no] debug zebra nexthop [detail$detail]", @@ -650,6 +677,11 @@ static int config_write_debug(struct vty *vty) write++; } + if (IS_ZEBRA_DEBUG_PBR) { + vty_out(vty, "debug zebra pbr\n"); + write++; + } + return write; } @@ -668,6 +700,7 @@ void zebra_debug_init(void) zebra_debug_evpn_mh = 0; zebra_debug_nht = 0; zebra_debug_nexthop = 0; + zebra_debug_pbr = 0; install_node(&debug_node); @@ -686,6 +719,7 @@ void zebra_debug_init(void) install_element(ENABLE_NODE, &debug_zebra_dplane_cmd); install_element(ENABLE_NODE, &debug_zebra_mlag_cmd); install_element(ENABLE_NODE, &debug_zebra_nexthop_cmd); + install_element(ENABLE_NODE, &debug_zebra_pbr_cmd); install_element(ENABLE_NODE, &no_debug_zebra_events_cmd); install_element(ENABLE_NODE, &no_debug_zebra_nht_cmd); install_element(ENABLE_NODE, &no_debug_zebra_mpls_cmd); @@ -696,6 +730,7 @@ void zebra_debug_init(void) install_element(ENABLE_NODE, &no_debug_zebra_rib_cmd); install_element(ENABLE_NODE, &no_debug_zebra_fpm_cmd); install_element(ENABLE_NODE, &no_debug_zebra_dplane_cmd); + install_element(ENABLE_NODE, &no_debug_zebra_pbr_cmd); install_element(ENABLE_NODE, &debug_zebra_evpn_mh_cmd); install_element(CONFIG_NODE, &debug_zebra_events_cmd); @@ -710,6 +745,8 @@ void zebra_debug_init(void) install_element(CONFIG_NODE, &debug_zebra_fpm_cmd); install_element(CONFIG_NODE, &debug_zebra_dplane_cmd); install_element(CONFIG_NODE, &debug_zebra_nexthop_cmd); + install_element(CONFIG_NODE, &debug_zebra_pbr_cmd); + install_element(CONFIG_NODE, &no_debug_zebra_events_cmd); install_element(CONFIG_NODE, &no_debug_zebra_nht_cmd); install_element(CONFIG_NODE, &no_debug_zebra_mpls_cmd); @@ -720,6 +757,7 @@ void zebra_debug_init(void) install_element(CONFIG_NODE, &no_debug_zebra_rib_cmd); install_element(CONFIG_NODE, &no_debug_zebra_fpm_cmd); install_element(CONFIG_NODE, &no_debug_zebra_dplane_cmd); + install_element(CONFIG_NODE, &no_debug_zebra_pbr_cmd); install_element(CONFIG_NODE, &debug_zebra_mlag_cmd); install_element(CONFIG_NODE, &debug_zebra_evpn_mh_cmd); } diff --git a/zebra/debug.h b/zebra/debug.h index 8402224f19..86506846ad 100644 --- a/zebra/debug.h +++ b/zebra/debug.h @@ -67,6 +67,8 @@ extern "C" { #define ZEBRA_DEBUG_EVPN_MH_MAC 0x04 #define ZEBRA_DEBUG_EVPN_MH_NEIGH 0x08 +#define ZEBRA_DEBUG_PBR 0x01 + /* Debug related macro. */ #define IS_ZEBRA_DEBUG_EVENT (zebra_debug_event & ZEBRA_DEBUG_EVENT) @@ -114,6 +116,8 @@ extern "C" { #define IS_ZEBRA_DEBUG_EVPN_MH_NEIGH \ (zebra_debug_evpn_mh & ZEBRA_DEBUG_EVPN_MH_NEIGH) +#define IS_ZEBRA_DEBUG_PBR (zebra_debug_pbr & ZEBRA_DEBUG_PBR) + extern unsigned long zebra_debug_event; extern unsigned long zebra_debug_packet; extern unsigned long zebra_debug_kernel; @@ -127,6 +131,7 @@ extern unsigned long zebra_debug_dplane; extern unsigned long zebra_debug_mlag; extern unsigned long zebra_debug_nexthop; extern unsigned long zebra_debug_evpn_mh; +extern unsigned long zebra_debug_pbr; extern void zebra_debug_init(void); diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c index c244d2a955..924aed023d 100644 --- a/zebra/zebra_pbr.c +++ b/zebra/zebra_pbr.c @@ -32,6 +32,7 @@ #include "zebra/zapi_msg.h" #include "zebra/zebra_memory.h" #include "zebra/zserv.h" +#include "zebra/debug.h" /* definitions */ DEFINE_MTYPE_STATIC(ZEBRA, PBR_IPTABLE_IFNAME, "PBR interface list") @@ -503,6 +504,12 @@ void zebra_pbr_add_rule(struct zebra_pbr_rule *rule) /* If found, this is an update */ if (found) { + if (IS_ZEBRA_DEBUG_PBR) + zlog_debug( + "%s: seq: %d, prior: %d, unique: %d, ifname: %s -- update", + __func__, rule->rule.seq, rule->rule.priority, + rule->rule.unique, rule->rule.ifname); + (void)dplane_pbr_rule_update(found, rule); if (pbr_rule_release(found)) @@ -510,12 +517,24 @@ void zebra_pbr_add_rule(struct zebra_pbr_rule *rule) "%s: Rule being updated we know nothing about", __PRETTY_FUNCTION__); - } else + } else { + if (IS_ZEBRA_DEBUG_PBR) + zlog_debug( + "%s: seq: %d, prior: %d, unique: %d, ifname: %s -- new", + __func__, rule->rule.seq, rule->rule.priority, + rule->rule.unique, rule->rule.ifname); + (void)dplane_pbr_rule_add(rule); + } } void zebra_pbr_del_rule(struct zebra_pbr_rule *rule) { + if (IS_ZEBRA_DEBUG_PBR) + zlog_debug("%s: seq: %d, prior: %d, unique: %d, ifname: %s", + __func__, rule->rule.seq, rule->rule.priority, + rule->rule.unique, rule->rule.ifname); + (void)dplane_pbr_rule_delete(rule); if (pbr_rule_release(rule)) From f7692085cb040e1900986e9eb02d30a196be1e98 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 11 Jan 2021 17:30:21 -0500 Subject: [PATCH 3/5] zebra: move pbr hash create after update release Move the pbr hash creation to be after the update release and dplane install. Now that rules are installed in a separate dplane pthread, we can have scenarios where we have an interface flapping and we install/remove rules sufficiently fast enough we could issue what we think is an update for an identical rule and end up releasing the rule right after we created it and sent it to the dplane. This solves the problem of recving duplicate rules during interface flapping. Signed-off-by: Stephen Worley --- zebra/zebra_pbr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c index 924aed023d..87ab900092 100644 --- a/zebra/zebra_pbr.c +++ b/zebra/zebra_pbr.c @@ -500,8 +500,6 @@ void zebra_pbr_add_rule(struct zebra_pbr_rule *rule) */ found = pbr_rule_lookup_unique(rule); - (void)hash_get(zrouter.rules_hash, rule, pbr_rule_alloc_intern); - /* If found, this is an update */ if (found) { if (IS_ZEBRA_DEBUG_PBR) @@ -526,6 +524,8 @@ void zebra_pbr_add_rule(struct zebra_pbr_rule *rule) (void)dplane_pbr_rule_add(rule); } + + (void)hash_get(zrouter.rules_hash, rule, pbr_rule_alloc_intern); } void zebra_pbr_del_rule(struct zebra_pbr_rule *rule) From e50f113bbfdb147ebff18d18381f778212f7de25 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 28 Jan 2021 13:53:34 -0500 Subject: [PATCH 4/5] doc: add pbr debug command docs Add some docs for debug pbr commands. Signed-off-by: Stephen Worley --- doc/user/pbr.rst | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/doc/user/pbr.rst b/doc/user/pbr.rst index c869c6bc45..5cec7cbe62 100644 --- a/doc/user/pbr.rst +++ b/doc/user/pbr.rst @@ -258,6 +258,21 @@ causes the policy to be installed into the kernel. | valid | Is the map well-formed? | Boolean | +--------+----------------------------+---------+ +.. _pbr-debugs: + +PBR Debugs +=========== + +.. index:: debug pbr +.. clicmd:: debug pbr events|map|nht|zebra + + Debug pbr in pbrd daemon. You specify what types of debugs to turn on. + +.. index:: debug zebra pbr +.. clicmd:: debug zebra pbr + + Debug pbr in zebra daemon. + .. _pbr-details: PBR Details From b74a0a33f3734a772b315323ce5265fce4715c58 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Fri, 29 Jan 2021 12:37:17 -0500 Subject: [PATCH 5/5] pbrd: remove extraneous break Remove extraneous break. Not needed after goto. Signed-off-by: Stephen Worley --- pbrd/pbr_nht.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/pbrd/pbr_nht.c b/pbrd/pbr_nht.c index 5b79a3d211..7a814bd724 100644 --- a/pbrd/pbr_nht.c +++ b/pbrd/pbr_nht.c @@ -727,14 +727,12 @@ pbr_nht_individual_nexthop_gw_update(struct pbr_nexthop_cache *pnhc, case NEXTHOP_TYPE_IPV4: case NEXTHOP_TYPE_IPV6: goto done; - break; case NEXTHOP_TYPE_IFINDEX: case NEXTHOP_TYPE_IPV4_IFINDEX: case NEXTHOP_TYPE_IPV6_IFINDEX: if (pnhc->nexthop.ifindex == pnhi->ifp->ifindex) is_valid = if_is_up(pnhi->ifp); goto done; - break; } goto done;