Re: [PATCH net-next v12 7/9] openvswitch: add Ethernet push and pop actions

[Jiri Benc]

On Thu, 20 Oct 2016 21:22:21 -0700, Pravin Shelar wrote:
> The eth pop substracts ETH_HLEN but here the length is set. I think it
> should be consistent with respect to eth-pop.

Agreed. Will use skb_reset_mac_len in both, that better reflects what's
going on.

Thanks,

 Jiri

Re: [PATCH net-next v12 7/9] openvswitch: add Ethernet push and pop actions

[Pravin Shelar]

On Mon, Oct 17, 2016 at 6:02 AM, Jiri Benc  wrote:
> It's not allowed to push Ethernet header in front of another Ethernet
> header.
>
> It's not allowed to pop Ethernet header if there's a vlan tag. This
> preserves the invariant that L3 packet never has a vlan tag.
>
> Based on previous versions by Lorand Jakab and Simon Horman.
>
> Signed-off-by: Lorand Jakab 
> Signed-off-by: Simon Horman 
> Signed-off-by: Jiri Benc 
> ---
>  include/uapi/linux/openvswitch.h | 15 
>  net/openvswitch/actions.c| 49 
> 
>  net/openvswitch/flow_netlink.c   | 18 +++
>  3 files changed, 82 insertions(+)
>
...
...
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 064cbcb7b0c5..a63572fb878e 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -317,6 +317,47 @@ static int set_eth_addr(struct sk_buff *skb, struct 
> sw_flow_key *flow_key,
> return 0;
>  }
>
> +/* pop_eth does not support VLAN packets as this action is never called
> + * for them.
> + */
> +static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> +   skb_pull_rcsum(skb, ETH_HLEN);
> +   skb_reset_mac_header(skb);
> +   skb->mac_len -= ETH_HLEN;
> +
> +   /* safe right before invalidate_flow_key */
> +   key->mac_proto = MAC_PROTO_NONE;
> +   invalidate_flow_key(key);
> +   return 0;
> +}
> +
> +static int push_eth(struct sk_buff *skb, struct sw_flow_key *key,
> +   const struct ovs_action_push_eth *ethh)
> +{
> +   struct ethhdr *hdr;
> +
> +   /* Add the new Ethernet header */
> +   if (skb_cow_head(skb, ETH_HLEN) < 0)
> +   return -ENOMEM;
> +
> +   skb_push(skb, ETH_HLEN);
> +   skb_reset_mac_header(skb);
> +   skb->mac_len = ETH_HLEN;
> +

The eth pop substracts ETH_HLEN but here the length is set. I think it
should be consistent with respect to eth-pop.

[PATCH net-next v12 7/9] openvswitch: add Ethernet push and pop actions

[Jiri Benc]

It's not allowed to push Ethernet header in front of another Ethernet
header.

It's not allowed to pop Ethernet header if there's a vlan tag. This
preserves the invariant that L3 packet never has a vlan tag.

Based on previous versions by Lorand Jakab and Simon Horman.

Signed-off-by: Lorand Jakab 
Signed-off-by: Simon Horman 
Signed-off-by: Jiri Benc 
---
 include/uapi/linux/openvswitch.h | 15 
 net/openvswitch/actions.c| 49 
 net/openvswitch/flow_netlink.c   | 18 +++
 3 files changed, 82 insertions(+)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 59ed3992c760..375d812fea36 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -705,6 +705,15 @@ enum ovs_nat_attr {
 
 #define OVS_NAT_ATTR_MAX (__OVS_NAT_ATTR_MAX - 1)
 
+/*
+ * struct ovs_action_push_eth - %OVS_ACTION_ATTR_PUSH_ETH action argument.
+ * @addresses: Source and destination MAC addresses.
+ * @eth_type: Ethernet type
+ */
+struct ovs_action_push_eth {
+   struct ovs_key_ethernet addresses;
+};
+
 /**
  * enum ovs_action_attr - Action types.
  *
@@ -738,6 +747,10 @@ enum ovs_nat_attr {
  * is no MPLS label stack, as determined by ethertype, no action is taken.
  * @OVS_ACTION_ATTR_CT: Track the connection. Populate the conntrack-related
  * entries in the flow key.
+ * @OVS_ACTION_ATTR_PUSH_ETH: Push a new outermost Ethernet header onto the
+ * packet.
+ * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the
+ * 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
@@ -765,6 +778,8 @@ enum ovs_action_attr {
   * bits. */
OVS_ACTION_ATTR_CT,   /* Nested OVS_CT_ATTR_* . */
OVS_ACTION_ATTR_TRUNC,/* u32 struct ovs_action_trunc. */
+   OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
+   OVS_ACTION_ATTR_POP_ETH,  /* No argument. */
 
__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 064cbcb7b0c5..a63572fb878e 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -317,6 +317,47 @@ static int set_eth_addr(struct sk_buff *skb, struct 
sw_flow_key *flow_key,
return 0;
 }
 
+/* pop_eth does not support VLAN packets as this action is never called
+ * for them.
+ */
+static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key)
+{
+   skb_pull_rcsum(skb, ETH_HLEN);
+   skb_reset_mac_header(skb);
+   skb->mac_len -= ETH_HLEN;
+
+   /* safe right before invalidate_flow_key */
+   key->mac_proto = MAC_PROTO_NONE;
+   invalidate_flow_key(key);
+   return 0;
+}
+
+static int push_eth(struct sk_buff *skb, struct sw_flow_key *key,
+   const struct ovs_action_push_eth *ethh)
+{
+   struct ethhdr *hdr;
+
+   /* Add the new Ethernet header */
+   if (skb_cow_head(skb, ETH_HLEN) < 0)
+   return -ENOMEM;
+
+   skb_push(skb, ETH_HLEN);
+   skb_reset_mac_header(skb);
+   skb->mac_len = ETH_HLEN;
+
+   hdr = eth_hdr(skb);
+   ether_addr_copy(hdr->h_source, ethh->addresses.eth_src);
+   ether_addr_copy(hdr->h_dest, ethh->addresses.eth_dst);
+   hdr->h_proto = skb->protocol;
+
+   skb_postpush_rcsum(skb, hdr, ETH_HLEN);
+
+   /* safe right before invalidate_flow_key */
+   key->mac_proto = MAC_PROTO_ETHERNET;
+   invalidate_flow_key(key);
+   return 0;
+}
+
 static void update_ip_l4_checksum(struct sk_buff *skb, struct iphdr *nh,
  __be32 addr, __be32 new_addr)
 {
@@ -1200,6 +1241,14 @@ static int do_execute_actions(struct datapath *dp, 
struct sk_buff *skb,
if (err)
return err == -EINPROGRESS ? 0 : err;
break;
+
+   case OVS_ACTION_ATTR_PUSH_ETH:
+   err = push_eth(skb, key, nla_data(a));
+   break;
+
+   case OVS_ACTION_ATTR_POP_ETH:
+   err = pop_eth(skb, key);
+   break;
}
 
if (unlikely(err)) {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index c3d0cc4321c3..d19044f2b1f4 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2383,6 +2383,8 @@ static int __ovs_nla_copy_actions(struct net *net, const 
struct nlattr *attr,
[OVS_ACTION_ATTR_HASH] = sizeof(struct ovs_action_hash),
[OVS_ACTION_ATTR_CT] = (u32)-1,
[OVS_ACTION_ATTR_TRUNC] = sizeof(struct