On 7/31/14, 10:09 AM, Jesse Gross wrote:
On Fri, Jun 27, 2014 at 6:21 AM, Lorand Jakab <loja...@cisco.com> wrote:
Implementation of the pop_eth and push_eth actions in the kernel, and
layer 3 flow support.
Signed-off-by: Lorand Jakab <loja...@cisco.com>
Thank you for your patience on this.
Thanks for finding the time to review it. I reply below to each of your
comments, please confirm you agree with the proposed changes before I
send a v5. Not sure what to do about the 'struct ovs_skb_cb' issue with
older kernels though...
diff --git a/datapath/actions.c b/datapath/actions.c
index cb26ad5..20c66f5 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
+static int pop_eth(struct sk_buff *skb)
+{
+ skb_pull_rcsum(skb, skb_network_offset(skb));
+ skb_reset_mac_header(skb);
+ vlan_set_tci(skb, 0);
+
+ OVS_CB(skb)->is_layer3 = true;
I think we should probably also set skb->mac_len to 0 here (and on
push as well).
OK. I made the following changes:
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -245,6 +245,7 @@ static int pop_eth(struct sk_buff *skb)
{
skb_pull_rcsum(skb, skb_network_offset(skb));
skb_reset_mac_header(skb);
+ skb->mac_len = 0;
vlan_set_tci(skb, 0);
OVS_CB(skb)->is_layer3 = true;
@@ -256,6 +257,7 @@ static void push_eth(struct sk_buff *skb, const
struct ovs_action_push_eth *ethh
{
skb_push(skb, ETH_HLEN);
skb_reset_mac_header(skb);
+ skb_reset_mac_len(skb);
ether_addr_copy(eth_hdr(skb)->h_source, ethh->addresses.eth_src);
ether_addr_copy(eth_hdr(skb)->h_dest, ethh->addresses.eth_dst);
+static void push_eth(struct sk_buff *skb, const struct ovs_action_push_eth
*ethh)
+{
+ skb_push(skb, ETH_HLEN);
+ skb_reset_mac_header(skb);
+
+ ether_addr_copy(eth_hdr(skb)->h_source, ethh->addresses.eth_src);
+ ether_addr_copy(eth_hdr(skb)->h_dest, ethh->addresses.eth_dst);
+
+ eth_hdr(skb)->h_proto = ethh->eth_type;
+ skb->protocol = ethh->eth_type;
+
+ ovs_skb_postpush_rcsum(skb, skb->data, ETH_HLEN);
+
+ OVS_CB(skb)->is_layer3 = false;
What happens if there is something in skb->vlan_tci? I think the
effect will be that it still on the outer Ethernet header, which
doesn't seem correct.
I missed that. Addressed with:
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -258,6 +258,7 @@ static void push_eth(struct sk_buff *skb, const
struct ovs_action_push_eth *ethh
skb_push(skb, ETH_HLEN);
skb_reset_mac_header(skb);
skb_reset_mac_len(skb);
+ vlan_set_tci(skb, 0);
ether_addr_copy(eth_hdr(skb)->h_source, ethh->addresses.eth_src);
ether_addr_copy(eth_hdr(skb)->h_dest, ethh->addresses.eth_dst);
diff --git a/datapath/datapath.h b/datapath/datapath.h
index fcd8e86..07ae0c8 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -107,6 +107,7 @@ struct ovs_skb_cb {
struct sw_flow_key *pkt_key;
struct ovs_tunnel_info *tun_info;
struct vport *input_vport;
+ bool is_layer3;
We have run into a problem with ovs_skb_cb: it is currently full on
kernels < 3.11 due to compatibility code.
What can we do about it?
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 5a1a487..347142b 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -114,14 +114,12 @@ static u16 range_n_bytes(const struct sw_flow_key_range
*range)
/* The following mask attributes allowed only if they
* pass the validation tests. */
- mask_allowed &= ~((1ULL << OVS_KEY_ATTR_IPV4)
- | (1ULL << OVS_KEY_ATTR_IPV6)
- | (1ULL << OVS_KEY_ATTR_TCP)
+ mask_allowed &= ~((1ULL << OVS_KEY_ATTR_TCP)
| (1ULL << OVS_KEY_ATTR_TCP_FLAGS)
| (1ULL << OVS_KEY_ATTR_UDP)
| (1ULL << OVS_KEY_ATTR_SCTP)
Shouldn't the IPv4 and v6 keys validate correctly anyways since we set
the EtherType when we receive the keys?
In any case, it would be nice to avoid relaxing this restriction for
non-L3 packets.
@@ -134,7 +132,10 @@ static bool match_validate(const struct sw_flow_match
*match,
/* Always allowed mask fields. */
mask_allowed |= ((1ULL << OVS_KEY_ATTR_TUNNEL)
| (1ULL << OVS_KEY_ATTR_IN_PORT)
- | (1ULL << OVS_KEY_ATTR_ETHERTYPE));
+ | (1ULL << OVS_KEY_ATTR_ETHERNET)
+ | (1ULL << OVS_KEY_ATTR_ETHERTYPE)
+ | (1ULL << OVS_KEY_ATTR_IPV4)
+ | (1ULL << OVS_KEY_ATTR_IPV6));
I don't understand why these masks should be allowed allowed without
having corresponding keys.
You're right, I reverted both the above changes.
@@ -600,8 +601,10 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
*match, u64 attrs,
eth_key->eth_src, ETH_ALEN, is_mask);
SW_FLOW_KEY_MEMCPY(match, eth.dst,
eth_key->eth_dst, ETH_ALEN, is_mask);
+ SW_FLOW_KEY_PUT(match, phy.noeth, false, is_mask);
attrs &= ~(1ULL << OVS_KEY_ATTR_ETHERNET);
- }
+ } else if (!is_mask)
+ SW_FLOW_KEY_PUT(match, phy.noeth, true, is_mask);
This will never set a mask for flows without an Ethernet header, which
means that an IP flow will match things both with and without a
header. Is that the intention? (It seems to me that it would be better
to force a match on whatever form we are using - this could result in
additional flow setups for mixed traffic but it seems unlikely that
this wouldn't be forced already by something else, like the port
number.)
I removed the "if".
@@ -1664,10 +1702,22 @@ static int ovs_nla_copy_actions__(const struct nlattr
*attr,
break;
}
+ case OVS_ACTION_ATTR_POP_ETH:
+ if (noeth)
+ return -EINVAL;
+ noeth = true;
+ break;
+
+ case OVS_ACTION_ATTR_PUSH_ETH:
+ noeth = false;
What is the policy for pushing multiple Ethernet headers onto a
packet? This is definitely useful in some use cases but it potentially
adds some extra complexity and I'm not sure that it's necessary here,
so I think that it can just be disallowed for the time being without
loss of generality.
Agree. I made the following changes:
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -1770,6 +1770,10 @@ static int ovs_nla_copy_actions__(const struct
nlattr *attr,
break;
case OVS_ACTION_ATTR_PUSH_ETH:
+ /* For now disallow pushing an Ethernet header
if one
+ * is already present */
+ if (!noeth)
+ return -EINVAL;
noeth = false;
break;
case OVS_ACTION_ATTR_POP_VLAN:
break;
Presumably pop vlan should require an Ethernet header to be present?
Added check:
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -1774,6 +1774,8 @@ static int ovs_nla_copy_actions__(const struct
nlattr *attr,
break;
case OVS_ACTION_ATTR_POP_VLAN:
+ if (noeth)
+ return -EINVAL;
vlan_tci = htons(0);
break;
diff --git a/datapath/vport.h b/datapath/vport.h
index bdd9a89..20eca8b 100644
--- a/datapath/vport.h
+++ b/datapath/vport.h
@@ -211,7 +211,7 @@ static inline struct vport *vport_from_priv(void *priv)
}
void ovs_vport_receive(struct vport *, struct sk_buff *,
- struct ovs_tunnel_info *);
+ struct ovs_tunnel_info *, bool);
Can you give the bool argument a name since it isn't obvious from the type?
I wanted to be consistent with the other parameters which only specify
the type, but I guess clarity is more important, so I added a
descriptive variable name:
--- a/datapath/vport.h
+++ b/datapath/vport.h
@@ -211,7 +211,7 @@ static inline struct vport *vport_from_priv(void *priv)
}
void ovs_vport_receive(struct vport *, struct sk_buff *,
- struct ovs_tunnel_info *, bool);
+ struct ovs_tunnel_info *, bool is_layer3);
/* List of statically compiled vport implementations. Don't forget to
also
* add yours to the list at the top of vport.c. */
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
