From 3ae327cb966d8e982a9d94fd47fad636444e8a31 Mon Sep 17 00:00:00 2001
From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Thu, 19 Dec 2019 17:11:26 -0500
Subject: [PATCH 1/9] zebra: define some explicit rule replace code paths

Define some explicit rule replace code paths into the dataplane
code and improve the handling around it/releasing the the old
rule from the hash table.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---
 zebra/rule_netlink.c | 25 +++++++++++++++++++
 zebra/rule_socket.c  |  8 ++++++
 zebra/zebra_pbr.c    | 59 +++++++++++++++++++++++++++++---------------
 zebra/zebra_pbr.h    |  7 ++++++
 4 files changed, 79 insertions(+), 20 deletions(-)

diff --git a/zebra/rule_netlink.c b/zebra/rule_netlink.c
index c9699c7d95..34df2485c0 100644
--- a/zebra/rule_netlink.c
+++ b/zebra/rule_netlink.c
@@ -173,6 +173,31 @@ enum zebra_dplane_result kernel_del_pbr_rule(struct zebra_pbr_rule *rule)
 	return ZEBRA_DPLANE_REQUEST_SUCCESS;
 }
 
+/*
+ * Update specified rule for a specific interface.
+ */
+enum zebra_dplane_result kernel_update_pbr_rule(struct zebra_pbr_rule *old_rule,
+						struct zebra_pbr_rule *new_rule)
+{
+	int ret = 0;
+
+	/* Add the new, updated one */
+	ret = netlink_rule_update(RTM_NEWRULE, new_rule);
+
+	/**
+	 * Delete the old one.
+	 *
+	 * Don't care about this result right?
+	 */
+	netlink_rule_update(RTM_DELRULE, old_rule);
+
+	kernel_pbr_rule_add_del_status(new_rule,
+				       (!ret) ? ZEBRA_DPLANE_INSTALL_SUCCESS
+					      : ZEBRA_DPLANE_INSTALL_FAILURE);
+
+	return ZEBRA_DPLANE_REQUEST_SUCCESS;
+}
+
 /*
  * Handle netlink notification informing a rule add or delete.
  * Handling of an ADD is TBD.
diff --git a/zebra/rule_socket.c b/zebra/rule_socket.c
index e2c650b4ad..219fa7de6f 100644
--- a/zebra/rule_socket.c
+++ b/zebra/rule_socket.c
@@ -57,4 +57,12 @@ enum zebra_dplane_result kernel_del_pbr_rule(struct zebra_pbr_rule *rule)
 	return ZEBRA_DPLANE_REQUEST_FAILURE;
 }
 
+enum zebra_dplane_result kernel_update_pbr_rule(struct zebra_pbr_rule *old_rule,
+						struct zebra_pbr_rule *new_rule)
+{
+	flog_err(EC_LIB_UNAVAILABLE, "%s not Implemented for this platform",
+		 __PRETTY_FUNCTION__);
+	return ZEBRA_DPLANE_REQUEST_FAILURE;
+}
+
 #endif
diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c
index 6728567e6e..c049aa14f6 100644
--- a/zebra/zebra_pbr.c
+++ b/zebra/zebra_pbr.c
@@ -431,32 +431,51 @@ static void *pbr_rule_alloc_intern(void *arg)
 	return new;
 }
 
-void zebra_pbr_add_rule(struct zebra_pbr_rule *rule)
-{
-	struct zebra_pbr_rule *unique =
-		pbr_rule_lookup_unique(rule);
-
-	(void)hash_get(zrouter.rules_hash, rule, pbr_rule_alloc_intern);
-	(void)kernel_add_pbr_rule(rule);
-	/*
-	 * Rule Replace semantics, if we have an old, install the
-	 * new rule, look above, and then delete the old
-	 */
-	if (unique)
-		zebra_pbr_del_rule(unique);
-}
-
-void zebra_pbr_del_rule(struct zebra_pbr_rule *rule)
+static int pbr_rule_release(struct zebra_pbr_rule *rule)
 {
 	struct zebra_pbr_rule *lookup;
 
 	lookup = hash_lookup(zrouter.rules_hash, rule);
+
+	if (!lookup)
+		return -ENOENT;
+
+	hash_release(zrouter.rules_hash, lookup);
+	XFREE(MTYPE_TMP, lookup);
+
+	return 0;
+}
+
+void zebra_pbr_add_rule(struct zebra_pbr_rule *rule)
+{
+	struct zebra_pbr_rule *found;
+
+	/**
+	 * Check if we already have it (this checks via a unique ID, walking
+	 * over the hash table, not via a hash operation).
+	 */
+	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) {
+		(void)kernel_update_pbr_rule(found, rule);
+
+		if (pbr_rule_release(found))
+			zlog_debug(
+				"%s: Rule being updated we know nothing about",
+				__PRETTY_FUNCTION__);
+
+	} else
+		(void)kernel_add_pbr_rule(rule);
+}
+
+void zebra_pbr_del_rule(struct zebra_pbr_rule *rule)
+{
 	(void)kernel_del_pbr_rule(rule);
 
-	if (lookup) {
-		hash_release(zrouter.rules_hash, lookup);
-		XFREE(MTYPE_TMP, lookup);
-	} else
+	if (pbr_rule_release(rule))
 		zlog_debug("%s: Rule being deleted we know nothing about",
 			   __func__);
 }
diff --git a/zebra/zebra_pbr.h b/zebra/zebra_pbr.h
index b7fbc9b7d5..83797b9521 100644
--- a/zebra/zebra_pbr.h
+++ b/zebra/zebra_pbr.h
@@ -182,6 +182,13 @@ extern enum zebra_dplane_result kernel_add_pbr_rule(struct zebra_pbr_rule *rule)
  */
 extern enum zebra_dplane_result kernel_del_pbr_rule(struct zebra_pbr_rule *rule);
 
+/*
+ * Update specified rule for a specific interface.
+ */
+extern enum zebra_dplane_result
+kernel_update_pbr_rule(struct zebra_pbr_rule *old_rule,
+		       struct zebra_pbr_rule *new_rule);
+
 /*
  * Get to know existing PBR rules in the kernel - typically called at startup.
  */

From b21d304247554b770acfcfd15b82148fd109daf0 Mon Sep 17 00:00:00 2001
From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Thu, 2 Apr 2020 13:31:24 -0400
Subject: [PATCH 2/9] pbrd: add seqno to debug in pbr_send_pbr_map

Add some more debug info for the sequence number we are
sending to zebra in pbr_send_pbr_map().

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---
 pbrd/pbr_zebra.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pbrd/pbr_zebra.c b/pbrd/pbr_zebra.c
index c2d93a405e..cb28015c18 100644
--- a/pbrd/pbr_zebra.c
+++ b/pbrd/pbr_zebra.c
@@ -582,9 +582,9 @@ void pbr_send_pbr_map(struct pbr_map_sequence *pbrms,
 	 */
 	stream_putl(s, 1);
 
-	DEBUGD(&pbr_dbg_zebra, "%s: \t%s %s %d %s %u", __func__,
-	       install ? "Installing" : "Deleting", pbrm->name, install,
-	       pmi->ifp->name, pmi->delete);
+	DEBUGD(&pbr_dbg_zebra, "%s: \t%s %s seq %u %d %s %u", __func__,
+	       install ? "Installing" : "Deleting", pbrm->name, pbrms->seqno,
+	       install, pmi->ifp->name, pmi->delete);
 
 	pbr_encode_pbr_map_sequence(s, pbrms, pmi->ifp);
 

From 1f375577f6965d64fe9c3ed5f210e87f51a00149 Mon Sep 17 00:00:00 2001
From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Mon, 6 Apr 2020 12:51:55 -0400
Subject: [PATCH 3/9] pbrd: delete pbr nhg cache after rlease from hash

Actually delete the allocated pbr_nhg_cache object we just
released.

Found via memory leak:

==3078405== 136 bytes in 1 blocks are definitely lost in loss record 8,282 of 8,802
==3078405==    at 0x483BB1A: calloc (vg_replace_malloc.c:762)
==3078405==    by 0x48E35E8: qcalloc (memory.c:110)
==3078405==    by 0x40EBA7: pbr_nhgc_alloc (pbr_nht.c:194)
==3078405==    by 0x48CC0EB: hash_get (hash.c:148)
==3078405==    by 0x40F825: pbr_nht_add_individual_nexthop (pbr_nht.c:534)
==3078405==    by 0x409853: pbr_map_nexthop_magic (pbr_vty.c:400)
==3078405==    by 0x4093F1: pbr_map_nexthop (pbr_vty_clippy.c:417)
==3078405==    by 0x48ACF72: cmd_execute_command_real (command.c:1073)
==3078405==    by 0x48ACB3B: cmd_execute_command (command.c:1133)
==3078405==    by 0x48AD063: cmd_execute (command.c:1288)
==3078405==    by 0x493D8EE: vty_command (vty.c:526)
==3078405==    by 0x493D397: vty_execute (vty.c:1293)

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---
 pbrd/pbr_nht.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pbrd/pbr_nht.c b/pbrd/pbr_nht.c
index ecd375333c..8a0168924d 100644
--- a/pbrd/pbr_nht.c
+++ b/pbrd/pbr_nht.c
@@ -564,6 +564,7 @@ void pbr_nht_delete_individual_nexthop(struct pbr_map_sequence *pbrms)
 	pbr_nht_uninstall_nexthop_group(pnhgc, *pbrms->nhg, nh_type);
 
 	hash_release(pbr_nhg_hash, pnhgc);
+	pbr_nhgc_delete(pnhgc);
 
 	nexthop_group_delete(&pbrms->nhg);
 	XFREE(MTYPE_TMP, pbrms->internal_nhg_name);

From 10fcd2a2a65496ef32c0c68e632f47b4233d1247 Mon Sep 17 00:00:00 2001
From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Mon, 6 Apr 2020 14:13:20 -0400
Subject: [PATCH 4/9] pbrd: free nexthop_group name on `no set nexthop-group`

Properly free the string pointed to by `pbrms->nhgrp_name`
when we are removiing the config for a nexthop group
on a pbr map sequence.

Found via memleak:
==3152214== 4 bytes in 1 blocks are definitely lost in loss record 308 of 8,814
==3152214==    at 0x483980B: malloc (vg_replace_malloc.c:309)
==3152214==    by 0x4DC9F7E: strdup (in /usr/lib64/libc-2.30.so)
==3152214==    by 0x48E373E: qstrdup (memory.c:122)
==3152214==    by 0x408FE7: pbr_map_nexthop_group_magic (pbr_vty.c:264)
==3152214==    by 0x408E04: pbr_map_nexthop_group (pbr_vty_clippy.c:347)
==3152214==    by 0x48ACF72: cmd_execute_command_real (command.c:1073)
==3152214==    by 0x48ACB3B: cmd_execute_command (command.c:1133)
==3152214==    by 0x48AD063: cmd_execute (command.c:1288)
==3152214==    by 0x493D8EE: vty_command (vty.c:526)
==3152214==    by 0x493D397: vty_execute (vty.c:1293)
==3152214==    by 0x493C4EC: vtysh_read (vty.c:2126)
==3152214==    by 0x49319DC: thread_call (thread.c:1548)

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---
 pbrd/pbr_map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pbrd/pbr_map.c b/pbrd/pbr_map.c
index e45e629649..95f97af945 100644
--- a/pbrd/pbr_map.c
+++ b/pbrd/pbr_map.c
@@ -384,7 +384,7 @@ static void pbr_map_delete_common(struct pbr_map_sequence *pbrms)
 	pbrm->valid = false;
 	pbrms->nhs_installed = false;
 	pbrms->reason |= PBR_MAP_INVALID_NO_NEXTHOPS;
-	pbrms->nhgrp_name = NULL;
+	XFREE(MTYPE_TMP, pbrms->nhgrp_name);
 }
 
 void pbr_map_delete_nexthops(struct pbr_map_sequence *pbrms)

From f143cffac2a3cfef96b9d6d796673ecc7c545f8f Mon Sep 17 00:00:00 2001
From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Wed, 18 Dec 2019 16:11:39 -0500
Subject: [PATCH 5/9] pbrd: implement `set *` and `match *` config replacement

Implement the ability to replace any existing `set *` or
`match` with another one or adding more config without having
to first delete the original config already there.

Before, we needed to constantly execute a `no` command for everything
to remove the rule before making changes to it. With this
patch, you can replace configs on individual sequences much
easier.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---
 pbrd/pbr_map.c   |  33 ++++----
 pbrd/pbr_map.h   |  10 ++-
 pbrd/pbr_nht.c   |  27 +++++-
 pbrd/pbr_nht.h   |   3 +-
 pbrd/pbr_vty.c   | 214 ++++++++++++++++-------------------------------
 pbrd/pbr_zebra.c |  10 ++-
 pbrd/pbr_zebra.h |   3 +-
 7 files changed, 131 insertions(+), 169 deletions(-)

diff --git a/pbrd/pbr_map.c b/pbrd/pbr_map.c
index 95f97af945..7928b8e2e7 100644
--- a/pbrd/pbr_map.c
+++ b/pbrd/pbr_map.c
@@ -145,7 +145,7 @@ static bool pbr_map_interface_is_valid(const struct pbr_map_interface *pmi)
 }
 
 static void pbr_map_pbrms_update_common(struct pbr_map_sequence *pbrms,
-					bool install)
+					bool install, bool changed)
 {
 	struct pbr_map *pbrm;
 	struct listnode *node;
@@ -161,19 +161,19 @@ static void pbr_map_pbrms_update_common(struct pbr_map_sequence *pbrms,
 			if (install && !pbr_map_interface_is_valid(pmi))
 				continue;
 
-			pbr_send_pbr_map(pbrms, pmi, install);
+			pbr_send_pbr_map(pbrms, pmi, install, changed);
 		}
 	}
 }
 
-static void pbr_map_pbrms_install(struct pbr_map_sequence *pbrms)
+static void pbr_map_pbrms_install(struct pbr_map_sequence *pbrms, bool changed)
 {
-	pbr_map_pbrms_update_common(pbrms, true);
+	pbr_map_pbrms_update_common(pbrms, true, changed);
 }
 
 static void pbr_map_pbrms_uninstall(struct pbr_map_sequence *pbrms)
 {
-	pbr_map_pbrms_update_common(pbrms, false);
+	pbr_map_pbrms_update_common(pbrms, false, false);
 }
 
 static const char *const pbr_map_reason_str[] = {
@@ -292,7 +292,7 @@ void pbr_map_policy_interface_update(const struct interface *ifp, bool state_up)
 	for (ALL_LIST_ELEMENTS_RO(pbrm->seqnumbers, node, pbrms))
 		for (ALL_LIST_ELEMENTS_RO(pbrm->incoming, inode, pmi))
 			if (pmi->ifp == ifp && pbr_map_interface_is_valid(pmi))
-				pbr_send_pbr_map(pbrms, pmi, state_up);
+				pbr_send_pbr_map(pbrms, pmi, state_up, false);
 }
 
 static void pbrms_vrf_update(struct pbr_map_sequence *pbrms,
@@ -306,7 +306,7 @@ static void pbrms_vrf_update(struct pbr_map_sequence *pbrms,
 		DEBUGD(&pbr_dbg_map, "\tSeq %u uses vrf %s (%u), updating map",
 		       pbrms->seqno, vrf_name, pbr_vrf_id(pbr_vrf));
 
-		pbr_map_check(pbrms);
+		pbr_map_check(pbrms, false);
 	}
 }
 
@@ -360,7 +360,7 @@ extern void pbr_map_delete(struct pbr_map_sequence *pbrms)
 	pbrm = pbrms->parent;
 
 	for (ALL_LIST_ELEMENTS_RO(pbrm->incoming, inode, pmi))
-		pbr_send_pbr_map(pbrms, pmi, false);
+		pbr_send_pbr_map(pbrms, pmi, false, false);
 
 	if (pbrms->nhg)
 		pbr_nht_delete_individual_nexthop(pbrms);
@@ -619,7 +619,7 @@ void pbr_map_schedule_policy_from_nhg(const char *nh_group)
 			    && (strcmp(nh_group, pbrms->nhgrp_name) == 0)) {
 				pbrms->nhs_installed = true;
 
-				pbr_map_check(pbrms);
+				pbr_map_check(pbrms, false);
 			}
 
 			if (pbrms->nhg
@@ -627,7 +627,7 @@ void pbr_map_schedule_policy_from_nhg(const char *nh_group)
 				== 0)) {
 				pbrms->nhs_installed = true;
 
-				pbr_map_check(pbrms);
+				pbr_map_check(pbrms, false);
 			}
 		}
 	}
@@ -656,7 +656,8 @@ void pbr_map_policy_install(const char *name)
 			       pbrms->seqno);
 			for (ALL_LIST_ELEMENTS_RO(pbrm->incoming, inode, pmi))
 				if (pbr_map_interface_is_valid(pmi))
-					pbr_send_pbr_map(pbrms, pmi, true);
+					pbr_send_pbr_map(pbrms, pmi, true,
+							 false);
 		}
 	}
 }
@@ -668,7 +669,7 @@ void pbr_map_policy_delete(struct pbr_map *pbrm, struct pbr_map_interface *pmi)
 
 
 	for (ALL_LIST_ELEMENTS_RO(pbrm->seqnumbers, node, pbrms))
-		pbr_send_pbr_map(pbrms, pmi, false);
+		pbr_send_pbr_map(pbrms, pmi, false, false);
 
 	pmi->delete = true;
 }
@@ -710,13 +711,13 @@ void pbr_map_check_nh_group_change(const char *nh_group)
 						     pbrm->incoming, inode,
 						     pmi))
 						pbr_send_pbr_map(pbrms, pmi,
-								 false);
+								 false, false);
 			}
 		}
 	}
 }
 
-void pbr_map_check(struct pbr_map_sequence *pbrms)
+void pbr_map_check(struct pbr_map_sequence *pbrms, bool changed)
 {
 	struct pbr_map *pbrm;
 	bool install;
@@ -741,7 +742,7 @@ void pbr_map_check(struct pbr_map_sequence *pbrms)
 	}
 
 	if (install)
-		pbr_map_pbrms_install(pbrms);
+		pbr_map_pbrms_install(pbrms, changed);
 	else
 		pbr_map_pbrms_uninstall(pbrms);
 }
@@ -755,7 +756,7 @@ void pbr_map_install(struct pbr_map *pbrm)
 		return;
 
 	for (ALL_LIST_ELEMENTS_RO(pbrm->seqnumbers, node, pbrms))
-		pbr_map_pbrms_install(pbrms);
+		pbr_map_pbrms_install(pbrms, false);
 }
 
 void pbr_map_init(void)
diff --git a/pbrd/pbr_map.h b/pbrd/pbr_map.h
index 8bd22cbf2a..41f1703954 100644
--- a/pbrd/pbr_map.h
+++ b/pbrd/pbr_map.h
@@ -182,7 +182,15 @@ extern void pbr_map_init(void);
 
 extern bool pbr_map_check_valid(const char *name);
 
-extern void pbr_map_check(struct pbr_map_sequence *pbrms);
+/**
+ * Re-check the pbr map for validity.
+ *
+ * Install if valid, remove if not.
+ *
+ * If changed is set, the config on the on the map has changed somewhere
+ * and the rules need to be replaced if valid.
+ */
+extern void pbr_map_check(struct pbr_map_sequence *pbrms, bool changed);
 extern void pbr_map_check_nh_group_change(const char *nh_group);
 extern void pbr_map_reason_string(unsigned int reason, char *buf, int size);
 
diff --git a/pbrd/pbr_nht.c b/pbrd/pbr_nht.c
index 8a0168924d..2f3591ac8d 100644
--- a/pbrd/pbr_nht.c
+++ b/pbrd/pbr_nht.c
@@ -510,12 +510,26 @@ char *pbr_nht_nexthop_make_name(char *name, size_t l,
 	return buffer;
 }
 
-void pbr_nht_add_individual_nexthop(struct pbr_map_sequence *pbrms)
+void pbr_nht_add_individual_nexthop(struct pbr_map_sequence *pbrms,
+				    const struct nexthop *nhop)
 {
 	struct pbr_nexthop_group_cache *pnhgc;
 	struct pbr_nexthop_group_cache find;
 	struct pbr_nexthop_cache *pnhc;
 	struct pbr_nexthop_cache lookup;
+	struct nexthop *nh;
+	char buf[PBR_NHC_NAMELEN];
+
+	pbrms->nhg = nexthop_group_new();
+	pbrms->internal_nhg_name = XSTRDUP(
+		MTYPE_TMP,
+		pbr_nht_nexthop_make_name(pbrms->parent->name, PBR_NHC_NAMELEN,
+					  pbrms->seqno, buf));
+
+	nh = nexthop_new();
+	memcpy(nh, nhop, sizeof(*nh));
+
+	nexthop_group_add_sorted(pbrms->nhg, nh);
 
 	memset(&find, 0, sizeof(find));
 	pbr_nht_nexthop_make_name(pbrms->parent->name, PBR_NHC_NAMELEN,
@@ -539,7 +553,7 @@ void pbr_nht_add_individual_nexthop(struct pbr_map_sequence *pbrms)
 	pbr_nht_install_nexthop_group(pnhgc, *pbrms->nhg);
 }
 
-void pbr_nht_delete_individual_nexthop(struct pbr_map_sequence *pbrms)
+static void pbr_nht_release_individual_nexthop(struct pbr_map_sequence *pbrms)
 {
 	struct pbr_nexthop_group_cache *pnhgc;
 	struct pbr_nexthop_group_cache find;
@@ -548,8 +562,6 @@ void pbr_nht_delete_individual_nexthop(struct pbr_map_sequence *pbrms)
 	struct nexthop *nh;
 	enum nexthop_types_t nh_type = 0;
 
-	pbr_map_delete_nexthops(pbrms);
-
 	memset(&find, 0, sizeof(find));
 	snprintf(find.name, sizeof(find.name), "%s", pbrms->internal_nhg_name);
 	pnhgc = hash_lookup(pbr_nhg_hash, &find);
@@ -570,6 +582,13 @@ void pbr_nht_delete_individual_nexthop(struct pbr_map_sequence *pbrms)
 	XFREE(MTYPE_TMP, pbrms->internal_nhg_name);
 }
 
+void pbr_nht_delete_individual_nexthop(struct pbr_map_sequence *pbrms)
+{
+	pbr_map_delete_nexthops(pbrms);
+
+	pbr_nht_release_individual_nexthop(pbrms);
+}
+
 struct pbr_nexthop_group_cache *pbr_nht_add_group(const char *name)
 {
 	struct nexthop *nhop;
diff --git a/pbrd/pbr_nht.h b/pbrd/pbr_nht.h
index 4ef41cede7..2533942547 100644
--- a/pbrd/pbr_nht.h
+++ b/pbrd/pbr_nht.h
@@ -88,7 +88,8 @@ extern struct pbr_nexthop_group_cache *pbr_nht_add_group(const char *name);
 extern void pbr_nht_change_group(const char *name);
 extern void pbr_nht_delete_group(const char *name);
 
-extern void pbr_nht_add_individual_nexthop(struct pbr_map_sequence *pbrms);
+extern void pbr_nht_add_individual_nexthop(struct pbr_map_sequence *pbrms,
+					   const struct nexthop *nhop);
 extern void pbr_nht_delete_individual_nexthop(struct pbr_map_sequence *pbrms);
 /*
  * Given the tableid of the installed default
diff --git a/pbrd/pbr_vty.c b/pbrd/pbr_vty.c
index dfc8bec1bc..7c1391df2e 100644
--- a/pbrd/pbr_vty.c
+++ b/pbrd/pbr_vty.c
@@ -142,18 +142,14 @@ DEFPY(pbr_map_match_src, pbr_map_match_src_cmd,
 		if (pbrms->src) {
 			if (prefix_same(pbrms->src, prefix))
 				return CMD_SUCCESS;
+		} else
+			pbrms->src = prefix_new();
 
-			vty_out(vty,
-				"A `match src-ip XX` command already exists, please remove that first\n");
-			return CMD_WARNING_CONFIG_FAILED;
-		}
-
-		pbrms->src = prefix_new();
 		prefix_copy(pbrms->src, prefix);
 	} else
 		prefix_free(&pbrms->src);
 
-	pbr_map_check(pbrms);
+	pbr_map_check(pbrms, true);
 
 	return CMD_SUCCESS;
 }
@@ -174,18 +170,14 @@ DEFPY(pbr_map_match_dst, pbr_map_match_dst_cmd,
 		if (pbrms->dst) {
 			if (prefix_same(pbrms->dst, prefix))
 				return CMD_SUCCESS;
+		} else
+			pbrms->dst = prefix_new();
 
-			vty_out(vty,
-				"A `match dst-ip XX` command already exists, please remove that first\n");
-			return CMD_WARNING_CONFIG_FAILED;
-		}
-
-		pbrms->dst = prefix_new();
 		prefix_copy(pbrms->dst, prefix);
 	} else
 		prefix_free(&pbrms->dst);
 
-	pbr_map_check(pbrms);
+	pbr_map_check(pbrms, true);
 
 	return CMD_SUCCESS;
 }
@@ -205,26 +197,49 @@ DEFPY(pbr_map_match_mark, pbr_map_match_mark_cmd,
 #endif
 
 	if (!no) {
-		if (pbrms->mark) {
+		if (pbrms->mark)
 			if (pbrms->mark == (uint32_t)mark)
 				return CMD_SUCCESS;
 
-			vty_out(vty,
-				"A `match mark XX` command already exists, please remove that first\n");
-			return CMD_WARNING_CONFIG_FAILED;
-		}
-
 		pbrms->mark = (uint32_t)mark;
 	} else
 		pbrms->mark = 0;
 
-	pbr_map_check(pbrms);
+	pbr_map_check(pbrms, true);
 
 	return CMD_SUCCESS;
 }
 
-#define SET_VRF_EXISTS_STR                                                     \
-	"A `set vrf XX` command already exists, please remove that first\n"
+static void pbrms_clear_set_vrf_config(struct pbr_map_sequence *pbrms)
+{
+	if (pbrms->vrf_lookup || pbrms->vrf_unchanged) {
+		pbr_map_delete_vrf(pbrms);
+		pbrms->vrf_name[0] = '\0';
+		pbrms->vrf_lookup = false;
+		pbrms->vrf_unchanged = false;
+	}
+}
+
+static void pbrms_clear_set_nhg_config(struct pbr_map_sequence *pbrms)
+{
+	if (pbrms->nhgrp_name)
+		pbr_map_delete_nexthops(pbrms);
+}
+
+static void pbrms_clear_set_nexthop_config(struct pbr_map_sequence *pbrms)
+{
+	if (pbrms->nhg)
+		pbr_nht_delete_individual_nexthop(pbrms);
+}
+
+static void pbrms_clear_set_config(struct pbr_map_sequence *pbrms)
+{
+	pbrms_clear_set_vrf_config(pbrms);
+	pbrms_clear_set_nhg_config(pbrms);
+	pbrms_clear_set_nexthop_config(pbrms);
+
+	pbrms->nhs_installed = false;
+}
 
 DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd,
       "[no] set nexthop-group NHGNAME$name",
@@ -236,17 +251,6 @@ DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd,
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
 	struct nexthop_group_cmd *nhgc;
 
-	if (pbrms->nhg) {
-		vty_out(vty,
-			"A `set nexthop XX` command already exists, please remove that first\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
-
-	if (pbrms->vrf_lookup || pbrms->vrf_unchanged) {
-		vty_out(vty, SET_VRF_EXISTS_STR);
-		return CMD_WARNING_CONFIG_FAILED;
-	}
-
 	nhgc = nhgc_find(name);
 	if (!nhgc) {
 		vty_out(vty, "Specified nexthop-group %s does not exist\n",
@@ -255,29 +259,22 @@ DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd,
 			"PBR-MAP will not be applied until it is created\n");
 	}
 
-	if (no) {
-		if (pbrms->nhgrp_name && strcmp(name, pbrms->nhgrp_name) == 0)
-			pbr_map_delete_nexthops(pbrms);
-		else {
-			vty_out(vty,
-				"Nexthop Group specified: %s does not exist to remove\n",
-				name);
-			return CMD_WARNING_CONFIG_FAILED;
-		}
-	} else {
-		if (pbrms->nhgrp_name) {
-			if (strcmp(name, pbrms->nhgrp_name) != 0) {
-				vty_out(vty,
-					"Please delete current nexthop group before modifying current one\n");
-				return CMD_WARNING_CONFIG_FAILED;
-			}
-
-			return CMD_SUCCESS;
-		}
-		pbrms->nhgrp_name = XSTRDUP(MTYPE_TMP, name);
-		pbr_map_check(pbrms);
+	if (no && pbrms->nhgrp_name && strcmp(name, pbrms->nhgrp_name) != 0) {
+		vty_out(vty,
+			"Nexthop Group specified: %s does not exist to remove\n",
+			name);
+		return CMD_WARNING_CONFIG_FAILED;
 	}
 
+	if (pbrms->nhgrp_name && strcmp(name, pbrms->nhgrp_name) == 0)
+		return CMD_SUCCESS;
+
+	/* This is new/replacement config */
+	pbrms_clear_set_config(pbrms);
+
+	pbrms->nhgrp_name = XSTRDUP(MTYPE_TMP, name);
+	pbr_map_check(pbrms, true);
+
 	return CMD_SUCCESS;
 }
 
@@ -301,18 +298,7 @@ DEFPY(pbr_map_nexthop, pbr_map_nexthop_cmd,
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
 	struct vrf *vrf;
 	struct nexthop nhop;
-	struct nexthop *nh;
-
-	if (pbrms->nhgrp_name) {
-		vty_out(vty,
-			"Please unconfigure the nexthop group before adding an individual nexthop\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
-
-	if (pbrms->vrf_lookup || pbrms->vrf_unchanged) {
-		vty_out(vty, SET_VRF_EXISTS_STR);
-		return CMD_WARNING_CONFIG_FAILED;
-	}
+	struct nexthop *nh = NULL;
 
 	if (vrf_name)
 		vrf = vrf_lookup_by_name(vrf_name);
@@ -362,45 +348,23 @@ DEFPY(pbr_map_nexthop, pbr_map_nexthop_cmd,
 
 	if (pbrms->nhg)
 		nh = nexthop_exists(pbrms->nhg, &nhop);
-	else {
-		char buf[PBR_NHC_NAMELEN];
 
-		if (no) {
-			vty_out(vty, "No nexthops to delete\n");
-			return CMD_WARNING_CONFIG_FAILED;
-		}
-
-		pbrms->nhg = nexthop_group_new();
-		pbrms->internal_nhg_name =
-			XSTRDUP(MTYPE_TMP,
-				pbr_nht_nexthop_make_name(pbrms->parent->name,
-							  PBR_NHC_NAMELEN,
-							  pbrms->seqno,
-							  buf));
-		nh = NULL;
+	if (no && !nh) {
+		vty_out(vty, "No nexthops to delete\n");
+		return CMD_WARNING_CONFIG_FAILED;
 	}
 
-	if (no) {
-		if (nh)
-			pbr_nht_delete_individual_nexthop(pbrms);
-	} else if (!nh) {
+	if (nh) /* Same config re-entered */
+		goto done;
 
-		if (pbrms->nhg->nexthop) {
-			vty_out(vty,
-				"If you would like more than one nexthop please use nexthop-groups\n");
-			return CMD_WARNING_CONFIG_FAILED;
-		}
+	/* This is new/replacement config */
+	pbrms_clear_set_config(pbrms);
 
-		/* must be adding new nexthop since !no and !nexthop_exists */
-		nh = nexthop_new();
+	pbr_nht_add_individual_nexthop(pbrms, &nhop);
 
-		memcpy(nh, &nhop, sizeof(nhop));
-		_nexthop_add(&pbrms->nhg->nexthop, nh);
-
-		pbr_nht_add_individual_nexthop(pbrms);
-		pbr_map_check(pbrms);
-	}
+	pbr_map_check(pbrms, true);
 
+done:
 	if (nhop.type == NEXTHOP_TYPE_IFINDEX
 	    || (nhop.type == NEXTHOP_TYPE_IPV6_IFINDEX
 		&& IN6_IS_ADDR_LINKLOCAL(&nhop.gate.ipv6))) {
@@ -425,54 +389,24 @@ DEFPY(pbr_map_vrf, pbr_map_vrf_cmd,
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
 
 	if (no) {
-		pbr_map_delete_vrf(pbrms);
-
 		/* Reset all data */
-		pbrms->nhs_installed = false;
-		pbrms->vrf_name[0] = '\0';
-		pbrms->vrf_lookup = false;
-		pbrms->vrf_unchanged = false;
+		pbrms_clear_set_config(pbrms);
 
 		return CMD_SUCCESS;
 	}
 
-	if (pbrms->nhgrp_name || pbrms->nhg) {
-		vty_out(vty,
-			"A `set nexthop/nexthop-group XX` command already exits, please remove that first\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
-
 	/*
-	 * Determine if a set vrf * command already exists.
-	 *
-	 * If its equivalent, just return success.
-	 *
-	 * Else, return failure, we don't allow atomic swaps yet.
+	 * If an equivalent set vrf * exists, just return success.
 	 */
-	if (vrf_name && pbrms->vrf_lookup) {
-		/* New vrf specified and one already exists */
-
-		/* Is this vrf different from one already configured? */
-		if (strncmp(pbrms->vrf_name, vrf_name, sizeof(pbrms->vrf_name))
-		    != 0)
-			goto vrf_exists;
-
+	if (vrf_name && pbrms->vrf_lookup
+	    && strncmp(pbrms->vrf_name, vrf_name, sizeof(pbrms->vrf_name)) == 0)
+		return CMD_SUCCESS;
+	else if (!vrf_name && pbrms->vrf_unchanged) /* Unchanged already set */
 		return CMD_SUCCESS;
 
-	} else if (!vrf_name && pbrms->vrf_unchanged) {
-		/* Unchanged specified and unchanged already exists */
-		return CMD_SUCCESS;
+	/* This is new/replacement config */
+	pbrms_clear_set_config(pbrms);
 
-	} else if (vrf_name && pbrms->vrf_unchanged) {
-		/* New vrf specified and unchanged is already set */
-		goto vrf_exists;
-
-	} else if (!vrf_name && pbrms->vrf_lookup) {
-		/* Unchanged specified and vrf to lookup already exists */
-		goto vrf_exists;
-	}
-
-	/* Create new lookup VRF or Unchanged */
 	if (vrf_name) {
 		if (!pbr_vrf_lookup_by_name(vrf_name)) {
 			vty_out(vty, "Specified: %s is non-existent\n",
@@ -485,13 +419,9 @@ DEFPY(pbr_map_vrf, pbr_map_vrf_cmd,
 	} else
 		pbrms->vrf_unchanged = true;
 
-	pbr_map_check(pbrms);
+	pbr_map_check(pbrms, true);
 
 	return CMD_SUCCESS;
-
-vrf_exists:
-	vty_out(vty, SET_VRF_EXISTS_STR);
-	return CMD_WARNING_CONFIG_FAILED;
 }
 
 DEFPY (pbr_policy,
diff --git a/pbrd/pbr_zebra.c b/pbrd/pbr_zebra.c
index cb28015c18..4178ad5803 100644
--- a/pbrd/pbr_zebra.c
+++ b/pbrd/pbr_zebra.c
@@ -548,7 +548,7 @@ static void pbr_encode_pbr_map_sequence(struct stream *s,
 }
 
 void pbr_send_pbr_map(struct pbr_map_sequence *pbrms,
-		      struct pbr_map_interface *pmi, bool install)
+		      struct pbr_map_interface *pmi, bool install, bool changed)
 {
 	struct pbr_map *pbrm = pbrms->parent;
 	struct stream *s;
@@ -560,11 +560,13 @@ void pbr_send_pbr_map(struct pbr_map_sequence *pbrms,
 	       pbrm->name, install, is_installed);
 
 	/*
-	 * If we are installed and asked to do so again
-	 * just return.  If we are not installed and asked
+	 * If we are installed and asked to do so again and the config
+	 * has not changed, just return.
+	 *
+	 * If we are not installed and asked
 	 * and asked to delete just return;
 	 */
-	if (install && is_installed)
+	if (install && is_installed && !changed)
 		return;
 
 	if (!install && !is_installed)
diff --git a/pbrd/pbr_zebra.h b/pbrd/pbr_zebra.h
index d5d938021a..cc42e21abe 100644
--- a/pbrd/pbr_zebra.h
+++ b/pbrd/pbr_zebra.h
@@ -36,7 +36,8 @@ extern void route_delete(struct pbr_nexthop_group_cache *pnhgc,
 extern void pbr_send_rnh(struct nexthop *nhop, bool reg);
 
 extern void pbr_send_pbr_map(struct pbr_map_sequence *pbrms,
-			     struct pbr_map_interface *pmi, bool install);
+			     struct pbr_map_interface *pmi, bool install,
+			     bool changed);
 
 extern struct pbr_interface *pbr_if_new(struct interface *ifp);
 

From bce03323a815b5d6cd7443dd93ebbe94b700c2fe Mon Sep 17 00:00:00 2001
From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Tue, 7 Apr 2020 16:53:52 -0400
Subject: [PATCH 6/9] pbrd: separate `set *` and `no set *` commands

Separate out the `set *` and `no set *` commands into
different DEFPYs to make the logic of the code easier to
read.

Further, allow non-exlpicit no commands.

So `no set nexthop`, `no set nexthop-group`, and
`no set vrf` will now work without having to specify
anymore data. Before you had to match what was already
there explicitly.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---
 pbrd/pbr_vty.c | 95 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 64 insertions(+), 31 deletions(-)

diff --git a/pbrd/pbr_vty.c b/pbrd/pbr_vty.c
index 7c1391df2e..4a3a9ca382 100644
--- a/pbrd/pbr_vty.c
+++ b/pbrd/pbr_vty.c
@@ -242,8 +242,7 @@ static void pbrms_clear_set_config(struct pbr_map_sequence *pbrms)
 }
 
 DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd,
-      "[no] set nexthop-group NHGNAME$name",
-      NO_STR
+      "set nexthop-group NHGNAME$name",
       "Set for the PBR-MAP\n"
       "nexthop-group to use\n"
       "The name of the nexthop-group\n")
@@ -259,13 +258,6 @@ DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd,
 			"PBR-MAP will not be applied until it is created\n");
 	}
 
-	if (no && pbrms->nhgrp_name && strcmp(name, pbrms->nhgrp_name) != 0) {
-		vty_out(vty,
-			"Nexthop Group specified: %s does not exist to remove\n",
-			name);
-		return CMD_WARNING_CONFIG_FAILED;
-	}
-
 	if (pbrms->nhgrp_name && strcmp(name, pbrms->nhgrp_name) == 0)
 		return CMD_SUCCESS;
 
@@ -278,14 +270,27 @@ DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd,
 	return CMD_SUCCESS;
 }
 
+DEFPY(no_pbr_map_nexthop_group, no_pbr_map_nexthop_group_cmd,
+      "no set nexthop-group [NHGNAME$name]",
+      NO_STR
+      "Set for the PBR-MAP\n"
+      "nexthop-group to use\n"
+      "The name of the nexthop-group\n")
+{
+	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
+
+	pbrms_clear_set_config(pbrms);
+
+	return CMD_SUCCESS;
+}
+
 DEFPY(pbr_map_nexthop, pbr_map_nexthop_cmd,
-      "[no] set nexthop\
+      "set nexthop\
         <\
 	  <A.B.C.D|X:X::X:X>$addr [INTERFACE$intf]\
 	  |INTERFACE$intf\
 	>\
         [nexthop-vrf NAME$vrf_name]",
-      NO_STR
       "Set for the PBR-MAP\n"
       "Specify one of the nexthops in this map\n"
       "v4 Address\n"
@@ -349,11 +354,6 @@ DEFPY(pbr_map_nexthop, pbr_map_nexthop_cmd,
 	if (pbrms->nhg)
 		nh = nexthop_exists(pbrms->nhg, &nhop);
 
-	if (no && !nh) {
-		vty_out(vty, "No nexthops to delete\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
-
 	if (nh) /* Same config re-entered */
 		goto done;
 
@@ -378,23 +378,39 @@ done:
 	return CMD_SUCCESS;
 }
 
-DEFPY(pbr_map_vrf, pbr_map_vrf_cmd,
-      "[no] set vrf <NAME$vrf_name|unchanged>",
+DEFPY(no_pbr_map_nexthop, no_pbr_map_nexthop_cmd,
+      "no set nexthop\
+        [<\
+	  <A.B.C.D|X:X::X:X>$addr [INTERFACE$intf]\
+	  |INTERFACE$intf\
+	>\
+        [nexthop-vrf NAME$vrf_name]]",
       NO_STR
       "Set for the PBR-MAP\n"
+      "Specify one of the nexthops in this map\n"
+      "v4 Address\n"
+      "v6 Address\n"
+      "Interface to use\n"
+      "Interface to use\n"
+      "If the nexthop is in a different vrf tell us\n"
+      "The nexthop-vrf Name\n")
+{
+	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
+
+	pbrms_clear_set_config(pbrms);
+
+	return CMD_SUCCESS;
+}
+
+DEFPY(pbr_map_vrf, pbr_map_vrf_cmd,
+      "set vrf <NAME$vrf_name|unchanged>",
+      "Set for the PBR-MAP\n"
       "Specify the VRF for this map\n"
       "The VRF Name\n"
       "Use the interface's VRF for lookup\n")
 {
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
 
-	if (no) {
-		/* Reset all data */
-		pbrms_clear_set_config(pbrms);
-
-		return CMD_SUCCESS;
-	}
-
 	/*
 	 * If an equivalent set vrf * exists, just return success.
 	 */
@@ -404,16 +420,15 @@ DEFPY(pbr_map_vrf, pbr_map_vrf_cmd,
 	else if (!vrf_name && pbrms->vrf_unchanged) /* Unchanged already set */
 		return CMD_SUCCESS;
 
+	if (vrf_name && !pbr_vrf_lookup_by_name(vrf_name)) {
+		vty_out(vty, "Specified: %s is non-existent\n", vrf_name);
+		return CMD_WARNING_CONFIG_FAILED;
+	}
+
 	/* This is new/replacement config */
 	pbrms_clear_set_config(pbrms);
 
 	if (vrf_name) {
-		if (!pbr_vrf_lookup_by_name(vrf_name)) {
-			vty_out(vty, "Specified: %s is non-existent\n",
-				vrf_name);
-			return CMD_WARNING_CONFIG_FAILED;
-		}
-
 		pbrms->vrf_lookup = true;
 		strlcpy(pbrms->vrf_name, vrf_name, sizeof(pbrms->vrf_name));
 	} else
@@ -424,6 +439,21 @@ DEFPY(pbr_map_vrf, pbr_map_vrf_cmd,
 	return CMD_SUCCESS;
 }
 
+DEFPY(no_pbr_map_vrf, no_pbr_map_vrf_cmd,
+      "no set vrf [<NAME$vrf_name|unchanged>]",
+      NO_STR
+      "Set for the PBR-MAP\n"
+      "Specify the VRF for this map\n"
+      "The VRF Name\n"
+      "Use the interface's VRF for lookup\n")
+{
+	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
+
+	pbrms_clear_set_config(pbrms);
+
+	return CMD_SUCCESS;
+}
+
 DEFPY (pbr_policy,
 	pbr_policy_cmd,
 	"[no] pbr-policy PBRMAP$mapname",
@@ -827,8 +857,11 @@ void pbr_vty_init(void)
 	install_element(PBRMAP_NODE, &pbr_map_match_dst_cmd);
 	install_element(PBRMAP_NODE, &pbr_map_match_mark_cmd);
 	install_element(PBRMAP_NODE, &pbr_map_nexthop_group_cmd);
+	install_element(PBRMAP_NODE, &no_pbr_map_nexthop_group_cmd);
 	install_element(PBRMAP_NODE, &pbr_map_nexthop_cmd);
+	install_element(PBRMAP_NODE, &no_pbr_map_nexthop_cmd);
 	install_element(PBRMAP_NODE, &pbr_map_vrf_cmd);
+	install_element(PBRMAP_NODE, &no_pbr_map_vrf_cmd);
 	install_element(VIEW_NODE, &show_pbr_cmd);
 	install_element(VIEW_NODE, &show_pbr_map_cmd);
 	install_element(VIEW_NODE, &show_pbr_interface_cmd);

From b6d34c260939cdcf7bba327108d2de35fc58fb54 Mon Sep 17 00:00:00 2001
From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Wed, 8 Apr 2020 19:09:29 -0400
Subject: [PATCH 7/9] zebra: set family on read in rule prefix

We were not setting the family for the prefix on the src/dst
IP of the rule.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---
 zebra/rule_netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/zebra/rule_netlink.c b/zebra/rule_netlink.c
index 34df2485c0..a361f73c10 100644
--- a/zebra/rule_netlink.c
+++ b/zebra/rule_netlink.c
@@ -271,6 +271,7 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 			memcpy(&rule.rule.filter.src_ip.u.prefix6,
 			       RTA_DATA(tb[FRA_SRC]), 16);
 		rule.rule.filter.src_ip.prefixlen = frh->src_len;
+		rule.rule.filter.src_ip.family = frh->family;
 		rule.rule.filter.filter_bm |= PBR_FILTER_SRC_IP;
 	}
 
@@ -282,6 +283,7 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 			memcpy(&rule.rule.filter.dst_ip.u.prefix6,
 			       RTA_DATA(tb[FRA_DST]), 16);
 		rule.rule.filter.dst_ip.prefixlen = frh->dst_len;
+		rule.rule.filter.dst_ip.family = frh->family;
 		rule.rule.filter.filter_bm |= PBR_FILTER_DST_IP;
 	}
 

From ab35be755ff5902cd0887fdbd4ee753f2969d136 Mon Sep 17 00:00:00 2001
From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Wed, 8 Apr 2020 19:10:24 -0400
Subject: [PATCH 8/9] zebra: read in and sweep rules on startup

On startup of zebra, read in all ipv4/ipv6 rules from
the kernel and remove any with the zebra proto.

If there are any, this means we failed to remove them
on shutdown due to a crash or something. Without this,
users have to manually remove them with iproute2 or some
such and its really annoying.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---
 zebra/rule_netlink.c | 108 +++++++++++++++++++++++++++++++++++--------
 zebra/zebra_ns.c     |   1 +
 2 files changed, 90 insertions(+), 19 deletions(-)

diff --git a/zebra/rule_netlink.c b/zebra/rule_netlink.c
index a361f73c10..07e1902f65 100644
--- a/zebra/rule_netlink.c
+++ b/zebra/rule_netlink.c
@@ -204,6 +204,10 @@ enum zebra_dplane_result kernel_update_pbr_rule(struct zebra_pbr_rule *old_rule,
  * DELs are notified up, if other attributes indicate it may be a
  * notification of interest. The expectation is that if this corresponds
  * to a PBR rule added by FRR, it will be readded.
+ *
+ * If startup and we see a rule we created, delete it as its leftover
+ * from a previous instance and should have been removed on shutdown.
+ *
  */
 int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 {
@@ -215,15 +219,12 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 	struct zebra_pbr_rule rule = {};
 	char buf1[PREFIX_STRLEN];
 	char buf2[PREFIX_STRLEN];
+	uint8_t proto = 0;
 
 	/* Basic validation followed by extracting attributes. */
 	if (h->nlmsg_type != RTM_NEWRULE && h->nlmsg_type != RTM_DELRULE)
 		return 0;
 
-	/* TBD */
-	if (h->nlmsg_type == RTM_NEWRULE)
-		return 0;
-
 	len = h->nlmsg_len - NLMSG_LENGTH(sizeof(struct fib_rule_hdr));
 	if (len < 0) {
 		zlog_err(
@@ -247,19 +248,6 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 	memset(tb, 0, sizeof(tb));
 	netlink_parse_rtattr(tb, FRA_MAX, RTM_RTA(frh), len);
 
-	/* TBD: We don't care about rules not specifying an IIF. */
-	if (tb[FRA_IFNAME] == NULL)
-		return 0;
-
-	ifname = (char *)RTA_DATA(tb[FRA_IFNAME]);
-	zns = zebra_ns_lookup(ns_id);
-
-	/* If we don't know the interface, we don't care. */
-	if (!if_lookup_by_name_per_ns(zns, ifname))
-		return 0;
-
-	strlcpy(rule.ifname, ifname, sizeof(rule.ifname));
-
 	if (tb[FRA_PRIORITY])
 		rule.rule.priority = *(uint32_t *)RTA_DATA(tb[FRA_PRIORITY]);
 
@@ -292,6 +280,49 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 	else
 		rule.rule.action.table = frh->table;
 
+	/* TBD: We don't care about rules not specifying an IIF. */
+	if (tb[FRA_IFNAME] == NULL)
+		return 0;
+
+	if (tb[FRA_PROTOCOL])
+		proto = *(uint8_t *)RTA_DATA(tb[FRA_PROTOCOL]);
+
+	ifname = (char *)RTA_DATA(tb[FRA_IFNAME]);
+	strlcpy(rule.ifname, ifname, sizeof(rule.ifname));
+
+	if (h->nlmsg_type == RTM_NEWRULE) {
+		/*
+		 * If we see a rule at startup we created, delete it now.
+		 * It should have been flushed on a previous shutdown.
+		 */
+		if (startup && proto == RTPROT_ZEBRA) {
+			int ret;
+
+			ret = netlink_rule_update(RTM_DELRULE, &rule);
+
+			zlog_debug(
+				"%s: %s leftover rule: family %s IF %s(%u) Pref %u Src %s Dst %s Table %u",
+				__func__,
+				((ret == 0) ? "Removed" : "Failed to remove"),
+				nl_family_to_str(frh->family), rule.ifname,
+				rule.rule.ifindex, rule.rule.priority,
+				prefix2str(&rule.rule.filter.src_ip, buf1,
+					   sizeof(buf1)),
+				prefix2str(&rule.rule.filter.dst_ip, buf2,
+					   sizeof(buf2)),
+				rule.rule.action.table);
+		}
+
+		/* TBD */
+		return 0;
+	}
+
+	zns = zebra_ns_lookup(ns_id);
+
+	/* If we don't know the interface, we don't care. */
+	if (!if_lookup_by_name_per_ns(zns, ifname))
+		return 0;
+
 	if (IS_ZEBRA_DEBUG_KERNEL)
 		zlog_debug(
 			"Rx %s family %s IF %s(%u) Pref %u Src %s Dst %s Table %u",
@@ -307,13 +338,52 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 	return kernel_pbr_rule_del(&rule);
 }
 
+/*
+ * Request rules from the kernel
+ */
+static int netlink_request_rules(struct zebra_ns *zns, int family, int type)
+{
+	struct {
+		struct nlmsghdr n;
+		struct fib_rule_hdr frh;
+		char buf[NL_PKT_BUF_SIZE];
+	} req;
+
+	memset(&req, 0, sizeof(req));
+	req.n.nlmsg_type = type;
+	req.n.nlmsg_flags = NLM_F_ROOT | NLM_F_MATCH | NLM_F_REQUEST;
+	req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct fib_rule_hdr));
+	req.frh.family = family;
+
+	return netlink_request(&zns->netlink_cmd, &req.n);
+}
+
 /*
  * Get to know existing PBR rules in the kernel - typically called at startup.
- * TBD.
  */
 int netlink_rules_read(struct zebra_ns *zns)
 {
-	return 0;
+	int ret;
+	struct zebra_dplane_info dp_info;
+
+	zebra_dplane_info_from_zns(&dp_info, zns, true);
+
+	ret = netlink_request_rules(zns, AF_INET, RTM_GETRULE);
+	if (ret < 0)
+		return ret;
+
+	ret = netlink_parse_info(netlink_rule_change, &zns->netlink_cmd,
+				 &dp_info, 0, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = netlink_request_rules(zns, AF_INET6, RTM_GETRULE);
+	if (ret < 0)
+		return ret;
+
+	ret = netlink_parse_info(netlink_rule_change, &zns->netlink_cmd,
+				 &dp_info, 0, 1);
+	return ret;
 }
 
 #endif /* HAVE_NETLINK */
diff --git a/zebra/zebra_ns.c b/zebra/zebra_ns.c
index 3287176ef5..4e51437337 100644
--- a/zebra/zebra_ns.c
+++ b/zebra/zebra_ns.c
@@ -126,6 +126,7 @@ int zebra_ns_enable(ns_id_t ns_id, void **info)
 	kernel_init(zns);
 	interface_list(zns);
 	route_read(zns);
+	kernel_read_pbr_rules(zns);
 
 	/* Initiate Table Manager per ZNS */
 	table_manager_enable(ns_id);

From f732636d70ce98956ea9bcfb5d7e7578ec3d05f0 Mon Sep 17 00:00:00 2001
From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Tue, 14 Apr 2020 11:09:54 -0400
Subject: [PATCH 9/9] pbrd: fix wording in pbr_send_pbr_map comment

Fix some bad wording in a comment when deciding whether
to send a pbr map sequence to zebra.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---
 pbrd/pbr_zebra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pbrd/pbr_zebra.c b/pbrd/pbr_zebra.c
index 4178ad5803..de2a99e269 100644
--- a/pbrd/pbr_zebra.c
+++ b/pbrd/pbr_zebra.c
@@ -564,7 +564,7 @@ void pbr_send_pbr_map(struct pbr_map_sequence *pbrms,
 	 * has not changed, just return.
 	 *
 	 * If we are not installed and asked
-	 * and asked to delete just return;
+	 * to delete just return.
 	 */
 	if (install && is_installed && !changed)
 		return;