[ovs-dev] [PATCH v4 3/3] datapath: add layer 3 flow/port support
Lori Jakab
lojakab at cisco.com
Thu Jul 31 12:45:24 UTC 2014
On 7/31/14, 10:09 AM, Jesse Gross wrote:
> On Fri, Jun 27, 2014 at 6:21 AM, Lorand Jakab <lojakab at 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 <lojakab at 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. */
More information about the dev
mailing list