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

Ben Pfaff blp at nicira.com
Wed Oct 17 22:15:07 UTC 2012


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.



More information about the dev mailing list