On Thu, Jun 19, 2014 at 4:07 PM, Jesse Gross <je...@nicira.com> wrote:
> On Thu, Jun 19, 2014 at 1:33 PM, Pravin Shelar <pshe...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
