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.
Everything in openvswitch.h, except the new FLOW_* macros, has an OVS_
prefix. Perhaps the new macros should also?
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.)
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.
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.
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;
}
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.
I think that gre_update_header() breaks GRE64 by using the low 32 bits
as both halves of the key.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
