On Wed, Oct 17, 2012 at 3:15 PM, Ben Pfaff <b...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
