Merge pull request #5161 from sworleys/Fix-Rule-Dbl-Free

lib,zebra: Fix PBR Rule Ifp Reference if Interface is Deleted

lib/pbr.h

@@ -90,7 +90,7 @@ struct pbr_rule {
 	uint32_t unique;
 	struct pbr_filter filter;
 	struct pbr_action action;
-	uint32_t ifindex;
+	ifindex_t ifindex;
 };
 
 /* TCP flags value shared

zebra/rule_netlink.c

@@ -86,9 +86,8 @@ static int netlink_rule_update(int cmd, struct zebra_pbr_rule *rule)
 	addattr32(&req.n, sizeof(req), FRA_PRIORITY, rule->rule.priority);
 
 	/* interface on which applied */
-	if (rule->ifp)
-		addattr_l(&req.n, sizeof(req), FRA_IFNAME, rule->ifp->name,
-			  strlen(rule->ifp->name) + 1);
+	addattr_l(&req.n, sizeof(req), FRA_IFNAME, rule->ifname,
+		  strlen(rule->ifname) + 1);
 
 	/* source IP, if specified */
 	if (IS_RULE_FILTERING_ON_SRC_IP(rule)) {
@@ -122,8 +121,7 @@ static int netlink_rule_update(int cmd, struct zebra_pbr_rule *rule)
 		zlog_debug(
 			"Tx %s family %s IF %s(%u) Pref %u Fwmark %u Src %s Dst %s Table %u",
 			nl_msg_type_to_str(cmd), nl_family_to_str(family),
-			rule->ifp ? rule->ifp->name : "Unknown",
-			rule->ifp ? rule->ifp->ifindex : 0, rule->rule.priority,
+			rule->ifname, rule->rule.ifindex, rule->rule.priority,
 			rule->rule.filter.fwmark,
 			prefix2str(&rule->rule.filter.src_ip, buf1,
 				   sizeof(buf1)),
@@ -227,13 +225,15 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 	if (tb[FRA_IFNAME] == NULL)
 		return 0;
 
-	/* If we don't know the interface, we don't care. */
 	ifname = (char *)RTA_DATA(tb[FRA_IFNAME]);
 	zns = zebra_ns_lookup(ns_id);
-	rule.ifp = if_lookup_by_name_per_ns(zns, ifname);
-	if (!rule.ifp)
+
+	/* 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]);
 
@@ -268,8 +268,8 @@ int netlink_rule_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 		zlog_debug(
 			"Rx %s family %s IF %s(%u) Pref %u Src %s Dst %s Table %u",
 			nl_msg_type_to_str(h->nlmsg_type),
-			nl_family_to_str(frh->family), rule.ifp->name,
-			rule.ifp->ifindex, rule.rule.priority,
+			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,

zebra/zapi_msg.c

@@ -783,10 +783,7 @@ void zsend_rule_notify_owner(struct zebra_pbr_rule *rule,
 	stream_putl(s, rule->rule.seq);
 	stream_putl(s, rule->rule.priority);
 	stream_putl(s, rule->rule.unique);
-	if (rule->ifp)
-		stream_putl(s, rule->ifp->ifindex);
-	else
-		stream_putl(s, 0);
+	stream_putl(s, rule->rule.ifindex);
 
 	stream_putw_at(s, 0, stream_get_endp(s));
 
@@ -2294,7 +2291,6 @@ static inline void zread_rule(ZAPI_HANDLER_ARGS)
 	struct zebra_pbr_rule zpr;
 	struct stream *s;
 	uint32_t total, i;
-	ifindex_t ifindex;
 
 	s = msg;
 	STREAM_GETL(s, total);
@@ -2319,17 +2315,20 @@ static inline void zread_rule(ZAPI_HANDLER_ARGS)
 		STREAM_GETW(s, zpr.rule.filter.dst_port);
 		STREAM_GETL(s, zpr.rule.filter.fwmark);
 		STREAM_GETL(s, zpr.rule.action.table);
-		STREAM_GETL(s, ifindex);
+		STREAM_GETL(s, zpr.rule.ifindex);
 
-		if (ifindex) {
-			zpr.ifp = if_lookup_by_index_per_ns(
-						zvrf->zns,
-						ifindex);
-			if (!zpr.ifp) {
+		if (zpr.rule.ifindex) {
+			struct interface *ifp;
+
+			ifp = if_lookup_by_index_per_ns(zvrf->zns,
+							zpr.rule.ifindex);
+			if (!ifp) {
 				zlog_debug("Failed to lookup ifindex: %u",
-					   ifindex);
+					   zpr.rule.ifindex);
 				return;
 			}
+
+			strlcpy(zpr.ifname, ifp->name, sizeof(zpr.ifname));
 		}
 
 		if (!is_default_prefix(&zpr.rule.filter.src_ip))

zebra/zebra_pbr.c

@@ -144,17 +144,12 @@ uint32_t zebra_pbr_rules_hash_key(const void *arg)
 	key = jhash_3words(rule->rule.seq, rule->rule.priority,
 			   rule->rule.action.table,
 			   prefix_hash_key(&rule->rule.filter.src_ip));
-	if (rule->ifp)
-		key = jhash_1word(rule->ifp->ifindex, key);
-	else
-		key = jhash_1word(0, key);
 
 	if (rule->rule.filter.fwmark)
-		key = jhash_1word(rule->rule.filter.fwmark, key);
+		key = jhash_3words(rule->rule.filter.fwmark, rule->vrf_id,
+				   rule->rule.ifindex, key);
 	else
-		key = jhash_1word(0, key);
-
-	key = jhash_1word(rule->vrf_id, key);
+		key = jhash_2words(rule->vrf_id, rule->rule.ifindex, key);
 
 	return jhash_3words(rule->rule.filter.src_port,
 			    rule->rule.filter.dst_port,
@@ -196,7 +191,7 @@ bool zebra_pbr_rules_hash_equal(const void *arg1, const void *arg2)
 	if (!prefix_same(&r1->rule.filter.dst_ip, &r2->rule.filter.dst_ip))
 		return false;
 
-	if (r1->ifp != r2->ifp)
+	if (r1->rule.ifindex != r2->rule.ifindex)
 		return false;
 
 	if (r1->vrf_id != r2->vrf_id)
@@ -208,7 +203,7 @@ bool zebra_pbr_rules_hash_equal(const void *arg1, const void *arg2)
 struct pbr_rule_unique_lookup {
 	struct zebra_pbr_rule *rule;
 	uint32_t unique;
-	struct interface *ifp;
+	ifindex_t ifindex;
 	vrf_id_t vrf_id;
 };
 
@@ -218,7 +213,7 @@ static int pbr_rule_lookup_unique_walker(struct hash_bucket *b, void *data)
 	struct zebra_pbr_rule *rule = b->data;
 
 	if (pul->unique == rule->rule.unique
-	    && pul->ifp == rule->ifp
+	    && pul->ifindex == rule->rule.ifindex
 	    && pul->vrf_id == rule->vrf_id) {
 		pul->rule = rule;
 		return HASHWALK_ABORT;
@@ -233,7 +228,7 @@ pbr_rule_lookup_unique(struct zebra_pbr_rule *zrule)
 	struct pbr_rule_unique_lookup pul;
 
 	pul.unique = zrule->rule.unique;
-	pul.ifp = zrule->ifp;
+	pul.ifindex = zrule->rule.ifindex;
 	pul.rule = NULL;
 	pul.vrf_id = zrule->vrf_id;
 	hash_walk(zrouter.rules_hash, &pbr_rule_lookup_unique_walker, &pul);
@@ -471,8 +466,12 @@ static void zebra_pbr_cleanup_rules(struct hash_bucket *b, void *data)
 
 	if (rule->sock == *sock) {
 		(void)kernel_del_pbr_rule(rule);
-		hash_release(zrouter.rules_hash, rule);
-		XFREE(MTYPE_TMP, rule);
+		if (hash_release(zrouter.rules_hash, rule))
+			XFREE(MTYPE_TMP, rule);
+		else
+			zlog_debug(
+				"%s: Rule seq: %u is being cleaned but we can't find it in our tables",
+				__func__, rule->rule.seq);
 	}
 }
 

zebra/zebra_pbr.h

@@ -41,7 +41,7 @@ struct zebra_pbr_rule {
 
 	struct pbr_rule rule;
 
-	struct interface *ifp;
+	char ifname[INTERFACE_NAMSIZ];
 
 	vrf_id_t vrf_id;
 };