[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