[PATCH v2 iproute2-next 0/2] tc: support for new MPLS L2 VPN actions

[PATCH v2 iproute2-next 0/2] tc: support for new MPLS L2 VPN actions

[Guillaume Nault]

This patch series adds the possibility for TC to tunnel Ethernet frames
over MPLS.

Patch 1 allows adding or removing the Ethernet header.
Patch 2 allows pushing an MPLS LSE before the MAC header.

By combining these actions, it becomes possible to encapsulate an
entire Ethernet frame into MPLS, then add an outer Ethernet header
and send the resulting frame to the next hop.

v2: trivial coding style fix (line wrap).

Guillaume Nault (2):
  m_vlan: add pop_eth and push_eth actions
  m_mpls: add mac_push action

 lib/ll_proto.c            |  1 +
 man/man8/tc-mpls.8        | 44 ++++++++++++++++++--
 man/man8/tc-vlan.8        | 44 ++++++++++++++++++--
 tc/m_mpls.c               | 43 ++++++++++++++------
 tc/m_vlan.c               | 69 +++++++++++++++++++++++++++++++
 testsuite/tests/tc/mpls.t | 69 +++++++++++++++++++++++++++++++
 testsuite/tests/tc/vlan.t | 86 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 337 insertions(+), 19 deletions(-)
 create mode 100755 testsuite/tests/tc/mpls.t
 create mode 100755 testsuite/tests/tc/vlan.t

-- 
2.21.3

[PATCH v2 iproute2-next 1/2] m_vlan: add pop_eth and push_eth actions

[Guillaume Nault]

Add support for the new TCA_VLAN_ACT_POP_ETH and TCA_VLAN_ACT_PUSH_ETH
actions (kernel commit 19fbcb36a39e ("net/sched: act_vlan:
Add {POP,PUSH}_ETH actions"). These action let TC remove or add the
Ethernet at the head of a frame.

Drop an Ethernet header:
 # tc filter add dev ethX matchall action vlan pop_eth

Push an Ethernet header (the original frame must have no MAC header):
 # tc filter add dev ethX matchall action vlan \
       push_eth dst_mac 0a:00:00:00:00:02 src_mac 0a:00:00:00:00:01

Also add a test suite for m_vlan, which covers these new actions and
the pre-existing ones.

Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 man/man8/tc-vlan.8        | 39 +++++++++++++++++-
 tc/m_vlan.c               | 69 +++++++++++++++++++++++++++++++
 testsuite/tests/tc/vlan.t | 86 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 192 insertions(+), 2 deletions(-)
 create mode 100755 testsuite/tests/tc/vlan.t

diff --git a/man/man8/tc-vlan.8 b/man/man8/tc-vlan.8
index f5ffc25f..5c2808b1 100644
--- a/man/man8/tc-vlan.8
+++ b/man/man8/tc-vlan.8
@@ -5,8 +5,8 @@ vlan - vlan manipulation module
 .SH SYNOPSIS
 .in +8
 .ti -8
-.BR tc " ... " "action vlan" " { " pop " |"
-.IR PUSH " | " MODIFY " } [ " CONTROL " ]"
+.BR tc " ... " "action vlan" " { " pop " | " pop_eth " |"
+.IR PUSH " | " MODIFY " | " PUSH_ETH " } [ " CONTROL " ]"
 
 .ti -8
 .IR PUSH " := "
@@ -24,6 +24,11 @@ vlan - vlan manipulation module
 .IR VLANPRIO " ] "
 .BI id " VLANID"
 
+.ti -8
+.IR PUSH_ETH " := "
+.B push_eth
+.BI dst_mac " LLADDR " src_mac " LLADDR "
+
 .ti -8
 .IR CONTROL " := { "
 .BR reclassify " | " pipe " | " drop " | " continue " | " pass " | " goto " " chain " " CHAIN_INDEX " }"
@@ -43,6 +48,20 @@ modes require at least a
 and allow to optionally choose the
 .I VLANPROTO
 to use.
+
+The
+.B vlan
+action can also be used to add or remove the base Ethernet header. The
+.B pop_eth
+mode, which takes no argument, is used to remove the base Ethernet header. All
+existing VLANs must have been previously dropped. The opposite operation,
+adding a base Ethernet header, is done with the
+.B push_eth
+mode. In that case, the packet must have no MAC header (stacking MAC headers is
+not permitted). This mode is mostly useful when a previous action has
+encapsulated the whole original frame behind a network header and one needs
+to prepend an Ethernet header before forwarding the resulting packet.
+
 .SH OPTIONS
 .TP
 .B pop
@@ -58,6 +77,16 @@ Replace mode. Existing 802.1Q tag is replaced. Requires at least
 .B id
 option.
 .TP
+.B pop_eth
+Ethernet header decapsulation mode. Only works on a plain Ethernet header:
+VLANs, if any, must be removed first.
+.TP
+.B push_eth
+Ethernet header encapsulation mode. The Ethertype is automatically set
+using the network header type. Chaining Ethernet headers is not allowed: the
+packet must have no MAC header when using this mode. Requires the
+.BR "dst_mac " and " src_mac " options.
+.TP
 .BI id " VLANID"
 Specify the VLAN ID to encapsulate into.
 .I VLANID
@@ -73,6 +102,12 @@ Choose the VLAN protocol to use. At the time of writing, the kernel accepts only
 .BI priority " VLANPRIO"
 Choose the VLAN priority to use. Decimal number in range of 0-7.
 .TP
+.BI dst_mac " LLADDR"
+Choose the destination MAC address to use.
+.TP
+.BI src_mac " LLADDR"
+Choose the source MAC address to use.
+.TP
 .I CONTROL
 How to continue after executing this action.
 .RS
diff --git a/tc/m_vlan.c b/tc/m_vlan.c
index 1096ba0f..e6b21330 100644
--- a/tc/m_vlan.c
+++ b/tc/m_vlan.c
@@ -23,6 +23,8 @@ static const char * const action_names[] = {
 	[TCA_VLAN_ACT_POP] = "pop",
 	[TCA_VLAN_ACT_PUSH] = "push",
 	[TCA_VLAN_ACT_MODIFY] = "modify",
+	[TCA_VLAN_ACT_POP_ETH] = "pop_eth",
+	[TCA_VLAN_ACT_PUSH_ETH] = "push_eth",
 };
 
 static void explain(void)
@@ -31,6 +33,8 @@ static void explain(void)
 		"Usage: vlan pop\n"
 		"       vlan push [ protocol VLANPROTO ] id VLANID [ priority VLANPRIO ] [CONTROL]\n"
 		"       vlan modify [ protocol VLANPROTO ] id VLANID [ priority VLANPRIO ] [CONTROL]\n"
+		"       vlan pop_eth [CONTROL]\n"
+		"       vlan push_eth dst_mac LLADDR src_mac LLADDR [CONTROL]\n"
 		"       VLANPROTO is one of 802.1Q or 802.1AD\n"
 		"            with default: 802.1Q\n"
 		"       CONTROL := reclassify | pipe | drop | continue | pass |\n"
@@ -63,6 +67,10 @@ static int parse_vlan(struct action_util *a, int *argc_p, char ***argv_p,
 	char **argv = *argv_p;
 	struct rtattr *tail;
 	int action = 0;
+	char dst_mac[ETH_ALEN] = {};
+	int dst_mac_set = 0;
+	char src_mac[ETH_ALEN] = {};
+	int src_mac_set = 0;
 	__u16 id;
 	int id_set = 0;
 	__u16 proto;
@@ -95,6 +103,18 @@ static int parse_vlan(struct action_util *a, int *argc_p, char ***argv_p,
 				return -1;
 			}
 			action = TCA_VLAN_ACT_MODIFY;
+		} else if (matches(*argv, "pop_eth") == 0) {
+			if (action) {
+				unexpected(*argv);
+				return -1;
+			}
+			action = TCA_VLAN_ACT_POP_ETH;
+		} else if (matches(*argv, "push_eth") == 0) {
+			if (action) {
+				unexpected(*argv);
+				return -1;
+			}
+			action = TCA_VLAN_ACT_PUSH_ETH;
 		} else if (matches(*argv, "id") == 0) {
 			if (!has_push_attribs(action))
 				invarg("only valid for push/modify", *argv);
@@ -119,6 +139,22 @@ static int parse_vlan(struct action_util *a, int *argc_p, char ***argv_p,
 			if (get_u8(&prio, *argv, 0) || (prio & ~0x7))
 				invarg("prio is invalid", *argv);
 			prio_set = 1;
+		} else if (matches(*argv, "dst_mac") == 0) {
+			if (action != TCA_VLAN_ACT_PUSH_ETH)
+				invarg("only valid for push_eth", *argv);
+
+			NEXT_ARG();
+			if (ll_addr_a2n(dst_mac, sizeof(dst_mac), *argv) < 0)
+				invarg("dst_mac is invalid", *argv);
+			dst_mac_set = 1;
+		} else if (matches(*argv, "src_mac") == 0) {
+			if (action != TCA_VLAN_ACT_PUSH_ETH)
+				invarg("only valid for push_eth", *argv);
+
+			NEXT_ARG();
+			if (ll_addr_a2n(src_mac, sizeof(src_mac), *argv) < 0)
+				invarg("src_mac is invalid", *argv);
+			src_mac_set = 1;
 		} else if (matches(*argv, "help") == 0) {
 			usage();
 		} else {
@@ -150,6 +186,20 @@ static int parse_vlan(struct action_util *a, int *argc_p, char ***argv_p,
 		return -1;
 	}
 
+	if (action == TCA_VLAN_ACT_PUSH_ETH) {
+		if (!dst_mac_set) {
+			fprintf(stderr, "dst_mac needs to be set for %s\n",
+				action_names[action]);
+			explain();
+			return -1;
+		} else if (!src_mac_set) {
+			fprintf(stderr, "src_mac needs to be set for %s\n",
+				action_names[action]);
+			explain();
+			return -1;
+		}
+	}
+
 	parm.v_action = action;
 	tail = addattr_nest(n, MAX_MSG, tca_id);
 	addattr_l(n, MAX_MSG, TCA_VLAN_PARMS, &parm, sizeof(parm));
@@ -167,6 +217,12 @@ static int parse_vlan(struct action_util *a, int *argc_p, char ***argv_p,
 	}
 	if (prio_set)
 		addattr8(n, MAX_MSG, TCA_VLAN_PUSH_VLAN_PRIORITY, prio);
+	if (dst_mac_set)
+		addattr_l(n, MAX_MSG, TCA_VLAN_PUSH_ETH_DST, dst_mac,
+			  sizeof(dst_mac));
+	if (src_mac_set)
+		addattr_l(n, MAX_MSG, TCA_VLAN_PUSH_ETH_SRC, src_mac,
+			  sizeof(src_mac));
 
 	addattr_nest_end(n, tail);
 
@@ -216,6 +272,19 @@ static int print_vlan(struct action_util *au, FILE *f, struct rtattr *arg)
 			print_uint(PRINT_ANY, "priority", " priority %u", val);
 		}
 		break;
+	case TCA_VLAN_ACT_PUSH_ETH:
+		if (tb[TCA_VLAN_PUSH_ETH_DST] &&
+		    RTA_PAYLOAD(tb[TCA_VLAN_PUSH_ETH_DST]) == ETH_ALEN) {
+			ll_addr_n2a(RTA_DATA(tb[TCA_VLAN_PUSH_ETH_DST]),
+				    ETH_ALEN, 0, b1, sizeof(b1));
+			print_string(PRINT_ANY, "dst_mac", " dst_mac %s", b1);
+		}
+		if (tb[TCA_VLAN_PUSH_ETH_SRC &&
+		       RTA_PAYLOAD(tb[TCA_VLAN_PUSH_ETH_SRC]) == ETH_ALEN]) {
+			ll_addr_n2a(RTA_DATA(tb[TCA_VLAN_PUSH_ETH_SRC]),
+				    ETH_ALEN, 0, b1, sizeof(b1));
+			print_string(PRINT_ANY, "src_mac", " src_mac %s", b1);
+		}
 	}
 	print_action_control(f, " ", parm->action, "");
 
diff --git a/testsuite/tests/tc/vlan.t b/testsuite/tests/tc/vlan.t
new file mode 100755
index 00000000..b86dc364
--- /dev/null
+++ b/testsuite/tests/tc/vlan.t
@@ -0,0 +1,86 @@
+#!/bin/sh
+
+. lib/generic.sh
+
+DEV="$(rand_dev)"
+ts_ip "$0" "Add $DEV dummy interface" link add dev $DEV up type dummy
+ts_tc "$0" "Add ingress qdisc" qdisc add dev $DEV ingress
+
+reset_qdisc()
+{
+	ts_tc "$0" "Remove ingress qdisc" qdisc del dev $DEV ingress
+	ts_tc "$0" "Add ingress qdisc" qdisc add dev $DEV ingress
+}
+
+ts_tc "$0" "Add vlan action pop" \
+	filter add dev $DEV ingress matchall action vlan pop
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "vlan"
+test_on "pop"
+test_on "pipe"
+
+reset_qdisc
+ts_tc "$0" "Add vlan action push (default parameters)" \
+	filter add dev $DEV ingress matchall action vlan push id 5
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "vlan"
+test_on "push"
+test_on "id 5"
+test_on "protocol 802.1Q"
+test_on "priority 0"
+test_on "pipe"
+
+reset_qdisc
+ts_tc "$0" "Add vlan action push (explicit parameters)" \
+	filter add dev $DEV ingress matchall            \
+	action vlan push id 5 protocol 802.1ad priority 2
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "vlan"
+test_on "push"
+test_on "id 5"
+test_on "protocol 802.1ad"
+test_on "priority 2"
+test_on "pipe"
+
+reset_qdisc
+ts_tc "$0" "Add vlan action modify (default parameters)" \
+	filter add dev $DEV ingress matchall action vlan modify id 5
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "vlan"
+test_on "modify"
+test_on "id 5"
+test_on "protocol 802.1Q"
+test_on "priority 0"
+test_on "pipe"
+
+reset_qdisc
+ts_tc "$0" "Add vlan action modify (explicit parameters)" \
+	filter add dev $DEV ingress matchall              \
+	action vlan modify id 5 protocol 802.1ad priority 2
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "vlan"
+test_on "modify"
+test_on "id 5"
+test_on "protocol 802.1ad"
+test_on "priority 2"
+test_on "pipe"
+
+reset_qdisc
+ts_tc "$0" "Add vlan action pop_eth" \
+	filter add dev $DEV ingress matchall action vlan pop_eth
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "vlan"
+test_on "pop_eth"
+test_on "pipe"
+
+reset_qdisc
+ts_tc "$0" "Add vlan action push_eth"                  \
+	filter add dev $DEV ingress matchall           \
+	action vlan push_eth dst_mac 02:00:00:00:00:02 \
+	src_mac 02:00:00:00:00:01
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "vlan"
+test_on "push_eth"
+test_on "dst_mac 02:00:00:00:00:02"
+test_on "src_mac 02:00:00:00:00:01"
+test_on "pipe"
-- 
2.21.3

[PATCH v2 iproute2-next 2/2] m_mpls: add mac_push action

[Guillaume Nault]

Add support for the new TCA_MPLS_ACT_MAC_PUSH action (kernel commit
a45294af9e96 ("net/sched: act_mpls: Add action to push MPLS LSE before
Ethernet header")). This action let TC push an MPLS header before the
MAC header of a frame.

Example (encapsulate all outgoing frames with label 20, then add an
outer Ethernet header):
 # tc filter add dev ethX matchall \
       action mpls mac_push label 20 ttl 64 \
       action vlan push_eth dst_mac 0a:00:00:00:00:02 \
                            src_mac 0a:00:00:00:00:01

This patch also adds an alias for ETH_P_TEB, since it is useful when
decapsulating MPLS packets that contain an Ethernet frame.

With MAC_PUSH, there's no previous Ethertype to modify. However, the
"protocol" option is still needed, because the kernel uses it to set
skb->protocol. So rename can_modify_ethtype() to can_set_ethtype().

Also add a test suite for m_mpls, which covers the new action and the
pre-existing ones.

Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 lib/ll_proto.c            |  1 +
 man/man8/tc-mpls.8        | 44 +++++++++++++++++++++++--
 man/man8/tc-vlan.8        |  5 ++-
 tc/m_mpls.c               | 43 ++++++++++++++++--------
 testsuite/tests/tc/mpls.t | 69 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 145 insertions(+), 17 deletions(-)
 create mode 100755 testsuite/tests/tc/mpls.t

diff --git a/lib/ll_proto.c b/lib/ll_proto.c
index 2a0c1cb3..78179311 100644
--- a/lib/ll_proto.c
+++ b/lib/ll_proto.c
@@ -80,6 +80,7 @@ __PF(8021Q,802.1Q)
 __PF(8021AD,802.1ad)
 __PF(MPLS_UC,mpls_uc)
 __PF(MPLS_MC,mpls_mc)
+__PF(TEB,teb)
 
 { 0x8100, "802.1Q" },
 { 0x88cc, "LLDP" },
diff --git a/man/man8/tc-mpls.8 b/man/man8/tc-mpls.8
index 84ef2ef1..9e563e98 100644
--- a/man/man8/tc-mpls.8
+++ b/man/man8/tc-mpls.8
@@ -17,7 +17,7 @@ mpls - mpls manipulation module
 
 .ti -8
 .IR PUSH " := "
-.BR push " [ " protocol
+.RB "{ " push " | " mac_push " } [ " protocol
 .IR MPLS_PROTO " ]"
 .RB " [ " tc
 .IR MPLS_TC " ] "
@@ -64,7 +64,14 @@ requires no arguments and simply subtracts 1 from the MPLS header TTL field.
 Decapsulation mode. Requires the protocol of the next header.
 .TP
 .B push
-Encapsulation mode. Requires at least the
+Encapsulation mode. Adds the MPLS header between the MAC and the network
+headers. Requires at least the
+.B label
+option.
+.TP
+.B mac_push
+Encapsulation mode. Adds the MPLS header before the MAC header. Requires at
+least the
 .B label
 option.
 .TP
@@ -152,5 +159,36 @@ ip packets and output to eth1:
 .EE
 .RE
 
+Here is another example, where incoming Ethernet frames are encapsulated into
+MPLS with label 123 and TTL 64. Then, an outer Ethernet header is added and the
+resulting frame is finally sent on eth1:
+
+.RS
+.EX
+#tc qdisc add dev eth0 ingress
+#tc filter add dev eth0 ingress matchall \\
+	action mpls mac_push label 123 ttl 64 \\
+	action vlan push_eth \\
+		dst_mac 02:00:00:00:00:02 \\
+		src_mac 02:00:00:00:00:01 \\
+	action mirred egress redirect dev eth1
+.EE
+.RE
+
+The following example assumes that incoming MPLS packets with label 123
+transport Ethernet frames. The outer Ethernet and the MPLS headers are
+stripped, then the inner Ethernet frame is sent on eth1:
+
+.RS
+.EX
+#tc qdisc add dev eth0 ingress
+#tc filter add dev eth0 ingress protocol mpls_uc \\
+	flower mpls_label 123 mpls_bos 1 \\
+	action vlan pop_eth \\
+	action mpls pop protocol teb \\
+	action mirred egress redirect dev eth1
+.EE
+.RE
+
 .SH SEE ALSO
-.BR tc (8)
+.BR tc "(8), " tc-mirred "(8), " tc-vlan (8)
diff --git a/man/man8/tc-vlan.8 b/man/man8/tc-vlan.8
index 5c2808b1..264053d3 100644
--- a/man/man8/tc-vlan.8
+++ b/man/man8/tc-vlan.8
@@ -157,5 +157,8 @@ process then restarted for the plain packet:
 .EE
 .RE
 
+For an example of the
+.BR pop_eth " and " push_eth " modes, see " tc-mpls (8).
+
 .SH SEE ALSO
-.BR tc (8)
+.BR tc "(8), " tc-mpls (8)
diff --git a/tc/m_mpls.c b/tc/m_mpls.c
index 3d5d9b25..cb8019b1 100644
--- a/tc/m_mpls.c
+++ b/tc/m_mpls.c
@@ -17,6 +17,7 @@ static const char * const action_names[] = {
 	[TCA_MPLS_ACT_PUSH] = "push",
 	[TCA_MPLS_ACT_MODIFY] = "modify",
 	[TCA_MPLS_ACT_DEC_TTL] = "dec_ttl",
+	[TCA_MPLS_ACT_MAC_PUSH] = "mac_push",
 };
 
 static void explain(void)
@@ -25,9 +26,11 @@ static void explain(void)
 		"Usage: mpls pop [ protocol MPLS_PROTO ]\n"
 		"       mpls push [ protocol MPLS_PROTO ] [ label MPLS_LABEL ] [ tc MPLS_TC ]\n"
 		"                 [ ttl MPLS_TTL ] [ bos MPLS_BOS ] [CONTROL]\n"
+		"       mpls mac_push [ protocol MPLS_PROTO ] [ label MPLS_LABEL ] [ tc MPLS_TC ]\n"
+		"                     [ ttl MPLS_TTL ] [ bos MPLS_BOS ] [CONTROL]\n"
 		"       mpls modify [ label MPLS_LABEL ] [ tc MPLS_TC ] [ ttl MPLS_TTL ] [CONTROL]\n"
-		"           for pop MPLS_PROTO is next header of packet - e.g. ip or mpls_uc\n"
-		"           for push MPLS_PROTO is one of mpls_uc or mpls_mc\n"
+		"           for pop, MPLS_PROTO is next header of packet - e.g. ip or mpls_uc\n"
+		"           for push and mac_push, MPLS_PROTO is one of mpls_uc or mpls_mc\n"
 		"               with default: mpls_uc\n"
 		"       CONTROL := reclassify | pipe | drop | continue | pass |\n"
 		"                  goto chain <CHAIN_INDEX>\n");
@@ -41,12 +44,14 @@ static void usage(void)
 
 static bool can_modify_mpls_fields(unsigned int action)
 {
-	return action == TCA_MPLS_ACT_PUSH || action == TCA_MPLS_ACT_MODIFY;
+	return action == TCA_MPLS_ACT_PUSH || action == TCA_MPLS_ACT_MAC_PUSH ||
+		action == TCA_MPLS_ACT_MODIFY;
 }
 
-static bool can_modify_ethtype(unsigned int action)
+static bool can_set_ethtype(unsigned int action)
 {
-	return action == TCA_MPLS_ACT_PUSH || action == TCA_MPLS_ACT_POP;
+	return action == TCA_MPLS_ACT_PUSH || action == TCA_MPLS_ACT_MAC_PUSH ||
+		action == TCA_MPLS_ACT_POP;
 }
 
 static bool is_valid_label(__u32 label)
@@ -94,6 +99,10 @@ static int parse_mpls(struct action_util *a, int *argc_p, char ***argv_p,
 			if (check_double_action(action, *argv))
 				return -1;
 			action = TCA_MPLS_ACT_PUSH;
+		} else if (matches(*argv, "mac_push") == 0) {
+			if (check_double_action(action, *argv))
+				return -1;
+			action = TCA_MPLS_ACT_MAC_PUSH;
 		} else if (matches(*argv, "modify") == 0) {
 			if (check_double_action(action, *argv))
 				return -1;
@@ -104,31 +113,36 @@ static int parse_mpls(struct action_util *a, int *argc_p, char ***argv_p,
 			action = TCA_MPLS_ACT_DEC_TTL;
 		} else if (matches(*argv, "label") == 0) {
 			if (!can_modify_mpls_fields(action))
-				invarg("only valid for push/modify", *argv);
+				invarg("only valid for push, mac_push and modify",
+				       *argv);
 			NEXT_ARG();
 			if (get_u32(&label, *argv, 0) || !is_valid_label(label))
 				invarg("label must be <=0xFFFFF", *argv);
 		} else if (matches(*argv, "tc") == 0) {
 			if (!can_modify_mpls_fields(action))
-				invarg("only valid for push/modify", *argv);
+				invarg("only valid for push, mac_push and modify",
+				       *argv);
 			NEXT_ARG();
 			if (get_u8(&tc, *argv, 0) || (tc & ~0x7))
 				invarg("tc field is 3 bits max", *argv);
 		} else if (matches(*argv, "ttl") == 0) {
 			if (!can_modify_mpls_fields(action))
-				invarg("only valid for push/modify", *argv);
+				invarg("only valid for push, mac_push and modify",
+				       *argv);
 			NEXT_ARG();
 			if (get_u8(&ttl, *argv, 0) || !ttl)
 				invarg("ttl must be >0 and <=255", *argv);
 		} else if (matches(*argv, "bos") == 0) {
 			if (!can_modify_mpls_fields(action))
-				invarg("only valid for push/modify", *argv);
+				invarg("only valid for push, mac_push and modify",
+				       *argv);
 			NEXT_ARG();
 			if (get_u8(&bos, *argv, 0) || (bos & ~0x1))
 				invarg("bos must be 0 or 1", *argv);
 		} else if (matches(*argv, "protocol") == 0) {
-			if (!can_modify_ethtype(action))
-				invarg("only valid for push/pop", *argv);
+			if (!can_set_ethtype(action))
+				invarg("only valid for push, mac_push and pop",
+				       *argv);
 			NEXT_ARG();
 			if (ll_proto_a2n(&proto, *argv))
 				invarg("protocol is invalid", *argv);
@@ -159,10 +173,12 @@ static int parse_mpls(struct action_util *a, int *argc_p, char ***argv_p,
 	if (action == TCA_MPLS_ACT_PUSH && label == 0xffffffff)
 		missarg("label");
 
-	if (action == TCA_MPLS_ACT_PUSH && proto &&
+	if ((action == TCA_MPLS_ACT_PUSH || action == TCA_MPLS_ACT_MAC_PUSH) &&
+	    proto &&
 	    proto != htons(ETH_P_MPLS_UC) && proto != htons(ETH_P_MPLS_MC)) {
 		fprintf(stderr,
-			"invalid push protocol \"0x%04x\" - use mpls_(uc|mc)\n",
+			"invalid %spush protocol \"0x%04x\" - use mpls_(uc|mc)\n",
+			action == TCA_MPLS_ACT_MAC_PUSH ? "mac_" : "",
 			ntohs(proto));
 		return -1;
 	}
@@ -223,6 +239,7 @@ static int print_mpls(struct action_util *au, FILE *f, struct rtattr *arg)
 		}
 		break;
 	case TCA_MPLS_ACT_PUSH:
+	case TCA_MPLS_ACT_MAC_PUSH:
 		if (tb[TCA_MPLS_PROTO]) {
 			__u16 proto;
 
diff --git a/testsuite/tests/tc/mpls.t b/testsuite/tests/tc/mpls.t
new file mode 100755
index 00000000..cb25f361
--- /dev/null
+++ b/testsuite/tests/tc/mpls.t
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+. lib/generic.sh
+
+DEV="$(rand_dev)"
+ts_ip "$0" "Add $DEV dummy interface" link add dev $DEV up type dummy
+ts_tc "$0" "Add ingress qdisc" qdisc add dev $DEV ingress
+
+reset_qdisc()
+{
+	ts_tc "$0" "Remove ingress qdisc" qdisc del dev $DEV ingress
+	ts_tc "$0" "Add ingress qdisc" qdisc add dev $DEV ingress
+}
+
+ts_tc "$0" "Add mpls action pop"                              \
+	filter add dev $DEV ingress protocol mpls_uc matchall \
+	action mpls pop protocol ip
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "mpls"
+test_on "pop protocol ip pipe"
+
+reset_qdisc
+ts_tc "$0" "Add mpls action push"                        \
+	filter add dev $DEV ingress protocol ip matchall \
+	action mpls push protocol mpls_uc label 20 tc 3 bos 1 ttl 64
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "mpls"
+test_on "push"
+test_on "protocol mpls_uc"
+test_on "label 20"
+test_on "tc 3"
+test_on "bos 1"
+test_on "ttl 64"
+test_on "pipe"
+
+reset_qdisc
+ts_tc "$0" "Add mpls action mac_push"        \
+	filter add dev $DEV ingress matchall \
+	action mpls mac_push protocol mpls_uc label 20 tc 3 bos 1 ttl 64
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "mpls"
+test_on "mac_push"
+test_on "protocol mpls_uc"
+test_on "label 20"
+test_on "tc 3"
+test_on "bos 1"
+test_on "ttl 64"
+test_on "pipe"
+
+reset_qdisc
+ts_tc "$0" "Add mpls action modify"                           \
+	filter add dev $DEV ingress protocol mpls_uc matchall \
+	action mpls modify label 20 tc 3 ttl 64
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "mpls"
+test_on "modify"
+test_on "label 20"
+test_on "tc 3"
+test_on "ttl 64"
+test_on "pipe"
+
+reset_qdisc
+ts_tc "$0" "Add mpls action dec_ttl"                          \
+	filter add dev $DEV ingress protocol mpls_uc matchall \
+	action mpls dec_ttl
+ts_tc "$0" "Show ingress filters" filter show dev $DEV ingress
+test_on "mpls"
+test_on "dec_ttl"
+test_on "pipe"
-- 
2.21.3

Re: [PATCH v2 iproute2-next 0/2] tc: support for new MPLS L2 VPN actions

[David Ahern]

On 10/19/20 9:22 AM, Guillaume Nault wrote:
> This patch series adds the possibility for TC to tunnel Ethernet frames
> over MPLS.
> 
> Patch 1 allows adding or removing the Ethernet header.
> Patch 2 allows pushing an MPLS LSE before the MAC header.
> 
> By combining these actions, it becomes possible to encapsulate an
> entire Ethernet frame into MPLS, then add an outer Ethernet header
> and send the resulting frame to the next hop.
> 
> v2: trivial coding style fix (line wrap).
> 
> Guillaume Nault (2):
>   m_vlan: add pop_eth and push_eth actions
>   m_mpls: add mac_push action

applied to iproute2-next. Thanks, Guillaume.

Re: [PATCH v2 iproute2-next 1/2] m_vlan: add pop_eth and push_eth actions

[Stephen Hemminger]

On Mon, 19 Oct 2020 17:23:01 +0200
Guillaume Nault <gnault@redhat.com> wrote:

> +		} else if (matches(*argv, "pop_eth") == 0) {

Using matches allows for shorter command lines but can be make
for bad user experience if strings overlap.

For example 'p' here will match the pop_eth and not the push_eth.

Is it time to use full string compare for these options?

Re: [PATCH v2 iproute2-next 1/2] m_vlan: add pop_eth and push_eth actions

[David Ahern]

On 10/21/20 12:32 PM, Stephen Hemminger wrote:
> On Mon, 19 Oct 2020 17:23:01 +0200
> Guillaume Nault <gnault@redhat.com> wrote:
> 
>> +		} else if (matches(*argv, "pop_eth") == 0) {
> 
> Using matches allows for shorter command lines but can be make
> for bad user experience if strings overlap.
> 
> For example 'p' here will match the pop_eth and not the push_eth.
> 
> Is it time to use full string compare for these options?
> 

I thought the same, but let it go given the pervasive use of 'matches'.

I do think iproute2 needs to stop adding more uses of it and forcing
full strcmp's.

Re: [PATCH v2 iproute2-next 1/2] m_vlan: add pop_eth and push_eth actions

[Guillaume Nault]

On Wed, Oct 21, 2020 at 11:32:34AM -0700, Stephen Hemminger wrote:
> On Mon, 19 Oct 2020 17:23:01 +0200
> Guillaume Nault <gnault@redhat.com> wrote:
> 
> > +		} else if (matches(*argv, "pop_eth") == 0) {
> 
> Using matches allows for shorter command lines but can be make
> for bad user experience if strings overlap.
> 
> For example 'p' here will match the pop_eth and not the push_eth.

Well, the action names are tested in the following order:
  * pop (old action)
  * push (old action)
  * pop_eth (new action)
  * push_eth (new action)

Therefore, 'p' matches 'pop', thus retaining the original behaviour.

I realise that for m_mpls.c, I put the 'mac_push' action before
'modify', and thus changed the behaviour of 'm'. I'll send a patch to
move the 'mac_push' test.

> Is it time to use full string compare for these options?

If there's consensus that matches() should be avoided for new options,
I'll also follow up on this and replace it with strcmp(). However, that
should be a clear project-wide policy IMHO.

Re: [PATCH v2 iproute2-next 1/2] m_vlan: add pop_eth and push_eth actions

[David Ahern]

On 10/22/20 2:36 AM, Guillaume Nault wrote:
> 
>> Is it time to use full string compare for these options?
> 
> If there's consensus that matches() should be avoided for new options,
> I'll also follow up on this and replace it with strcmp(). However, that
> should be a clear project-wide policy IMHO.
> 

we can't change existing uses of 'matches'; it needs to be a policy
change going forward hence the discussion now.

Re: [PATCH v2 iproute2-next 1/2] m_vlan: add pop_eth and push_eth actions

[Guillaume Nault]

On Thu, Oct 22, 2020 at 08:11:45AM -0600, David Ahern wrote:
> On 10/22/20 2:36 AM, Guillaume Nault wrote:
> > 
> >> Is it time to use full string compare for these options?
> > 
> > If there's consensus that matches() should be avoided for new options,
> > I'll also follow up on this and replace it with strcmp(). However, that
> > should be a clear project-wide policy IMHO.
> > 
> 
> we can't change existing uses of 'matches'; it needs to be a policy
> change going forward hence the discussion now.

Yes, that's what I meant. Sorry if I wasn't clear.