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.
> 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).
> +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.
> 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.
> 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.
> @@ -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.)
> @@ -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.
> case OVS_ACTION_ATTR_POP_VLAN:
> break;
Presumably pop vlan should require an Ethernet header to be present?
> 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?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
