pbrd: use flags to indicate active fields

Before now, PBRD used non-zero values to imply that a rule's
    match or action field was active. This approach was getting
    cumbersome for fields where 0 is a valid active value and
    various field-specific magic values had to be used.

    This commit changes PBRD to use a flag bit per field to
    indicate that the field is active.

Signed-off-by: G. Paul Ziemba <paulz@labn.net>

lib/pbr.h

@@ -57,7 +57,7 @@ struct pbr_filter {
 	struct prefix src_ip;
 	struct prefix dst_ip;
 
-	/* Source and Destination higher-layer (TCP/UDP) port numbers */
+	/* Source and Destination layer 4 (TCP/UDP/etc.) port numbers */
 	uint16_t src_port;
 	uint16_t dst_port;
 
@@ -88,11 +88,11 @@ struct pbr_filter {
 struct pbr_action {
 	uint32_t flags;
 
-#define PBR_ACTION_TABLE      (1 << 0)
+#define PBR_ACTION_TABLE		(1 << 0)
-#define PBR_ACTION_QUEUE_ID   (1 << 1)
+#define PBR_ACTION_QUEUE_ID		(1 << 1)
-#define PBR_ACTION_PCP        (1 << 2)
+#define PBR_ACTION_PCP			(1 << 2)
-#define PBR_ACTION_VLAN_ID    (1 << 3)
+#define PBR_ACTION_VLAN_ID		(1 << 3)
-#define PBR_ACTION_VLAN_FLAGS (1 << 4)
+#define PBR_ACTION_VLAN_STRIP_INNER_ANY (1 << 4)
 
 	uint32_t table;
 	uint32_t queue_id;
@@ -100,7 +100,6 @@ struct pbr_action {
 	/* VLAN */
 	uint8_t pcp;
 	uint16_t vlan_id;
-	uint16_t vlan_flags;
 
 
 };

lib/zclient.c

@@ -1631,40 +1631,79 @@ static void zapi_pbr_rule_filter_encode(struct stream *s, struct pbr_filter *f)
 	assert((f->src_ip.family == AF_INET) || (f->src_ip.family == AF_INET6));
 
 	stream_putl(s, f->filter_bm);
-	stream_putc(s, f->ip_proto);
+
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_IP_PROTOCOL))
+		stream_putc(s, f->ip_proto);
 
 	/* addresses */
-	zapi_encode_prefix(s, &f->src_ip, f->src_ip.family);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_SRC_IP))
-	zapi_encode_prefix(s, &f->dst_ip, f->dst_ip.family);
+		zapi_encode_prefix(s, &f->src_ip, f->src_ip.family);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_DST_IP))
+		zapi_encode_prefix(s, &f->dst_ip, f->dst_ip.family);
 
 	/* port numbers */
-	stream_putw(s, f->src_port);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_SRC_PORT))
-	stream_putw(s, f->dst_port);
+		stream_putw(s, f->src_port);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_DST_PORT))
+		stream_putw(s, f->dst_port);
+
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_DSCP))
+		stream_putc(s, f->dsfield & PBR_DSFIELD_DSCP);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_ECN))
+		stream_putc(s, f->dsfield & PBR_DSFIELD_ECN);
 
 	/* vlan */
-	stream_putc(s, f->pcp);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_PCP))
-	stream_putw(s, f->vlan_id);
+		stream_putc(s, f->pcp);
-	stream_putw(s, f->vlan_flags);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_VLAN_ID))
+		stream_putw(s, f->vlan_id);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_VLAN_FLAGS))
+		stream_putw(s, f->vlan_flags);
 
-	stream_putc(s, f->dsfield);
+
-	stream_putl(s, f->fwmark);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_FWMARK))
+		stream_putl(s, f->fwmark);
 }
 
 static bool zapi_pbr_rule_filter_decode(struct stream *s, struct pbr_filter *f)
 {
+	uint8_t dscp = 0;
+	uint8_t ecn = 0;
+
 	STREAM_GETL(s, f->filter_bm);
-	STREAM_GETC(s, f->ip_proto);
+
-	if (!zapi_decode_prefix(s, &(f->src_ip)))
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_IP_PROTOCOL))
-		goto stream_failure;
+		STREAM_GETC(s, f->ip_proto);
-	if (!zapi_decode_prefix(s, &(f->dst_ip)))
+
-		goto stream_failure;
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_SRC_IP))
-	STREAM_GETW(s, f->src_port);
+		if (!zapi_decode_prefix(s, &(f->src_ip)))
-	STREAM_GETW(s, f->dst_port);
+			goto stream_failure;
-	STREAM_GETC(s, f->pcp);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_DST_IP))
-	STREAM_GETW(s, f->vlan_id);
+		if (!zapi_decode_prefix(s, &(f->dst_ip)))
-	STREAM_GETW(s, f->vlan_flags);
+			goto stream_failure;
-	STREAM_GETC(s, f->dsfield);
+
-	STREAM_GETL(s, f->fwmark);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_SRC_PORT))
+		STREAM_GETW(s, f->src_port);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_DST_PORT))
+		STREAM_GETW(s, f->dst_port);
+
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_DSCP))
+		STREAM_GETC(s, dscp);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_ECN))
+		STREAM_GETC(s, ecn);
+	f->dsfield = (dscp & PBR_DSFIELD_DSCP) | (ecn & PBR_DSFIELD_ECN);
+
+	/* vlan */
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_PCP))
+		STREAM_GETC(s, f->pcp);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_VLAN_ID))
+		STREAM_GETW(s, f->vlan_id);
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_VLAN_FLAGS))
+		STREAM_GETW(s, f->vlan_flags);
+
+	if (CHECK_FLAG(f->filter_bm, PBR_FILTER_FWMARK))
+		STREAM_GETL(s, f->fwmark);
+
 	return true;
 
 stream_failure:
@@ -1674,21 +1713,34 @@ stream_failure:
 static void zapi_pbr_rule_action_encode(struct stream *s, struct pbr_action *a)
 {
 	stream_putl(s, a->flags);
-	stream_putl(s, a->table);
+
-	stream_putl(s, a->queue_id);
+	if (CHECK_FLAG(a->flags, PBR_ACTION_TABLE))
-	stream_putc(s, a->pcp);
+		stream_putl(s, a->table);
-	stream_putw(s, a->vlan_id);
+	if (CHECK_FLAG(a->flags, PBR_ACTION_QUEUE_ID))
-	stream_putw(s, a->vlan_flags);
+		stream_putl(s, a->queue_id);
+
+	/* L2 */
+	if (CHECK_FLAG(a->flags, PBR_ACTION_PCP))
+		stream_putc(s, a->pcp);
+	if (CHECK_FLAG(a->flags, PBR_ACTION_VLAN_ID))
+		stream_putw(s, a->vlan_id);
 }
 
 static bool zapi_pbr_rule_action_decode(struct stream *s, struct pbr_action *a)
 {
 	STREAM_GETL(s, a->flags);
-	STREAM_GETL(s, a->table);
+
-	STREAM_GETL(s, a->queue_id);
+	if (CHECK_FLAG(a->flags, PBR_ACTION_TABLE))
-	STREAM_GETC(s, a->pcp);
+		STREAM_GETL(s, a->table);
-	STREAM_GETW(s, a->vlan_id);
+	if (CHECK_FLAG(a->flags, PBR_ACTION_QUEUE_ID))
-	STREAM_GETW(s, a->vlan_flags);
+		STREAM_GETL(s, a->queue_id);
+
+	/* L2 */
+	if (CHECK_FLAG(a->flags, PBR_ACTION_PCP))
+		STREAM_GETC(s, a->pcp);
+	if (CHECK_FLAG(a->flags, PBR_ACTION_VLAN_ID))
+		STREAM_GETW(s, a->vlan_id);
+
 	return true;
 
 stream_failure:

pbrd/pbr_map.c

@@ -105,38 +105,6 @@ static bool pbrms_is_installed(const struct pbr_map_sequence *pbrms,
 	return false;
 }
 
-void pbr_set_match_clause_for_pcp(struct pbr_map_sequence *pbrms, bool set,
-				  uint8_t pcp)
-{
-	bool changed = false;
-
-	if (set) {
-		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP) ||
-		    (pcp != pbrms->match_pcp)) {
-			SET_FLAG(pbrms->filter_bm, PBR_FILTER_PCP);
-			pbrms->match_pcp = pcp;
-			changed = true;
-		}
-	} else {
-		if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP)) {
-			UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_PCP);
-			changed = true;
-		}
-	}
-	if (changed)
-		pbr_map_check(pbrms, true);
-}
-
-void pbr_set_match_clause_for_vlan(struct pbr_map_sequence *pbrms,
-				   uint16_t vlan_id, uint16_t vlan_flags)
-{
-	if (pbrms) {
-		pbrms->match_vlan_id = vlan_id;
-		pbrms->match_vlan_flags = vlan_flags;
-		pbr_map_check(pbrms, true);
-	}
-}
-
 /* If any sequence is installed on the interface, assume installed */
 static bool
 pbr_map_interface_is_installed(const struct pbr_map *pbrm,
@@ -562,14 +530,6 @@ struct pbr_map_sequence *pbrms_get(const char *name, uint32_t seqno)
 		pbrms->ruleno = pbr_nht_get_next_rule(seqno);
 		pbrms->parent = pbrm;
 
-		pbrms->match_vlan_id = 0;
-		pbrms->match_vlan_flags = 0;
-		pbrms->match_pcp = 0;
-
-		pbrms->action_vlan_id = 0;
-		pbrms->action_vlan_flags = 0;
-		pbrms->action_pcp = 0;
-
 		pbrms->action_queue_id = PBR_MAP_UNDEFINED_QUEUE_ID;
 
 		pbrms->reason =
@@ -634,13 +594,35 @@ pbr_map_sequence_check_nexthops_valid(struct pbr_map_sequence *pbrms)
 
 static void pbr_map_sequence_check_not_empty(struct pbr_map_sequence *pbrms)
 {
-	if (!pbrms->src && !pbrms->dst && !pbrms->mark && !pbrms->dsfield &&
+	/* clang-format off */
-	    !CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP) &&
+	if (
-	    !pbrms->action_pcp && !pbrms->match_vlan_id &&
+		!CHECK_FLAG(pbrms->filter_bm, (
-	    !pbrms->match_vlan_flags && !pbrms->action_vlan_id &&
+			PBR_FILTER_SRC_IP |
-	    !pbrms->action_vlan_flags &&
+			PBR_FILTER_DST_IP |
-	    pbrms->action_queue_id == PBR_MAP_UNDEFINED_QUEUE_ID)
+			PBR_FILTER_SRC_PORT |
+			PBR_FILTER_DST_PORT |
+
+			PBR_FILTER_IP_PROTOCOL |
+			PBR_FILTER_DSCP |
+			PBR_FILTER_ECN |
+
+			PBR_FILTER_FWMARK |
+			PBR_FILTER_PCP |
+			PBR_FILTER_VLAN_ID |
+			PBR_FILTER_VLAN_FLAGS
+		)) &&
+		!CHECK_FLAG(pbrms->action_bm, (
+
+			PBR_ACTION_PCP |
+			PBR_ACTION_VLAN_ID |
+			PBR_ACTION_VLAN_STRIP_INNER_ANY |
+
+			PBR_ACTION_QUEUE_ID
+		))
+	) {
 		pbrms->reason |= PBR_MAP_INVALID_EMPTY;
+	}
+	/* clang-format on */
 }
 
 static void pbr_map_sequence_check_vlan_actions(struct pbr_map_sequence *pbrms)
@@ -653,7 +635,8 @@ static void pbr_map_sequence_check_vlan_actions(struct pbr_map_sequence *pbrms)
 	 * The strip vlan action removes any inner tag, so it is invalid to
 	 * specify both a set and strip action.
 	 */
-	if ((pbrms->action_vlan_id != 0) && (pbrms->action_vlan_flags != 0))
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID) &&
+	    (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_STRIP_INNER_ANY)))
 		pbrms->reason |= PBR_MAP_INVALID_SET_STRIP_VLAN;
 }
 

pbrd/pbr_map.h

@@ -3,7 +3,9 @@
  * PBR-map Header
  * Copyright (C) 2018 Cumulus Networks, Inc.
  *               Donald Sharp
- * Copyright (c) 2023 LabN Consulting, L.L.C.
+ * Portions:
+ *	Copyright (c) 2023 LabN Consulting, L.L.C.
+ *	Copyright (c) 2021 The MITRE Corporation
  */
 #ifndef __PBR_MAP_H__
 #define __PBR_MAP_H__
@@ -54,6 +56,14 @@ struct pbr_map_interface {
 	bool delete;
 };
 
+enum pbr_forwarding_type {
+	PBR_FT_UNSPEC = 0,
+	PBR_FT_VRF_UNCHANGED,
+	PBR_FT_SETVRF,
+	PBR_FT_NEXTHOP_GROUP,
+	PBR_FT_NEXTHOP_SINGLE,
+};
+
 struct pbr_map_sequence {
 	struct pbr_map *parent;
 
@@ -109,14 +119,19 @@ struct pbr_map_sequence {
 	 * Action fields
 	 *****************************************************************/
 
+	/*
+	 * same bit definitions as in lib/pbr.h
+	 */
+	uint32_t action_bm;
+
 	uint8_t action_pcp;
 	uint8_t action_vlan_id;
-#define PBR_MAP_STRIP_INNER_ANY (1 << 0)
-	uint8_t action_vlan_flags;
 
 #define PBR_MAP_UNDEFINED_QUEUE_ID 0
 	uint32_t action_queue_id;
 
+	enum pbr_forwarding_type forwarding_type;
+
 	/*
 	 * Use interface's vrf.
 	 */
@@ -233,9 +248,4 @@ extern void pbr_map_check_vrf_nh_group_change(const char *nh_group,
 extern void pbr_map_check_interface_nh_group_change(const char *nh_group,
 						    struct interface *ifp,
 						    ifindex_t oldifindex);
-extern void pbr_set_match_clause_for_vlan(struct pbr_map_sequence *pbrms,
-					  uint16_t vlan_id,
-					  uint16_t vlan_flags);
-extern void pbr_set_match_clause_for_pcp(struct pbr_map_sequence *pbrms,
-					 bool set, uint8_t pcp);
 #endif

pbrd/pbr_nht.c

@@ -549,6 +549,7 @@ void pbr_nht_set_seq_nhg(struct pbr_map_sequence *pbrms, const char *name)
 		return;
 
 	pbrms->nhgrp_name = XSTRDUP(MTYPE_TMP, name);
+	pbrms->forwarding_type = PBR_FT_NEXTHOP_GROUP;
 
 	nhgc = nhgc_find(name);
 	if (!nhgc)
@@ -572,6 +573,7 @@ void pbr_nht_add_individual_nexthop(struct pbr_map_sequence *pbrms,
 		MTYPE_TMP,
 		pbr_nht_nexthop_make_name(pbrms->parent->name, PBR_NHC_NAMELEN,
 					  pbrms->seqno, buf));
+	pbrms->forwarding_type = PBR_FT_NEXTHOP_SINGLE;
 
 	nh = nexthop_new();
 	memcpy(nh, nhop, sizeof(*nh));

pbrd/pbr_zebra.c

@@ -564,44 +564,18 @@ static bool pbr_encode_pbr_map_sequence(struct stream *s,
 	r.filter.fwmark = pbrms->mark;
 	r.filter.ip_proto = pbrms->ip_proto;
 
-	/*
+	r.filter.filter_bm = pbrms->filter_bm;
-	 * Fix up filter flags for now, since PBRD doesn't maintain
-	 * them yet (aside from PBR_FILTER_PCP)
-	 */
-	if (!is_default_prefix(&r.filter.src_ip))
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_SRC_IP);
-	if (!is_default_prefix(&r.filter.dst_ip))
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_DST_IP);
-	if (r.filter.src_port)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_SRC_PORT);
-	if (r.filter.dst_port)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_DST_PORT);
-	if (r.filter.vlan_id)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_VLAN_ID);
-	if (r.filter.vlan_flags)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_VLAN_FLAGS);
-	if (r.filter.dsfield)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_DSFIELD);
-	if (r.filter.fwmark)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_FWMARK);
-	if (r.filter.ip_proto)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_IP_PROTOCOL);
 
 	/* actions */
 
+	SET_FLAG(r.action.flags, PBR_ACTION_TABLE); /* always valid */
+
 	/*
 	 * PBR should maintain its own set of action flags that we
 	 * can copy here instead of trying to infer from magic values.
 	 */
-	SET_FLAG(r.action.flags, PBR_ACTION_TABLE); /* always valid */
+
-	if (pbrms->action_queue_id != PBR_MAP_UNDEFINED_QUEUE_ID)
+	r.action.flags = pbrms->action_bm;
-		SET_FLAG(r.action.flags, PBR_ACTION_QUEUE_ID);
-	if (pbrms->action_pcp != 0)
-		SET_FLAG(r.action.flags, PBR_ACTION_PCP);
-	if (pbrms->action_vlan_id != 0)
-		SET_FLAG(r.action.flags, PBR_ACTION_VLAN_ID);
-	if (pbrms->action_vlan_flags != 0)
-		SET_FLAG(r.action.flags, PBR_ACTION_VLAN_FLAGS);
 
 	/*
 	 * if the user does not use the command "set vrf name unchanged"
@@ -619,9 +593,9 @@ static bool pbr_encode_pbr_map_sequence(struct stream *s,
 	}
 
 	r.action.queue_id = pbrms->action_queue_id;
+
 	r.action.pcp = pbrms->action_pcp;
 	r.action.vlan_id = pbrms->action_vlan_id;
-	r.action.vlan_flags = pbrms->action_vlan_flags;
 
 	strlcpy(r.ifname, ifp->name, sizeof(r.ifname));
 

zebra/zapi_msg.c

@@ -3188,10 +3188,32 @@ static inline void zread_rule(ZAPI_HANDLER_ARGS)
 
 		strlcpy(zpr.ifname, zpr.rule.ifname, sizeof(zpr.ifname));
 
+		if ((zpr.rule.family != AF_INET) &&
+		    (zpr.rule.family != AF_INET6)) {
+			zlog_warn("Unsupported PBR source IP family: %s (%hhu)",
+				  family2str(zpr.rule.family), zpr.rule.family);
+			return;
+		}
+
+		/*
+		 * Fixup filter src/dst IP addresses if they are unset
+		 * because the netlink code currently obtains address family
+		 * from them. Address family is used to specify which
+		 * kernel database to use when adding/deleting rule.
+		 *
+		 * TBD: propagate zpr.rule.family into dataplane and
+		 * netlink code so they can stop using filter src/dst addrs.
+		 */
+		if (!CHECK_FLAG(zpr.rule.filter.filter_bm, PBR_FILTER_SRC_IP))
+			zpr.rule.filter.src_ip.family = zpr.rule.family;
+		if (!CHECK_FLAG(zpr.rule.filter.filter_bm, PBR_FILTER_DST_IP))
+			zpr.rule.filter.dst_ip.family = zpr.rule.family;
+
+		/* TBD delete below block when netlink code gets family from zpr.rule.family */
 		if (!(zpr.rule.filter.src_ip.family == AF_INET
 		      || zpr.rule.filter.src_ip.family == AF_INET6)) {
 			zlog_warn(
-				"Unsupported PBR source IP family: %s (%hhu)",
+				"Unsupported PBR source IP family: %s (%u)",
 				family2str(zpr.rule.filter.src_ip.family),
 				zpr.rule.filter.src_ip.family);
 			return;
@@ -3199,11 +3221,12 @@ static inline void zread_rule(ZAPI_HANDLER_ARGS)
 		if (!(zpr.rule.filter.dst_ip.family == AF_INET
 		      || zpr.rule.filter.dst_ip.family == AF_INET6)) {
 			zlog_warn(
-				"Unsupported PBR destination IP family: %s (%hhu)",
+				"Unsupported PBR destination IP family: %s (%u)",
 				family2str(zpr.rule.filter.dst_ip.family),
 				zpr.rule.filter.dst_ip.family);
 			return;
 		}
+		/* TBD delete above block when netlink code gets family from zpr.rule.family */
 
 
 		zpr.vrf_id = zvrf->vrf->vrf_id;