[ovs-dev] [PATCH 1/3] v7: datapath: Add support for tun_key to Open vSwitch datapath

[Pravin Shelar]

On Wed, Oct 17, 2012 at 3:15 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Sun, Oct 14, 2012 at 06:41:06PM -0700, Pravin B Shelar wrote:
>> This patch was posted by Kyle. I fixed few issues found in earlier
>> version.
>
> I'm not sure that the tun_key handling is correct in the sampling
> case.  ovs_execute_actions() sets OVS_CB(skb)->tun_key to NULL and
> passes &tun_key (a local variable) to ovs_execute_actions().  If
> ovs_execute_actions() calls sample(), then sample() will pass
> OVS_CB(skb)->tun_key, that is, NULL, back to the recursive call to
> do_execute_actions(), and then if the sampled action includes
> OVS_ACTION_ATTR_SET of OVS_KEY_ATTR_TUN_ID, I think that this line in
> execute_set_action() will just set OVS_CB(skb)->tun_key back to NULL
> and cause a null pointer dereference:
>                         OVS_CB(skb)->tun_key = tun_key;
> Maybe I'm missing something?  I didn't try it.

right, it is possible, I need to pass tun_key local var ref.

>
> Everything in openvswitch.h, except the new FLOW_* macros, has an OVS_
> prefix.  Perhaps the new macros should also?
>
ok.

> In ovs_flow_from_nlattrs(), I'm not sure that
>                 swkey->tun.tun_key = *tun_key;
> is safe, because tun_key contains 64-bit members but it is not
> necessarily aligned on a 64-bit boundary.  I would suggest using
> memcpy() in this case.  (This is in two places.)
>
ok

> In ovs_flow_to_nlattrs(), I'd be inclined to fold:
>                 nla = nla_reserve(skb, OVS_KEY_ATTR_IPV4_TUNNEL,
>                                   sizeof(*tun_key));
> into one line, because that better fits my understanding of kernel
> style preferences.  But I may be wrong.
>
ok.

> In build_cache(), the code here seems risky:
> +       tunnel_hlen = tnl_vport->tnl_ops->hdr_len(mutable, &tun_key) +
> +                               sizeof(struct iphdr);
> +       if (tunnel_hlen < 0)
> +               return NULL;
> +
> because if ->hdr_len() returns an error code that happens to be
> between -19 and -1, inclusive, then it will succeed when it should
> fail.  It looks like -EINVAL, which is -22, is the most common (only?)
> error, but I still think that this is a poor design.
>
fixed.

> The previous version was safer:
> -       mutable->tunnel_hlen = tnl_ops->hdr_len(mutable);
> -       if (mutable->tunnel_hlen < 0)
> -               return mutable->tunnel_hlen;
> -
> -       mutable->tunnel_hlen += sizeof(struct iphdr);
> -
>
> In ovs_tnl_send(), can this case happen?  I'm a little surprised:
>                 if (!daddr) {
>                         /* Trying to sent packet from Null-port without
>                          * tunnel info? Drop this packet. */
>                         err = VPORT_E_TX_DROPPED;
>                         goto error_free;
>                 }
>
It is possible if vswitchd generate incorrect action. Can we assume
that kernel will never get wrong actions?

> The use of key_preset in capwap_rcv() seems suspicious.  It is
> initially false and it may be set to true by the call to
> process_capwap_proto(), but this value is never used, instead it is
> always thrown away and replaced by:
>         if (mutable->flags & TNL_F_IN_KEY_MATCH)
>                 key_preset = true;
>         else
>                 key_preset = false;
> later in the function.
>
right, I need to check for key_present here.

> I think that gre_update_header() breaks GRE64 by using the low 32 bits
> as both halves of the key.
>
fixed.

Thanks,
Pravin.