diff mbox series

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

Message ID 9e3b73cd6967927fc6654cbdcd7b9e7431441c3f.1576488935.git.martin.varghese@nokia.com
State Changes Requested
Delegated to: David Miller
Headers show
Series New openvswitch MPLS actions for layer 2 tunnelling | expand

Commit Message

Martin Varghese Dec. 16, 2019, 2:03 p.m. UTC 
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.
   

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

Comments

Jakub Kicinski Dec. 17, 2019, 12:13 a.m. UTC | #1 
On Mon, 16 Dec 2019 19:33:43 +0530, Martin Varghese wrote:
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index a87b44c..b7221ad 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -673,6 +673,25 @@ 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.
> + * @l2_tun: 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.
> + *
> + * 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 */
> +	bool l2_tun;

In file included from <command-line>:32:                                        
./usr/include/linux/openvswitch.h:674:2: error: unknown type name ‘bool’        
  674 |  bool l2_tun;                                                           
      |  ^~~~                                                                   
make[3]: *** [usr/include/linux/openvswitch.hdrtest] Error 1  

> +};
> +
> +

Why the double new line? Please use checkpatch --strict.
Martin Varghese Dec. 17, 2019, 5:15 p.m. UTC | #2 
On Mon, Dec 16, 2019 at 04:13:55PM -0800, Jakub Kicinski wrote:
> On Mon, 16 Dec 2019 19:33:43 +0530, Martin Varghese wrote:
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index a87b44c..b7221ad 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -673,6 +673,25 @@ 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.
> > + * @l2_tun: 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.
> > + *
> > + * 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 */
> > +	bool l2_tun;
> 
> In file included from <command-line>:32:                                        
> ./usr/include/linux/openvswitch.h:674:2: error: unknown type name ‘bool’        
>   674 |  bool l2_tun;                                                           
>       |  ^~~~                                                                   
> make[3]: *** [usr/include/linux/openvswitch.hdrtest] Error 1  
> 

Does that mean bool cannot be used in interface header files ? but what is
the alternative u8?
> > +};
> > +
> > +
> 
> Why the double new line? Please use checkpatch --strict.

Noted

Thanks for your time.
Jakub Kicinski Dec. 17, 2019, 5:30 p.m. UTC | #3 
On Tue, 17 Dec 2019 22:45:39 +0530, Martin Varghese wrote:
> On Mon, Dec 16, 2019 at 04:13:55PM -0800, Jakub Kicinski wrote:
> > On Mon, 16 Dec 2019 19:33:43 +0530, Martin Varghese wrote:  
> > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > > index a87b44c..b7221ad 100644
> > > --- a/include/uapi/linux/openvswitch.h
> > > +++ b/include/uapi/linux/openvswitch.h
> > > @@ -673,6 +673,25 @@ 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.
> > > + * @l2_tun: 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.
> > > + *
> > > + * 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 */
> > > +	bool l2_tun;  
> > 
> > In file included from <command-line>:32:                                        
> > ./usr/include/linux/openvswitch.h:674:2: error: unknown type name ‘bool’        
> >   674 |  bool l2_tun;                                                           
> >       |  ^~~~                                                                   
> > make[3]: *** [usr/include/linux/openvswitch.hdrtest] Error 1  
> >   
> 
> Does that mean bool cannot be used in interface header files ? but what is
> the alternative u8?

I think you have a 16 bit hole there anyway due to the alignment, so
I'd declare the space as __u16 (underscores needed since this is the
user space type), and then validate in the kernel that the higher bits
are unused i.e. return -EINVAL if the field is not 0 or 1, to allow for
a future use of those bits.
diff mbox series

Patch

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index a87b44c..b7221ad 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -673,6 +673,25 @@  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.
+ * @l2_tun: 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.
+ *
+ * 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 */
+	bool l2_tun;
+};
+
+
+/**
  * 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,7 +911,8 @@  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_PUSH_MPLS: Push a new MPLS label stack entry at specified
+ * offset from start of packet.
  * 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
  * type may not be changed.
@@ -927,6 +947,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..5138473 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->l2_tun)
+				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..8d650e3 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->l2_tun) {
+				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)