[ovs-dev] [PATCH 7/7] datapath: Add support for Geneve tunneling.

Pravin Shelar pshelar at nicira.com
Thu Jun 19 23:30:22 UTC 2014


On Thu, Jun 19, 2014 at 4:07 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Thu, Jun 19, 2014 at 1:33 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>> git am warning:
>> /home/pravin/ovs/w7/.git/rebase-apply/patch:53: trailing whitespace.
>>
>> }
>>
>> warning: 1 line adds whitespace errors.
>
> Fixed.
>
>> -----------------------------------------------------------
>> compiler warning:
>>
>> lib/odp-util.c:869:15: warning: cast from 'uint8_t *' (aka 'unsigned
>> char *') to 'struct geneve_opt *'
>>       increases required alignment from 1 to 2 [-Wcast-align]
>>         opt = (struct geneve_opt *)((uint8_t *)opt + len);
>>               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> lib/odp-util.c:845:63: warning: unused parameter 'tun' [-Wunused-parameter]
>> parse_geneve_opts(const struct nlattr *attr, struct flow_tnl *tun)
>
> Both fixed.
>
>>> diff --git a/datapath/flow.c b/datapath/flow.c
>>> index f1bb95d..7b108ed 100644
>>> --- a/datapath/flow.c
>>> +++ b/datapath/flow.c
>>> @@ -455,6 +455,13 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>>>                 struct ovs_tunnel_info *tun_info = OVS_CB(skb)->tun_info;
>>>                 memcpy(&key->tun_key, &tun_info->tunnel,
>>>                         sizeof(key->tun_key));
>>> +               if (tun_info->options) {
>>> +                       memcpy(GENEVE_OPTS(key, tun_info->options_len),
>>> +                               tun_info->options, tun_info->options_len);
>> Need to check options_len before copying data from packet.
>
> I think we should be OK here since we already checked it in
> geneve_rcv() when we populated tun_info. Is there anything else that
> we need to check?
>
I am not sure where is opts_len checked if it is less than key->tun_opts[] size.

>>> diff --git a/datapath/vport-geneve.c b/datapath/vport-geneve.c
>>> new file mode 100644
>>> index 0000000..a6e9287
>>> --- /dev/null
>>> +++ b/datapath/vport-geneve.c
>>> +static int geneve_rcv(struct sock *sk, struct sk_buff *skb)
>>> +{
>>> +       struct geneve_port *geneve_port;
>>> +       struct genevehdr *geneveh;
>>> +       int opts_len;
>>> +       struct ovs_tunnel_info tun_info;
>>> +       __be64 key;
>>> +       __be16 flags;
>>> +
>>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,16,0)
>>> +       if (unlikely(udp_lib_checksum_complete(skb)))
>>> +               goto error;
>>> +#endif
>>> +
>>> +       if (unlikely(!pskb_may_pull(skb, GENEVE_BASE_HLEN)))
>>> +               goto error;
>>> +
>>> +       geneveh = geneve_hdr(skb);
>>> +
>>> +       if (unlikely(geneveh->ver != GENEVE_VER))
>>> +               goto error;
>>> +
>>> +       if (unlikely(geneveh->proto_type != htons(ETH_P_TEB)))
>>> +               goto error;
>>> +
>>> +       geneve_port = geneve_find_port(dev_net(skb->dev), udp_hdr(skb)->dest);
>>
>> I guess we will start using rcu_dereference_sk_user_data() after
>> uptreaming udp tunnels.
>
> I think there's no reason why we can't start doing this now and it
> will also help make this code look more like the existing upstream
> code, so I went ahead and did that.
>
>>> +       if (unlikely(!geneve_port))
>>> +               goto error;
>>> +
>>> +       opts_len = geneveh->opt_len * 4;
>>> +       if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len,
>>> +                                htons(ETH_P_TEB)))
>>> +               goto error;
>>> +
>>> +       geneveh = geneve_hdr(skb);
>>> +
>>> +       flags = TUNNEL_KEY |
>>> +               (udp_hdr(skb)->check != 0 ? TUNNEL_CSUM : 0) |
>>> +               (geneveh->oam ? TUNNEL_OAM : 0);
>>> +
>> I am not sure why TUNNEL_CRIT_OPT is not parsed here.
>
> This is mostly intended for devices that aren't capable of parsing the
> options. Since OVS always parses options, we don't really have a use
> for it. That being said, it's probably a good idea for consistency and
> doesn't hurt anything.
>
>>> +static int geneve_send(struct vport *vport, struct sk_buff *skb)
>>> +{
>>> +       struct ovs_key_ipv4_tunnel *tun_key = &OVS_CB(skb)->tun_info->tunnel;
>>> +       int network_offset = skb_network_offset(skb);
>>> +       struct rtable *rt;
>>> +       int min_headroom;
>>> +       __be32 saddr;
>>> +       __be16 df;
>>> +       int sent_len;
>>> +       int err;
>>> +
>>> +       if (unlikely(!OVS_CB(skb)->tun_info))
>>> +               return -EINVAL;
>>> +
>>> +       /* Route lookup */
>>> +       saddr = tun_key->ipv4_src;
>>> +       rt = find_route(ovs_dp_get_net(vport->dp),
>>> +                       &saddr, tun_key->ipv4_dst,
>>> +                       IPPROTO_UDP, tun_key->ipv4_tos,
>>> +                       skb->mark);
>>> +       if (IS_ERR(rt)) {
>>> +               err = PTR_ERR(rt);
>>> +               goto error;
>>> +       }
>>> +
>>> +       min_headroom = LL_RESERVED_SPACE(rt_dst(rt).dev) + rt_dst(rt).header_len
>>> +                       + GENEVE_BASE_HLEN + sizeof(struct iphdr)
>>> +                       + (vlan_tx_tag_present(skb) ? VLAN_HLEN : 0);
>>> +
>> we need to add options_len to headroom calculation.
>
> Good catch.



More information about the dev mailing list