[PATCH net-next v4 0/3] New openvswitch MPLS actions for layer 2 tunnelling

[PATCH net-next v4 0/3] New openvswitch MPLS actions for layer 2 tunnelling

[Martin Varghese]

From: Martin Varghese <martin.varghese@nokia.com>

The existing PUSH MPLS action inserts MPLS header between ethernet header
and the IP header. Though this behaviour is fine for L3 VPN where an IP
packet is encapsulated inside a MPLS tunnel, it does not suffice the L2
VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
encapsulate the ethernet packet.

The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start of the
packet or at the start of the l3 header depending on the value of l2 tunnel
flag in the PTAP_PUSH_MPLS arguments.

POP_MPLS action is extended to support ethertype 0x6558

OVS userspace changes -
---------------------
Encap & Decap ovs actions are extended to support MPLS packet type. The encap & decap
adds and removes MPLS header at the start of packet as depicted below.

Actions - encap(mpls(ether_type=0x8847)),encap(ethernet)

Incoming packet -> | ETH | IP | Payload |

1 Actions -  encap(mpls(ether_type=0x8847)) [Kernel action - ptap_push_mpls:0x8847]

        Outgoing packet -> | MPLS | ETH | Payload|

2 Actions - encap(ethernet) [ Kernel action - push_eth ]

        Outgoing packet -> | ETH | MPLS | ETH | Payload|

Decapsulation:

Incoming packet -> | ETH | MPLS | ETH | IP | Payload |

Actions - decap(),decap(packet_type(ns=0,type=0)

1 Actions -  decap() [Kernel action - pop_eth)

        Outgoing packet -> | MPLS | ETH | IP | Payload|

2 Actions - decap(packet_type(ns=0,type=0) [Kernel action - pop_mpls:0]

        Outgoing packet -> | ETH  | IP | Payload

Martin Varghese (3):
  net: skb_mpls_push() modified to allow MPLS header push at start of
    packet.
  net: Rephrased comments section of skb_mpls_pop()
  openvswitch: New MPLS actions for layer 2 tunnelling

 include/uapi/linux/openvswitch.h | 31 +++++++++++++++++++++++++++++++
 net/core/skbuff.c                | 10 +++++++---
 net/openvswitch/actions.c        | 30 ++++++++++++++++++++++++------
 net/openvswitch/flow_netlink.c   | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH net-next v4 1/3] net: skb_mpls_push() modified to allow MPLS header push at start of packet.

[Martin Varghese]

From: Martin Varghese <martin.varghese@nokia.com>

The existing skb_mpls_push() implementation always inserts mpls header
after the mac header. L2 VPN use cases requires MPLS header to be
inserted before the ethernet header as the ethernet packet gets tunnelled
inside MPLS header in those cases.

Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
---
Changes in v2:
    - Fixed comments section of skb_mpls_push().
    - Added skb_reset_mac_len() in skb_mpls_push(). The mac len changes
      when MPLS header in inserted at the start of the packet.

 net/core/skbuff.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 973a71f..d90c827 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5472,12 +5472,15 @@ static void skb_mod_eth_type(struct sk_buff *skb, struct ethhdr *hdr,
 }
 
 /**
- * skb_mpls_push() - push a new MPLS header after the mac header
+ * skb_mpls_push() - push a new MPLS header after mac_len bytes from start of
+ *                   the packet
  *
  * @skb: buffer
  * @mpls_lse: MPLS label stack entry to push
  * @mpls_proto: ethertype of the new MPLS header (expects 0x8847 or 0x8848)
  * @mac_len: length of the MAC header
+ * @ethernet: flag to indicate if the resulting packet after skb_mpls_push is
+ *            ethernet
  *
  * Expects skb->data at mac header.
  *
@@ -5501,7 +5504,7 @@ int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto,
 		return err;
 
 	if (!skb->inner_protocol) {
-		skb_set_inner_network_header(skb, mac_len);
+		skb_set_inner_network_header(skb, skb_network_offset(skb));
 		skb_set_inner_protocol(skb, skb->protocol);
 	}
 
@@ -5510,6 +5513,7 @@ int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto,
 		mac_len);
 	skb_reset_mac_header(skb);
 	skb_set_network_header(skb, mac_len);
+	skb_reset_mac_len(skb);
 
 	lse = mpls_hdr(skb);
 	lse->label_stack_entry = mpls_lse;
-- 
1.8.3.1

[PATCH net-next v4 2/3] net: Rephrased comments section of skb_mpls_pop()

[Martin Varghese]

From: Martin Varghese <martin.varghese@nokia.com>

Rephrased comments section of skb_mpls_pop() to align it with
comments section of skb_mpls_push().

Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d90c827..44b0894 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5533,7 +5533,7 @@ int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto,
  * @skb: buffer
  * @next_proto: ethertype of header after popped MPLS header
  * @mac_len: length of the MAC header
- * @ethernet: flag to indicate if ethernet header is present in packet
+ * @ethernet: flag to indicate if the packet is ethernet
  *
  * Expects skb->data at mac header.
  *
-- 
1.8.3.1

[PATCH net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

From: Martin Varghese <martin.varghese@nokia.com>

The existing PUSH MPLS action inserts MPLS header between ethernet header
and the IP header. Though this behaviour is fine for L3 VPN where an IP
packet is encapsulated inside a MPLS tunnel, it does not suffice the L2
VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
encapsulate the ethernet packet.

The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start of the
packet or at the start of the l3 header depending on the value of l2 tunnel
flag in the PTAP_PUSH_MPLS arguments.

POP_MPLS action is extended to support ethertype 0x6558.

Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
---
Changes in v2:
   - PTAP_POP_MPLS action removed.
   - Special handling for ethertype 0 added in PUSH_MPLS.
   - Refactored push_mpls function to cater existing push_mpls and
     ptap_push_mpls actions.
   - mac len to specify the MPLS header location added in PTAP_PUSH_MPLS
     arguments.

Changes in v3:
   - Special handling for ethertype 0 removed.
   - Added support for ether type 0x6558.
   - Removed mac len from PTAP_PUSH_MPLS argument list
   - used l2_tun flag to distinguish l2 and l3 tunnelling.
   - Extended PTAP_PUSH_MPLS handling to cater PUSH_MPLS action also.

Changes in v4:
   - Removed extra blank lines.
   - Replaced bool l2_tun with u16 tun flags in
     struct ovs_action_ptap_push_mpls.

 include/uapi/linux/openvswitch.h | 31 +++++++++++++++++++++++++++++++
 net/openvswitch/actions.c        | 30 ++++++++++++++++++++++++------
 net/openvswitch/flow_netlink.c   | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index a87b44c..d9461ce 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -673,6 +673,32 @@ struct ovs_action_push_mpls {
 };
 
 /**
+ * struct ovs_action_ptap_push_mpls - %OVS_ACTION_ATTR_PTAP_PUSH_MPLS action
+ * argument.
+ * @mpls_lse: MPLS label stack entry to push.
+ * @mpls_ethertype: Ethertype to set in the encapsulating ethernet frame.
+ * @tun_flags: MPLS tunnel attributes.
+ *
+ * The only values @mpls_ethertype should ever be given are %ETH_P_MPLS_UC and
+ * %ETH_P_MPLS_MC, indicating MPLS unicast or multicast. Other are rejected.
+ */
+struct ovs_action_ptap_push_mpls {
+	__be32 mpls_lse;
+	__be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
+	__u16 tun_flags;
+};
+
+#define OVS_MPLS_L2_TUNNEL_FLAG_MASK  (1 << 0) /* Flag to specify the place of
+						* insertion of MPLS header.
+						* When true, the MPLS header
+						* will be inserted at the start
+						* of the packet.
+						* When false, the MPLS header
+						* will be inserted at the start
+						* of the l3 header.
+						*/
+
+/**
  * struct ovs_action_push_vlan - %OVS_ACTION_ATTR_PUSH_VLAN action argument.
  * @vlan_tpid: Tag protocol identifier (TPID) to push.
  * @vlan_tci: Tag control identifier (TCI) to push.  The CFI bit must be set
@@ -892,6 +918,10 @@ struct check_pkt_len_arg {
  * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the packet length and execute a set
  * of actions if greater than the specified packet length, else execute
  * another set of actions.
+ * @OVS_ACTION_ATTR_PTAP_PUSH_MPLS: Push a new MPLS label stack entry at the
+ * start of the packet or at the start of the l3 header depending on the value
+ * of l2 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_PTAP_PUSH_MPLS
+ * argument.
  *
  * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
  * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -927,6 +957,7 @@ enum ovs_action_attr {
 	OVS_ACTION_ATTR_METER,        /* u32 meter ID. */
 	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
 	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
+	OVS_ACTION_ATTR_PTAP_PUSH_MPLS, /* struct ovs_action_ptap_push_mpls. */
 
 	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
 				       * from userspace. */
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 4c83954..24c12ad 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -161,16 +161,17 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			      const struct nlattr *attr, int len);
 
 static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
-		     const struct ovs_action_push_mpls *mpls)
+		     __be32 mpls_lse, __be16 mpls_ethertype, __u16 mac_len)
 {
 	int err;
 
-	err = skb_mpls_push(skb, mpls->mpls_lse, mpls->mpls_ethertype,
-			    skb->mac_len,
-			    ovs_key_mac_proto(key) == MAC_PROTO_ETHERNET);
+	err = skb_mpls_push(skb, mpls_lse, mpls_ethertype, mac_len, !!mac_len);
 	if (err)
 		return err;
 
+	if (!mac_len)
+		key->mac_proto = MAC_PROTO_NONE;
+
 	invalidate_flow_key(key);
 	return 0;
 }
@@ -185,6 +186,9 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 	if (err)
 		return err;
 
+	if (ethertype == htons(ETH_P_TEB))
+		key->mac_proto = MAC_PROTO_ETHERNET;
+
 	invalidate_flow_key(key);
 	return 0;
 }
@@ -1229,10 +1233,24 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			execute_hash(skb, key, a);
 			break;
 
-		case OVS_ACTION_ATTR_PUSH_MPLS:
-			err = push_mpls(skb, key, nla_data(a));
+		case OVS_ACTION_ATTR_PUSH_MPLS: {
+			struct ovs_action_push_mpls *mpls = nla_data(a);
+
+			err = push_mpls(skb, key, mpls->mpls_lse,
+					mpls->mpls_ethertype, skb->mac_len);
 			break;
+		}
+		case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
+			struct ovs_action_ptap_push_mpls *mpls = nla_data(a);
+			__u16 mac_len = 0;
+
+			if (!(mpls->tun_flags & OVS_MPLS_L2_TUNNEL_FLAG_MASK))
+				mac_len = skb->mac_len;
 
+			err = push_mpls(skb, key, mpls->mpls_lse,
+					mpls->mpls_ethertype, mac_len);
+			break;
+		}
 		case OVS_ACTION_ATTR_POP_MPLS:
 			err = pop_mpls(skb, key, nla_get_be16(a));
 			break;
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 65c2e34..85fe7df 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
 		case OVS_ACTION_ATTR_SET_MASKED:
 		case OVS_ACTION_ATTR_METER:
 		case OVS_ACTION_ATTR_CHECK_PKT_LEN:
+		case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
 		default:
 			return true;
 		}
@@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			[OVS_ACTION_ATTR_METER] = sizeof(u32),
 			[OVS_ACTION_ATTR_CLONE] = (u32)-1,
 			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
+			[OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_ptap_push_mpls),
 		};
 		const struct ovs_action_push_vlan *vlan;
 		int type = nla_type(a);
@@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 		case OVS_ACTION_ATTR_RECIRC:
 			break;
 
+		case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
+			const struct ovs_action_ptap_push_mpls *mpls = nla_data(a);
+
+			if (!eth_p_mpls(mpls->mpls_ethertype))
+				return -EINVAL;
+
+			if (!(mpls->tun_flags & OVS_MPLS_L2_TUNNEL_FLAG_MASK)) {
+				if (vlan_tci & htons(VLAN_CFI_MASK) ||
+				    (eth_type != htons(ETH_P_IP) &&
+				     eth_type != htons(ETH_P_IPV6) &&
+				     eth_type != htons(ETH_P_ARP) &&
+				     eth_type != htons(ETH_P_RARP) &&
+				     !eth_p_mpls(eth_type)))
+					return -EINVAL;
+				mpls_label_count++;
+			} else {
+				if (mac_proto != MAC_PROTO_NONE) {
+					mpls_label_count = 1;
+					mac_proto = MAC_PROTO_NONE;
+				} else {
+					mpls_label_count++;
+				}
+			}
+			eth_type = mpls->mpls_ethertype;
+			break;
+		}
+
 		case OVS_ACTION_ATTR_PUSH_MPLS: {
 			const struct ovs_action_push_mpls *mpls = nla_data(a);
 
@@ -3109,6 +3138,11 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			 * recirculation.
 			 */
 			proto = nla_get_be16(a);
+
+			if (proto == htons(ETH_P_TEB) &&
+			    mac_proto != MAC_PROTO_NONE)
+				return -EINVAL;
+
 			mpls_label_count--;
 
 			if (!eth_p_mpls(proto) || !mpls_label_count)
-- 
1.8.3.1

Re: [PATCH net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Pravin Shelar]

On Tue, Dec 17, 2019 at 10:56 PM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> From: Martin Varghese <martin.varghese@nokia.com>
>
> The existing PUSH MPLS action inserts MPLS header between ethernet header
> and the IP header. Though this behaviour is fine for L3 VPN where an IP
> packet is encapsulated inside a MPLS tunnel, it does not suffice the L2
> VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
> encapsulate the ethernet packet.
>
> The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start of the
> packet or at the start of the l3 header depending on the value of l2 tunnel
> flag in the PTAP_PUSH_MPLS arguments.
>
> POP_MPLS action is extended to support ethertype 0x6558.
>
> Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> ---
> Changes in v2:
>    - PTAP_POP_MPLS action removed.
>    - Special handling for ethertype 0 added in PUSH_MPLS.
>    - Refactored push_mpls function to cater existing push_mpls and
>      ptap_push_mpls actions.
>    - mac len to specify the MPLS header location added in PTAP_PUSH_MPLS
>      arguments.
>
> Changes in v3:
>    - Special handling for ethertype 0 removed.
>    - Added support for ether type 0x6558.
>    - Removed mac len from PTAP_PUSH_MPLS argument list
>    - used l2_tun flag to distinguish l2 and l3 tunnelling.
>    - Extended PTAP_PUSH_MPLS handling to cater PUSH_MPLS action also.
>
> Changes in v4:
>    - Removed extra blank lines.
>    - Replaced bool l2_tun with u16 tun flags in
>      struct ovs_action_ptap_push_mpls.
>
The patch looks almost ready. I have couple of comments.

>  include/uapi/linux/openvswitch.h | 31 +++++++++++++++++++++++++++++++
>  net/openvswitch/actions.c        | 30 ++++++++++++++++++++++++------
>  net/openvswitch/flow_netlink.c   | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 89 insertions(+), 6 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index a87b44c..d9461ce 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -673,6 +673,32 @@ struct ovs_action_push_mpls {
>  };
>
...
...
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 65c2e34..85fe7df 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
>                 case OVS_ACTION_ATTR_SET_MASKED:
>                 case OVS_ACTION_ATTR_METER:
>                 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
>                 default:
>                         return true;
>                 }
> @@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>                         [OVS_ACTION_ATTR_METER] = sizeof(u32),
>                         [OVS_ACTION_ATTR_CLONE] = (u32)-1,
>                         [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> +                       [OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_ptap_push_mpls),
>                 };
>                 const struct ovs_action_push_vlan *vlan;
>                 int type = nla_type(a);
> @@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>                 case OVS_ACTION_ATTR_RECIRC:
>                         break;
>
> +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
Can you change name of this action given this can handle both L2 and
L3 MPLS tunneling?

> +                       const struct ovs_action_ptap_push_mpls *mpls = nla_data(a);
> +
> +                       if (!eth_p_mpls(mpls->mpls_ethertype))
> +                               return -EINVAL;
> +
> +                       if (!(mpls->tun_flags & OVS_MPLS_L2_TUNNEL_FLAG_MASK)) {
> +                               if (vlan_tci & htons(VLAN_CFI_MASK) ||
> +                                   (eth_type != htons(ETH_P_IP) &&
> +                                    eth_type != htons(ETH_P_IPV6) &&
> +                                    eth_type != htons(ETH_P_ARP) &&
> +                                    eth_type != htons(ETH_P_RARP) &&
> +                                    !eth_p_mpls(eth_type)))
> +                                       return -EINVAL;
> +                               mpls_label_count++;
> +                       } else {
> +                               if (mac_proto != MAC_PROTO_NONE) {
It is better to check for 'MAC_PROTO_ETHERNET', rather than this negative test.

> +                                       mpls_label_count = 1;
> +                                       mac_proto = MAC_PROTO_NONE;
> +                               } else {
> +                                       mpls_label_count++;
> +                               }
We need to either disallow combination of L3 and L2 MPLS_PUSH, POP
actions in a action list or keep separate label count. Otherwise it is
impossible to validate mpls labels stack depth in POP actions.

> +                       }
> +                       eth_type = mpls->mpls_ethertype;
> +                       break;
> +               }
> +
>                 case OVS_ACTION_ATTR_PUSH_MPLS: {
>                         const struct ovs_action_push_mpls *mpls = nla_data(a);
>

Re: [PATCH net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On Wed, Dec 18, 2019 at 07:50:52PM -0800, Pravin Shelar wrote:
> On Tue, Dec 17, 2019 at 10:56 PM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > From: Martin Varghese <martin.varghese@nokia.com>
> >
> > The existing PUSH MPLS action inserts MPLS header between ethernet header
> > and the IP header. Though this behaviour is fine for L3 VPN where an IP
> > packet is encapsulated inside a MPLS tunnel, it does not suffice the L2
> > VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
> > encapsulate the ethernet packet.
> >
> > The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start of the
> > packet or at the start of the l3 header depending on the value of l2 tunnel
> > flag in the PTAP_PUSH_MPLS arguments.
> >
> > POP_MPLS action is extended to support ethertype 0x6558.
> >
> > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> > ---
> > Changes in v2:
> >    - PTAP_POP_MPLS action removed.
> >    - Special handling for ethertype 0 added in PUSH_MPLS.
> >    - Refactored push_mpls function to cater existing push_mpls and
> >      ptap_push_mpls actions.
> >    - mac len to specify the MPLS header location added in PTAP_PUSH_MPLS
> >      arguments.
> >
> > Changes in v3:
> >    - Special handling for ethertype 0 removed.
> >    - Added support for ether type 0x6558.
> >    - Removed mac len from PTAP_PUSH_MPLS argument list
> >    - used l2_tun flag to distinguish l2 and l3 tunnelling.
> >    - Extended PTAP_PUSH_MPLS handling to cater PUSH_MPLS action also.
> >
> > Changes in v4:
> >    - Removed extra blank lines.
> >    - Replaced bool l2_tun with u16 tun flags in
> >      struct ovs_action_ptap_push_mpls.
> >
> The patch looks almost ready. I have couple of comments.
> 
> >  include/uapi/linux/openvswitch.h | 31 +++++++++++++++++++++++++++++++
> >  net/openvswitch/actions.c        | 30 ++++++++++++++++++++++++------
> >  net/openvswitch/flow_netlink.c   | 34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 89 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index a87b44c..d9461ce 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -673,6 +673,32 @@ struct ovs_action_push_mpls {
> >  };
> >
> ...
> ...
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index 65c2e34..85fe7df 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> >                 case OVS_ACTION_ATTR_SET_MASKED:
> >                 case OVS_ACTION_ATTR_METER:
> >                 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
> >                 default:
> >                         return true;
> >                 }
> > @@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >                         [OVS_ACTION_ATTR_METER] = sizeof(u32),
> >                         [OVS_ACTION_ATTR_CLONE] = (u32)-1,
> >                         [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> > +                       [OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_ptap_push_mpls),
> >                 };
> >                 const struct ovs_action_push_vlan *vlan;
> >                 int type = nla_type(a);
> > @@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >                 case OVS_ACTION_ATTR_RECIRC:
> >                         break;
> >
> > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
> Can you change name of this action given this can handle both L2 and
> L3 MPLS tunneling?
> 
> > +                       const struct ovs_action_ptap_push_mpls *mpls = nla_data(a);
> > +
> > +                       if (!eth_p_mpls(mpls->mpls_ethertype))
> > +                               return -EINVAL;
> > +
> > +                       if (!(mpls->tun_flags & OVS_MPLS_L2_TUNNEL_FLAG_MASK)) {
> > +                               if (vlan_tci & htons(VLAN_CFI_MASK) ||
> > +                                   (eth_type != htons(ETH_P_IP) &&
> > +                                    eth_type != htons(ETH_P_IPV6) &&
> > +                                    eth_type != htons(ETH_P_ARP) &&
> > +                                    eth_type != htons(ETH_P_RARP) &&
> > +                                    !eth_p_mpls(eth_type)))
> > +                                       return -EINVAL;
> > +                               mpls_label_count++;
> > +                       } else {
> > +                               if (mac_proto != MAC_PROTO_NONE) {
> It is better to check for 'MAC_PROTO_ETHERNET', rather than this negative test.
> 
The idea is that if you have a l2 header you need to reset the mpls label count
Either way fine for me.Let me know 
> > +                                       mpls_label_count = 1;
> > +                                       mac_proto = MAC_PROTO_NONE;
> > +                               } else {
> > +                                       mpls_label_count++;
> > +                               }
> We need to either disallow combination of L3 and L2 MPLS_PUSH, POP
> actions in a action list or keep separate label count. Otherwise it is
> impossible to validate mpls labels stack depth in POP actions.
>

I assume it is taken in care in the above block

let us consider the different cases

1.
  Incoming Packet - ETH|IP|Payload
  Actions = push_mpls(0x1),push_mpls(0x2),ptap_push_mpls(0x03)
  Resulting packet - MPLS(3)|Eth|MPLS(2)|MPLS(1)|IP|Payload
  Total Mpls count = 1

  Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to 
  parse the inner packets

2. Incoming Packet - ETH|MPLS(1)|IP|Payload
   Actions = ptap_push_mpls(0x03)
   Resulting packet - MPLS(3)|Eth|MPLS(1)|IP|Payload
   Total Mpls count = 1

   Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to
   parse the inner packets

3. Incoming Packet - MPLS(1)|IP|Payload
   Actions = ptap_push_mpls(0x03)
   Resulting packet - MPLS(3)|MPLS(1)|IP|Payload
   Total Mpls count = 2

   Since the total MPLS acount is 2 , 2 pops are allowd

   
Is there any other case ?
 
> > +                       }
> > +                       eth_type = mpls->mpls_ethertype;
> > +                       break;
> > +               }
> > +
> >                 case OVS_ACTION_ATTR_PUSH_MPLS: {
> >                         const struct ovs_action_push_mpls *mpls = nla_data(a);
> >

Re: [PATCH net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Pravin Shelar]

On Wed, Dec 18, 2019 at 8:12 PM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> On Wed, Dec 18, 2019 at 07:50:52PM -0800, Pravin Shelar wrote:
> > On Tue, Dec 17, 2019 at 10:56 PM Martin Varghese
> > <martinvarghesenokia@gmail.com> wrote:
> > >
> > > From: Martin Varghese <martin.varghese@nokia.com>
> > >
> > > The existing PUSH MPLS action inserts MPLS header between ethernet header
> > > and the IP header. Though this behaviour is fine for L3 VPN where an IP
> > > packet is encapsulated inside a MPLS tunnel, it does not suffice the L2
> > > VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
> > > encapsulate the ethernet packet.
> > >
> > > The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start of the
> > > packet or at the start of the l3 header depending on the value of l2 tunnel
> > > flag in the PTAP_PUSH_MPLS arguments.
> > >
> > > POP_MPLS action is extended to support ethertype 0x6558.
> > >
> > > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> > > ---
> > > Changes in v2:
> > >    - PTAP_POP_MPLS action removed.
> > >    - Special handling for ethertype 0 added in PUSH_MPLS.
> > >    - Refactored push_mpls function to cater existing push_mpls and
> > >      ptap_push_mpls actions.
> > >    - mac len to specify the MPLS header location added in PTAP_PUSH_MPLS
> > >      arguments.
> > >
> > > Changes in v3:
> > >    - Special handling for ethertype 0 removed.
> > >    - Added support for ether type 0x6558.
> > >    - Removed mac len from PTAP_PUSH_MPLS argument list
> > >    - used l2_tun flag to distinguish l2 and l3 tunnelling.
> > >    - Extended PTAP_PUSH_MPLS handling to cater PUSH_MPLS action also.
> > >
> > > Changes in v4:
> > >    - Removed extra blank lines.
> > >    - Replaced bool l2_tun with u16 tun flags in
> > >      struct ovs_action_ptap_push_mpls.
> > >
> > The patch looks almost ready. I have couple of comments.
> >
> > >  include/uapi/linux/openvswitch.h | 31 +++++++++++++++++++++++++++++++
> > >  net/openvswitch/actions.c        | 30 ++++++++++++++++++++++++------
> > >  net/openvswitch/flow_netlink.c   | 34 ++++++++++++++++++++++++++++++++++
> > >  3 files changed, 89 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > > index a87b44c..d9461ce 100644
> > > --- a/include/uapi/linux/openvswitch.h
> > > +++ b/include/uapi/linux/openvswitch.h
> > > @@ -673,6 +673,32 @@ struct ovs_action_push_mpls {
> > >  };
> > >
> > ...
> > ...
> > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > > index 65c2e34..85fe7df 100644
> > > --- a/net/openvswitch/flow_netlink.c
> > > +++ b/net/openvswitch/flow_netlink.c
> > > @@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> > >                 case OVS_ACTION_ATTR_SET_MASKED:
> > >                 case OVS_ACTION_ATTR_METER:
> > >                 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> > > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
> > >                 default:
> > >                         return true;
> > >                 }
> > > @@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > >                         [OVS_ACTION_ATTR_METER] = sizeof(u32),
> > >                         [OVS_ACTION_ATTR_CLONE] = (u32)-1,
> > >                         [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> > > +                       [OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_ptap_push_mpls),
> > >                 };
> > >                 const struct ovs_action_push_vlan *vlan;
> > >                 int type = nla_type(a);
> > > @@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > >                 case OVS_ACTION_ATTR_RECIRC:
> > >                         break;
> > >
> > > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
> > Can you change name of this action given this can handle both L2 and
> > L3 MPLS tunneling?
> >
> > > +                       const struct ovs_action_ptap_push_mpls *mpls = nla_data(a);
> > > +
> > > +                       if (!eth_p_mpls(mpls->mpls_ethertype))
> > > +                               return -EINVAL;
> > > +
> > > +                       if (!(mpls->tun_flags & OVS_MPLS_L2_TUNNEL_FLAG_MASK)) {
> > > +                               if (vlan_tci & htons(VLAN_CFI_MASK) ||
> > > +                                   (eth_type != htons(ETH_P_IP) &&
> > > +                                    eth_type != htons(ETH_P_IPV6) &&
> > > +                                    eth_type != htons(ETH_P_ARP) &&
> > > +                                    eth_type != htons(ETH_P_RARP) &&
> > > +                                    !eth_p_mpls(eth_type)))
> > > +                                       return -EINVAL;
> > > +                               mpls_label_count++;
> > > +                       } else {
> > > +                               if (mac_proto != MAC_PROTO_NONE) {
> > It is better to check for 'MAC_PROTO_ETHERNET', rather than this negative test.
> >
> The idea is that if you have a l2 header you need to reset the mpls label count
> Either way fine for me.Let me know
Lets change it to  "if (mac_proto  == MAC_PROTO_ETHERNET)"

> > > +                                       mpls_label_count = 1;
> > > +                                       mac_proto = MAC_PROTO_NONE;
> > > +                               } else {
> > > +                                       mpls_label_count++;
> > > +                               }
> > We need to either disallow combination of L3 and L2 MPLS_PUSH, POP
> > actions in a action list or keep separate label count. Otherwise it is
> > impossible to validate mpls labels stack depth in POP actions.
> >
>
> I assume it is taken in care in the above block
>
> let us consider the different cases
>
> 1.
>   Incoming Packet - ETH|IP|Payload
>   Actions = push_mpls(0x1),push_mpls(0x2),ptap_push_mpls(0x03)
>   Resulting packet - MPLS(3)|Eth|MPLS(2)|MPLS(1)|IP|Payload
>   Total Mpls count = 1
>
>   Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to
>   parse the inner packets
>
> 2. Incoming Packet - ETH|MPLS(1)|IP|Payload
>    Actions = ptap_push_mpls(0x03)
>    Resulting packet - MPLS(3)|Eth|MPLS(1)|IP|Payload
>    Total Mpls count = 1
>
>    Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to
>    parse the inner packets
>
> 3. Incoming Packet - MPLS(1)|IP|Payload
>    Actions = ptap_push_mpls(0x03)
>    Resulting packet - MPLS(3)|MPLS(1)|IP|Payload
>    Total Mpls count = 2
>
>    Since the total MPLS acount is 2 , 2 pops are allowd
>
>
> Is there any other case ?
>
I was think case of action list: PUSH_MPLS_L2, PUSH_MPLS_L3, ....,
POP_MPLS_L2, POP_MPLS_L2
This action will pass the validation. It would also work fine in
datapath since POP action can detect L2 and L3 packet dynamically. But
it is inconsistent with actions intention.

Re: [PATCH net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On Thu, Dec 19, 2019 at 05:07:25PM -0800, Pravin Shelar wrote:
> On Wed, Dec 18, 2019 at 8:12 PM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > On Wed, Dec 18, 2019 at 07:50:52PM -0800, Pravin Shelar wrote:
> > > On Tue, Dec 17, 2019 at 10:56 PM Martin Varghese
> > > <martinvarghesenokia@gmail.com> wrote:
> > > >
> > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > >
> > > > The existing PUSH MPLS action inserts MPLS header between ethernet header
> > > > and the IP header. Though this behaviour is fine for L3 VPN where an IP
> > > > packet is encapsulated inside a MPLS tunnel, it does not suffice the L2
> > > > VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
> > > > encapsulate the ethernet packet.
> > > >
> > > > The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start of the
> > > > packet or at the start of the l3 header depending on the value of l2 tunnel
> > > > flag in the PTAP_PUSH_MPLS arguments.
> > > >
> > > > POP_MPLS action is extended to support ethertype 0x6558.
> > > >
> > > > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> > > > ---
> > > > Changes in v2:
> > > >    - PTAP_POP_MPLS action removed.
> > > >    - Special handling for ethertype 0 added in PUSH_MPLS.
> > > >    - Refactored push_mpls function to cater existing push_mpls and
> > > >      ptap_push_mpls actions.
> > > >    - mac len to specify the MPLS header location added in PTAP_PUSH_MPLS
> > > >      arguments.
> > > >
> > > > Changes in v3:
> > > >    - Special handling for ethertype 0 removed.
> > > >    - Added support for ether type 0x6558.
> > > >    - Removed mac len from PTAP_PUSH_MPLS argument list
> > > >    - used l2_tun flag to distinguish l2 and l3 tunnelling.
> > > >    - Extended PTAP_PUSH_MPLS handling to cater PUSH_MPLS action also.
> > > >
> > > > Changes in v4:
> > > >    - Removed extra blank lines.
> > > >    - Replaced bool l2_tun with u16 tun flags in
> > > >      struct ovs_action_ptap_push_mpls.
> > > >
> > > The patch looks almost ready. I have couple of comments.
> > >
> > > >  include/uapi/linux/openvswitch.h | 31 +++++++++++++++++++++++++++++++
> > > >  net/openvswitch/actions.c        | 30 ++++++++++++++++++++++++------
> > > >  net/openvswitch/flow_netlink.c   | 34 ++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 89 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > > > index a87b44c..d9461ce 100644
> > > > --- a/include/uapi/linux/openvswitch.h
> > > > +++ b/include/uapi/linux/openvswitch.h
> > > > @@ -673,6 +673,32 @@ struct ovs_action_push_mpls {
> > > >  };
> > > >
> > > ...
> > > ...
> > > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > > > index 65c2e34..85fe7df 100644
> > > > --- a/net/openvswitch/flow_netlink.c
> > > > +++ b/net/openvswitch/flow_netlink.c
> > > > @@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> > > >                 case OVS_ACTION_ATTR_SET_MASKED:
> > > >                 case OVS_ACTION_ATTR_METER:
> > > >                 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> > > > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
> > > >                 default:
> > > >                         return true;
> > > >                 }
> > > > @@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > > >                         [OVS_ACTION_ATTR_METER] = sizeof(u32),
> > > >                         [OVS_ACTION_ATTR_CLONE] = (u32)-1,
> > > >                         [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> > > > +                       [OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_ptap_push_mpls),
> > > >                 };
> > > >                 const struct ovs_action_push_vlan *vlan;
> > > >                 int type = nla_type(a);
> > > > @@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > > >                 case OVS_ACTION_ATTR_RECIRC:
> > > >                         break;
> > > >
> > > > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
> > > Can you change name of this action given this can handle both L2 and
> > > L3 MPLS tunneling?
> > >
> > > > +                       const struct ovs_action_ptap_push_mpls *mpls = nla_data(a);
> > > > +
> > > > +                       if (!eth_p_mpls(mpls->mpls_ethertype))
> > > > +                               return -EINVAL;
> > > > +
> > > > +                       if (!(mpls->tun_flags & OVS_MPLS_L2_TUNNEL_FLAG_MASK)) {
> > > > +                               if (vlan_tci & htons(VLAN_CFI_MASK) ||
> > > > +                                   (eth_type != htons(ETH_P_IP) &&
> > > > +                                    eth_type != htons(ETH_P_IPV6) &&
> > > > +                                    eth_type != htons(ETH_P_ARP) &&
> > > > +                                    eth_type != htons(ETH_P_RARP) &&
> > > > +                                    !eth_p_mpls(eth_type)))
> > > > +                                       return -EINVAL;
> > > > +                               mpls_label_count++;
> > > > +                       } else {
> > > > +                               if (mac_proto != MAC_PROTO_NONE) {
> > > It is better to check for 'MAC_PROTO_ETHERNET', rather than this negative test.
> > >
> > The idea is that if you have a l2 header you need to reset the mpls label count
> > Either way fine for me.Let me know
> Lets change it to  "if (mac_proto  == MAC_PROTO_ETHERNET)"
> 
Yes, but it will not work for a hypothetical case where l2
is no ethernet.
> > > > +                                       mpls_label_count = 1;
> > > > +                                       mac_proto = MAC_PROTO_NONE;
> > > > +                               } else {
> > > > +                                       mpls_label_count++;
> > > > +                               }
> > > We need to either disallow combination of L3 and L2 MPLS_PUSH, POP
> > > actions in a action list or keep separate label count. Otherwise it is
> > > impossible to validate mpls labels stack depth in POP actions.
> > >
> >
> > I assume it is taken in care in the above block
> >
> > let us consider the different cases
> >
> > 1.
> >   Incoming Packet - ETH|IP|Payload
> >   Actions = push_mpls(0x1),push_mpls(0x2),ptap_push_mpls(0x03)
> >   Resulting packet - MPLS(3)|Eth|MPLS(2)|MPLS(1)|IP|Payload
> >   Total Mpls count = 1
> >
> >   Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to
> >   parse the inner packets
> >
> > 2. Incoming Packet - ETH|MPLS(1)|IP|Payload
> >    Actions = ptap_push_mpls(0x03)
> >    Resulting packet - MPLS(3)|Eth|MPLS(1)|IP|Payload
> >    Total Mpls count = 1
> >
> >    Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to
> >    parse the inner packets
> >
> > 3. Incoming Packet - MPLS(1)|IP|Payload
> >    Actions = ptap_push_mpls(0x03)
> >    Resulting packet - MPLS(3)|MPLS(1)|IP|Payload
> >    Total Mpls count = 2
> >
> >    Since the total MPLS acount is 2 , 2 pops are allowd
> >
> >
> > Is there any other case ?
> >
> I was think case of action list: PUSH_MPLS_L2, PUSH_MPLS_L3, ....,
> POP_MPLS_L2, POP_MPLS_L2
> This action will pass the validation. It would also work fine in
> datapath since POP action can detect L2 and L3 packet dynamically. But
> it is inconsistent with actions intention.

I couldnt get the concern correctly.
THere is no POPMPLS l2.There is only one type of MPLS POP
the pop MPLS always removes MPLS header after mac header

In the case of POP_MPLS:0x6558 the ethernet header should not be
present as we dont support ethernet in ethernet.We have the validation
in flow_netlink.c
So for the POP_MPLS:0x6558 to work ,it should be preceeded by a pop_eth
if is a ethernet packet.

Considering the action above.
Incomming packet - ETH|IP|Payload
1 Actions - push_mpls_l2
outgoing packet -  l2 MPLS label|eth|IP - ( Packet is l3 now)
2 Actions  - Push_mpls_l2
outgoing packet  | L3 MPLS Label| l2 MPLS label|eth|IP.

2 actions - POP_MPLS,POP_MPLS
outgoing packet - ETH |IP|Payload  (Packet is l2 now)

Re: [PATCH net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On 12/20/19, Martin Varghese <martinvarghesenokia@gmail.com> wrote:
> On Thu, Dec 19, 2019 at 05:07:25PM -0800, Pravin Shelar wrote:
>> On Wed, Dec 18, 2019 at 8:12 PM Martin Varghese
>> <martinvarghesenokia@gmail.com> wrote:
>> >
>> > On Wed, Dec 18, 2019 at 07:50:52PM -0800, Pravin Shelar wrote:
>> > > On Tue, Dec 17, 2019 at 10:56 PM Martin Varghese
>> > > <martinvarghesenokia@gmail.com> wrote:
>> > > >
>> > > > From: Martin Varghese <martin.varghese@nokia.com>
>> > > >
>> > > > The existing PUSH MPLS action inserts MPLS header between ethernet
>> > > > header
>> > > > and the IP header. Though this behaviour is fine for L3 VPN where an
>> > > > IP
>> > > > packet is encapsulated inside a MPLS tunnel, it does not suffice the
>> > > > L2
>> > > > VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
>> > > > encapsulate the ethernet packet.
>> > > >
>> > > > The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start
>> > > > of the
>> > > > packet or at the start of the l3 header depending on the value of l2
>> > > > tunnel
>> > > > flag in the PTAP_PUSH_MPLS arguments.
>> > > >
>> > > > POP_MPLS action is extended to support ethertype 0x6558.
>> > > >
>> > > > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
>> > > > ---
>> > > > Changes in v2:
>> > > >    - PTAP_POP_MPLS action removed.
>> > > >    - Special handling for ethertype 0 added in PUSH_MPLS.
>> > > >    - Refactored push_mpls function to cater existing push_mpls and
>> > > >      ptap_push_mpls actions.
>> > > >    - mac len to specify the MPLS header location added in
>> > > > PTAP_PUSH_MPLS
>> > > >      arguments.
>> > > >
>> > > > Changes in v3:
>> > > >    - Special handling for ethertype 0 removed.
>> > > >    - Added support for ether type 0x6558.
>> > > >    - Removed mac len from PTAP_PUSH_MPLS argument list
>> > > >    - used l2_tun flag to distinguish l2 and l3 tunnelling.
>> > > >    - Extended PTAP_PUSH_MPLS handling to cater PUSH_MPLS action
>> > > > also.
>> > > >
>> > > > Changes in v4:
>> > > >    - Removed extra blank lines.
>> > > >    - Replaced bool l2_tun with u16 tun flags in
>> > > >      struct ovs_action_ptap_push_mpls.
>> > > >
>> > > The patch looks almost ready. I have couple of comments.
>> > >
>> > > >  include/uapi/linux/openvswitch.h | 31
>> > > > +++++++++++++++++++++++++++++++
>> > > >  net/openvswitch/actions.c        | 30
>> > > > ++++++++++++++++++++++++------
>> > > >  net/openvswitch/flow_netlink.c   | 34
>> > > > ++++++++++++++++++++++++++++++++++
>> > > >  3 files changed, 89 insertions(+), 6 deletions(-)
>> > > >
>> > > > diff --git a/include/uapi/linux/openvswitch.h
>> > > > b/include/uapi/linux/openvswitch.h
>> > > > index a87b44c..d9461ce 100644
>> > > > --- a/include/uapi/linux/openvswitch.h
>> > > > +++ b/include/uapi/linux/openvswitch.h
>> > > > @@ -673,6 +673,32 @@ struct ovs_action_push_mpls {
>> > > >  };
>> > > >
>> > > ...
>> > > ...
>> > > > diff --git a/net/openvswitch/flow_netlink.c
>> > > > b/net/openvswitch/flow_netlink.c
>> > > > index 65c2e34..85fe7df 100644
>> > > > --- a/net/openvswitch/flow_netlink.c
>> > > > +++ b/net/openvswitch/flow_netlink.c
>> > > > @@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct
>> > > > nlattr *actions)
>> > > >                 case OVS_ACTION_ATTR_SET_MASKED:
>> > > >                 case OVS_ACTION_ATTR_METER:
>> > > >                 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>> > > > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
>> > > >                 default:
>> > > >                         return true;
>> > > >                 }
>> > > > @@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net
>> > > > *net, const struct nlattr *attr,
>> > > >                         [OVS_ACTION_ATTR_METER] = sizeof(u32),
>> > > >                         [OVS_ACTION_ATTR_CLONE] = (u32)-1,
>> > > >                         [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
>> > > > +                       [OVS_ACTION_ATTR_PTAP_PUSH_MPLS] =
>> > > > sizeof(struct ovs_action_ptap_push_mpls),
>> > > >                 };
>> > > >                 const struct ovs_action_push_vlan *vlan;
>> > > >                 int type = nla_type(a);
>> > > > @@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net
>> > > > *net, const struct nlattr *attr,
>> > > >                 case OVS_ACTION_ATTR_RECIRC:
>> > > >                         break;
>> > > >
>> > > > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
>> > > Can you change name of this action given this can handle both L2 and
>> > > L3 MPLS tunneling?
>> > >
>> > > > +                       const struct ovs_action_ptap_push_mpls *mpls
>> > > > = nla_data(a);
>> > > > +
>> > > > +                       if (!eth_p_mpls(mpls->mpls_ethertype))
>> > > > +                               return -EINVAL;
>> > > > +
>> > > > +                       if (!(mpls->tun_flags &
>> > > > OVS_MPLS_L2_TUNNEL_FLAG_MASK)) {
>> > > > +                               if (vlan_tci & htons(VLAN_CFI_MASK)
>> > > > ||
>> > > > +                                   (eth_type != htons(ETH_P_IP) &&
>> > > > +                                    eth_type != htons(ETH_P_IPV6)
>> > > > &&
>> > > > +                                    eth_type != htons(ETH_P_ARP)
>> > > > &&
>> > > > +                                    eth_type != htons(ETH_P_RARP)
>> > > > &&
>> > > > +                                    !eth_p_mpls(eth_type)))
>> > > > +                                       return -EINVAL;
>> > > > +                               mpls_label_count++;
>> > > > +                       } else {
>> > > > +                               if (mac_proto != MAC_PROTO_NONE) {
>> > > It is better to check for 'MAC_PROTO_ETHERNET', rather than this
>> > > negative test.
>> > >
>> > The idea is that if you have a l2 header you need to reset the mpls
>> > label count
>> > Either way fine for me.Let me know
>> Lets change it to  "if (mac_proto  == MAC_PROTO_ETHERNET)"
>>
> Yes, but it will not work for a hypothetical case where l2
> is no ethernet.
>> > > > +                                       mpls_label_count = 1;
>> > > > +                                       mac_proto = MAC_PROTO_NONE;
>> > > > +                               } else {
>> > > > +                                       mpls_label_count++;
>> > > > +                               }
>> > > We need to either disallow combination of L3 and L2 MPLS_PUSH, POP
>> > > actions in a action list or keep separate label count. Otherwise it
>> > > is
>> > > impossible to validate mpls labels stack depth in POP actions.
>> > >
>> >
>> > I assume it is taken in care in the above block
>> >
>> > let us consider the different cases
>> >
>> > 1.
>> >   Incoming Packet - ETH|IP|Payload
>> >   Actions = push_mpls(0x1),push_mpls(0x2),ptap_push_mpls(0x03)
>> >   Resulting packet - MPLS(3)|Eth|MPLS(2)|MPLS(1)|IP|Payload
>> >   Total Mpls count = 1
>> >
>> >   Since the total MPLS count = 1,ony one POP will be allowed and a
>> > recirc is need to
>> >   parse the inner packets
>> >
>> > 2. Incoming Packet - ETH|MPLS(1)|IP|Payload
>> >    Actions = ptap_push_mpls(0x03)
>> >    Resulting packet - MPLS(3)|Eth|MPLS(1)|IP|Payload
>> >    Total Mpls count = 1
>> >
>> >    Since the total MPLS count = 1,ony one POP will be allowed and a
>> > recirc is need to
>> >    parse the inner packets
>> >
>> > 3. Incoming Packet - MPLS(1)|IP|Payload
>> >    Actions = ptap_push_mpls(0x03)
>> >    Resulting packet - MPLS(3)|MPLS(1)|IP|Payload
>> >    Total Mpls count = 2
>> >
>> >    Since the total MPLS acount is 2 , 2 pops are allowd
>> >
>> >
>> > Is there any other case ?
>> >
>> I was think case of action list: PUSH_MPLS_L2, PUSH_MPLS_L3, ....,
>> POP_MPLS_L2, POP_MPLS_L2
>> This action will pass the validation. It would also work fine in
>> datapath since POP action can detect L2 and L3 packet dynamically. But
>> it is inconsistent with actions intention.
>
> I couldnt get the concern correctly.
> THere is no POPMPLS l2.There is only one type of MPLS POP
> the pop MPLS always removes MPLS header after mac header
>
> In the case of POP_MPLS:0x6558 the ethernet header should not be
> present as we dont support ethernet in ethernet.We have the validation
> in flow_netlink.c
> So for the POP_MPLS:0x6558 to work ,it should be preceeded by a pop_eth
> if is a ethernet packet.
>
> Considering the action above.
> Incomming packet - ETH|IP|Payload
> 1 Actions - push_mpls_l2
> outgoing packet -  l2 MPLS label|eth|IP - ( Packet is l3 now)
> 2 Actions  - Push_mpls_l2
> outgoing packet  | L3 MPLS Label| l2 MPLS label|eth|IP.
>
> 2 actions - POP_MPLS,POP_MPLS
> outgoing packet - ETH |IP|Payload  (Packet is l2 now)
>
>

The point here is there is no one to one mapping between
l2_tunnel_flag in push_mpls to
l2 VPN
As decsribed in the comments ,if the flag is set it inserts MPLS
header at the start of the packet.Hence if the packet is not having a
ethernet header start of the packet is start of l3 header as :well.

In that sense l2_tunnel_flag is bit ambiguous.
The better flag is l3_tunnel_flag.

The new action always insert MPLS header at the start of packet always
if not told otherwise
with l3_tunnel_flag.
I think what we have to keep in mind is the the new action has no
direct mapping to l3 or l2 VPN.It just says where to insert the MPLS
header.Similary for pop_mpls.0x6558  handling is just an extention to
existing action

Re: [PATCH net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Pravin Shelar]

On Thu, Dec 19, 2019 at 7:04 PM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> On Thu, Dec 19, 2019 at 05:07:25PM -0800, Pravin Shelar wrote:
> > On Wed, Dec 18, 2019 at 8:12 PM Martin Varghese
> > <martinvarghesenokia@gmail.com> wrote:
> > >
> > > On Wed, Dec 18, 2019 at 07:50:52PM -0800, Pravin Shelar wrote:
> > > > On Tue, Dec 17, 2019 at 10:56 PM Martin Varghese
> > > > <martinvarghesenokia@gmail.com> wrote:
> > > > >
> > > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > > >
> > > > > The existing PUSH MPLS action inserts MPLS header between ethernet header
> > > > > and the IP header. Though this behaviour is fine for L3 VPN where an IP
> > > > > packet is encapsulated inside a MPLS tunnel, it does not suffice the L2
> > > > > VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
> > > > > encapsulate the ethernet packet.
> > > > >
> > > > > The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start of the
> > > > > packet or at the start of the l3 header depending on the value of l2 tunnel
> > > > > flag in the PTAP_PUSH_MPLS arguments.
> > > > >
> > > > > POP_MPLS action is extended to support ethertype 0x6558.
> > > > >
> > > > > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> > > > > ---
> > > > > Changes in v2:
> > > > >    - PTAP_POP_MPLS action removed.
> > > > >    - Special handling for ethertype 0 added in PUSH_MPLS.
> > > > >    - Refactored push_mpls function to cater existing push_mpls and
> > > > >      ptap_push_mpls actions.
> > > > >    - mac len to specify the MPLS header location added in PTAP_PUSH_MPLS
> > > > >      arguments.
> > > > >
> > > > > Changes in v3:
> > > > >    - Special handling for ethertype 0 removed.
> > > > >    - Added support for ether type 0x6558.
> > > > >    - Removed mac len from PTAP_PUSH_MPLS argument list
> > > > >    - used l2_tun flag to distinguish l2 and l3 tunnelling.
> > > > >    - Extended PTAP_PUSH_MPLS handling to cater PUSH_MPLS action also.
> > > > >
> > > > > Changes in v4:
> > > > >    - Removed extra blank lines.
> > > > >    - Replaced bool l2_tun with u16 tun flags in
> > > > >      struct ovs_action_ptap_push_mpls.
> > > > >
> > > > The patch looks almost ready. I have couple of comments.
> > > >
> > > > >  include/uapi/linux/openvswitch.h | 31 +++++++++++++++++++++++++++++++
> > > > >  net/openvswitch/actions.c        | 30 ++++++++++++++++++++++++------
> > > > >  net/openvswitch/flow_netlink.c   | 34 ++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 89 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > > > > index a87b44c..d9461ce 100644
> > > > > --- a/include/uapi/linux/openvswitch.h
> > > > > +++ b/include/uapi/linux/openvswitch.h
> > > > > @@ -673,6 +673,32 @@ struct ovs_action_push_mpls {
> > > > >  };
> > > > >
> > > > ...
> > > > ...
> > > > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > > > > index 65c2e34..85fe7df 100644
> > > > > --- a/net/openvswitch/flow_netlink.c
> > > > > +++ b/net/openvswitch/flow_netlink.c
> > > > > @@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> > > > >                 case OVS_ACTION_ATTR_SET_MASKED:
> > > > >                 case OVS_ACTION_ATTR_METER:
> > > > >                 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> > > > > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
> > > > >                 default:
> > > > >                         return true;
> > > > >                 }
> > > > > @@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > > > >                         [OVS_ACTION_ATTR_METER] = sizeof(u32),
> > > > >                         [OVS_ACTION_ATTR_CLONE] = (u32)-1,
> > > > >                         [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> > > > > +                       [OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_ptap_push_mpls),
> > > > >                 };
> > > > >                 const struct ovs_action_push_vlan *vlan;
> > > > >                 int type = nla_type(a);
> > > > > @@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > > > >                 case OVS_ACTION_ATTR_RECIRC:
> > > > >                         break;
> > > > >
> > > > > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
> > > > Can you change name of this action given this can handle both L2 and
> > > > L3 MPLS tunneling?
> > > >
> > > > > +                       const struct ovs_action_ptap_push_mpls *mpls = nla_data(a);
> > > > > +
> > > > > +                       if (!eth_p_mpls(mpls->mpls_ethertype))
> > > > > +                               return -EINVAL;
> > > > > +
> > > > > +                       if (!(mpls->tun_flags & OVS_MPLS_L2_TUNNEL_FLAG_MASK)) {
> > > > > +                               if (vlan_tci & htons(VLAN_CFI_MASK) ||
> > > > > +                                   (eth_type != htons(ETH_P_IP) &&
> > > > > +                                    eth_type != htons(ETH_P_IPV6) &&
> > > > > +                                    eth_type != htons(ETH_P_ARP) &&
> > > > > +                                    eth_type != htons(ETH_P_RARP) &&
> > > > > +                                    !eth_p_mpls(eth_type)))
> > > > > +                                       return -EINVAL;
> > > > > +                               mpls_label_count++;
> > > > > +                       } else {
> > > > > +                               if (mac_proto != MAC_PROTO_NONE) {
> > > > It is better to check for 'MAC_PROTO_ETHERNET', rather than this negative test.
> > > >
> > > The idea is that if you have a l2 header you need to reset the mpls label count
> > > Either way fine for me.Let me know
> > Lets change it to  "if (mac_proto  == MAC_PROTO_ETHERNET)"
> >
> Yes, but it will not work for a hypothetical case where l2
> is no ethernet.
At this point it would be clear if you just check with MAC_PROTO_ETHERNET.

> > > > > +                                       mpls_label_count = 1;
> > > > > +                                       mac_proto = MAC_PROTO_NONE;
> > > > > +                               } else {
> > > > > +                                       mpls_label_count++;
> > > > > +                               }
> > > > We need to either disallow combination of L3 and L2 MPLS_PUSH, POP
> > > > actions in a action list or keep separate label count. Otherwise it is
> > > > impossible to validate mpls labels stack depth in POP actions.
> > > >
> > >
> > > I assume it is taken in care in the above block
> > >
> > > let us consider the different cases
> > >
> > > 1.
> > >   Incoming Packet - ETH|IP|Payload
> > >   Actions = push_mpls(0x1),push_mpls(0x2),ptap_push_mpls(0x03)
> > >   Resulting packet - MPLS(3)|Eth|MPLS(2)|MPLS(1)|IP|Payload
> > >   Total Mpls count = 1
> > >
> > >   Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to
> > >   parse the inner packets
> > >
> > > 2. Incoming Packet - ETH|MPLS(1)|IP|Payload
> > >    Actions = ptap_push_mpls(0x03)
> > >    Resulting packet - MPLS(3)|Eth|MPLS(1)|IP|Payload
> > >    Total Mpls count = 1
> > >
> > >    Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to
> > >    parse the inner packets
> > >
> > > 3. Incoming Packet - MPLS(1)|IP|Payload
> > >    Actions = ptap_push_mpls(0x03)
> > >    Resulting packet - MPLS(3)|MPLS(1)|IP|Payload
> > >    Total Mpls count = 2
> > >
> > >    Since the total MPLS acount is 2 , 2 pops are allowd
> > >
> > >
> > > Is there any other case ?
> > >
> > I was think case of action list: PUSH_MPLS_L2, PUSH_MPLS_L3, ....,
> > POP_MPLS_L2, POP_MPLS_L2
> > This action will pass the validation. It would also work fine in
> > datapath since POP action can detect L2 and L3 packet dynamically. But
> > it is inconsistent with actions intention.
>
> I couldnt get the concern correctly.

My concern is more about separating L3 MPLS vs PTAP cases.
I guess its not that big deal. so I am fine with current validation.

Thanks.
> THere is no POPMPLS l2.There is only one type of MPLS POP
> the pop MPLS always removes MPLS header after mac header
>
> In the case of POP_MPLS:0x6558 the ethernet header should not be
> present as we dont support ethernet in ethernet.We have the validation
> in flow_netlink.c
> So for the POP_MPLS:0x6558 to work ,it should be preceeded by a pop_eth
> if is a ethernet packet.
>
> Considering the action above.
> Incomming packet - ETH|IP|Payload
> 1 Actions - push_mpls_l2
> outgoing packet -  l2 MPLS label|eth|IP - ( Packet is l3 now)
> 2 Actions  - Push_mpls_l2
> outgoing packet  | L3 MPLS Label| l2 MPLS label|eth|IP.
>
> 2 actions - POP_MPLS,POP_MPLS
> outgoing packet - ETH |IP|Payload  (Packet is l2 now)
>
>
>
>
>
>
>

Re: [PATCH net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On Wed, Dec 18, 2019 at 07:50:52PM -0800, Pravin Shelar wrote:
> On Tue, Dec 17, 2019 at 10:56 PM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > From: Martin Varghese <martin.varghese@nokia.com>
> >
> > The existing PUSH MPLS action inserts MPLS header between ethernet header
> > and the IP header. Though this behaviour is fine for L3 VPN where an IP
> > packet is encapsulated inside a MPLS tunnel, it does not suffice the L2
> > VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
> > encapsulate the ethernet packet.
> >
> > The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start of the
> > packet or at the start of the l3 header depending on the value of l2 tunnel
> > flag in the PTAP_PUSH_MPLS arguments.
> >
> > POP_MPLS action is extended to support ethertype 0x6558.
> >
> > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> > ---
> > Changes in v2:
> >    - PTAP_POP_MPLS action removed.
> >    - Special handling for ethertype 0 added in PUSH_MPLS.
> >    - Refactored push_mpls function to cater existing push_mpls and
> >      ptap_push_mpls actions.
> >    - mac len to specify the MPLS header location added in PTAP_PUSH_MPLS
> >      arguments.
> >
> > Changes in v3:
> >    - Special handling for ethertype 0 removed.
> >    - Added support for ether type 0x6558.
> >    - Removed mac len from PTAP_PUSH_MPLS argument list
> >    - used l2_tun flag to distinguish l2 and l3 tunnelling.
> >    - Extended PTAP_PUSH_MPLS handling to cater PUSH_MPLS action also.
> >
> > Changes in v4:
> >    - Removed extra blank lines.
> >    - Replaced bool l2_tun with u16 tun flags in
> >      struct ovs_action_ptap_push_mpls.
> >
> The patch looks almost ready. I have couple of comments.
> 
> >  include/uapi/linux/openvswitch.h | 31 +++++++++++++++++++++++++++++++
> >  net/openvswitch/actions.c        | 30 ++++++++++++++++++++++++------
> >  net/openvswitch/flow_netlink.c   | 34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 89 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index a87b44c..d9461ce 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -673,6 +673,32 @@ struct ovs_action_push_mpls {
> >  };
> >
> ...
> ...
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index 65c2e34..85fe7df 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> >                 case OVS_ACTION_ATTR_SET_MASKED:
> >                 case OVS_ACTION_ATTR_METER:
> >                 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
> >                 default:
> >                         return true;
> >                 }
> > @@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >                         [OVS_ACTION_ATTR_METER] = sizeof(u32),
> >                         [OVS_ACTION_ATTR_CLONE] = (u32)-1,
> >                         [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> > +                       [OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_ptap_push_mpls),
> >                 };
> >                 const struct ovs_action_push_vlan *vlan;
> >                 int type = nla_type(a);
> > @@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >                 case OVS_ACTION_ATTR_RECIRC:
> >                         break;
> >
> > +               case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
> Can you change name of this action given this can handle both L2 and
> L3 MPLS tunneling?
>
ADD_MPLS ?
PUSH_INSERT_MPLS ?
anything else?

I prefer ADD_MPLS  
> > +                       const struct ovs_action_ptap_push_mpls *mpls = nla_data(a);
> > +
> > +                       if (!eth_p_mpls(mpls->mpls_ethertype))
> > +                               return -EINVAL;
> > +
> > +                       if (!(mpls->tun_flags & OVS_MPLS_L2_TUNNEL_FLAG_MASK)) {
> > +                               if (vlan_tci & htons(VLAN_CFI_MASK) ||
> > +                                   (eth_type != htons(ETH_P_IP) &&
> > +                                    eth_type != htons(ETH_P_IPV6) &&
> > +                                    eth_type != htons(ETH_P_ARP) &&
> > +                                    eth_type != htons(ETH_P_RARP) &&
> > +                                    !eth_p_mpls(eth_type)))
> > +                                       return -EINVAL;
> > +                               mpls_label_count++;
> > +                       } else {
> > +                               if (mac_proto != MAC_PROTO_NONE) {
> It is better to check for 'MAC_PROTO_ETHERNET', rather than this negative test.
> 
> > +                                       mpls_label_count = 1;
> > +                                       mac_proto = MAC_PROTO_NONE;
> > +                               } else {
> > +                                       mpls_label_count++;
> > +                               }
> We need to either disallow combination of L3 and L2 MPLS_PUSH, POP
> actions in a action list or keep separate label count. Otherwise it is
> impossible to validate mpls labels stack depth in POP actions.
> 
> > +                       }
> > +                       eth_type = mpls->mpls_ethertype;
> > +                       break;
> > +               }
> > +
> >                 case OVS_ACTION_ATTR_PUSH_MPLS: {
> >                         const struct ovs_action_push_mpls *mpls = nla_data(a);
> >