odp-util: Update ODPUTIL_FLOW_KEY_BYTES for current kernel flow format.

Before we submitted the kernel module upstream, we updated the flow format
by adding two fields to the description of packets with VLAN headers, but
we forgot to update ODPUTIL_FLOW_KEY_BYTES to reflect these changes.  The
result was that a maximum-length flow did not fit in the given space.

This fixes a crash processing IPv6 neighbor discovery packets with VLAN
headers received in a tunnel configured with key=flow or in_key=flow.

This updates some comments to better describe the implications of
ODPUTIL_FLOW_KEY_BYTES (suggested by Justin).

This also updates test-odp.c so that it would have caught this problem, and
updates odp.at to demonstrate that a full 156 bytes are necessary.  (To see
that, revert the change to ODPUTIL_FLOW_KEY_BYTES and run the test.)

Reported-by: Dan Wendlandt <dan@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 8693d3c81baf635d64b789ec20bd6ec85c0141e7..0574c9f359b80463a9db2d717932334bc879a12e 100644 (file)
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1246,7 +1246,10 @@ ovs_to_odp_frag(uint8_t nw_frag)
           : OVS_FRAG_TYPE_LATER);
 }
 
-/* Appends a representation of 'flow' as OVS_KEY_ATTR_* attributes to 'buf'. */
+/* Appends a representation of 'flow' as OVS_KEY_ATTR_* attributes to 'buf'.
+ *
+ * 'buf' must have at least ODPUTIL_FLOW_KEY_BYTES bytes of space, or be
+ * capable of being expanded to allow for that much space. */
 void
 odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow)
 {

diff --git a/lib/odp-util.h b/lib/odp-util.h
index d53f0836ba749bc8704edb839cc02d74b6e5ece3..f902b766dd1d8013314a87e9ed1822ace1426699 100644 (file)
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -65,8 +65,16 @@ void format_odp_actions(struct ds *, const struct nlattr *odp_actions,
 int odp_actions_from_string(const char *, const struct simap *port_names,
                             struct ofpbuf *odp_actions);
 
-/* Upper bound on the length of a nlattr-formatted flow key.  The longest
- * nlattr-formatted flow key would be:
+/* The maximum number of bytes that odp_flow_key_from_flow() appends to a
+ * buffer.  This is the upper bound on the length of a nlattr-formatted flow
+ * key that ovs-vswitchd fully understands.
+ *
+ * OVS doesn't insist that ovs-vswitchd and the datapath have exactly the same
+ * idea of a flow, so therefore this value isn't necessarily an upper bound on
+ * the length of a flow key that the datapath can pass to ovs-vswitchd.
+ *
+ * The longest nlattr-formatted flow key appended by odp_flow_key_from_flow()
+ * would be:
  *
  *                         struct  pad  nl hdr  total
  *                         ------  ---  ------  -----
@@ -74,15 +82,20 @@ int odp_actions_from_string(const char *, const struct simap *port_names,
  *  OVS_KEY_ATTR_TUN_ID        8    --     4     12
  *  OVS_KEY_ATTR_IN_PORT       4    --     4      8
  *  OVS_KEY_ATTR_ETHERNET     12    --     4     16
+ *  OVS_KEY_ATTR_ETHERTYPE     2     2     4      8  (outer VLAN ethertype)
  *  OVS_KEY_ATTR_8021Q         4    --     4      8
- *  OVS_KEY_ATTR_ETHERTYPE     2     2     4      8
+ *  OVS_KEY_ATTR_ENCAP         0    --     4      4  (VLAN encapsulation)
+ *  OVS_KEY_ATTR_ETHERTYPE     2     2     4      8  (inner VLAN ethertype)
  *  OVS_KEY_ATTR_IPV6         40    --     4     44
  *  OVS_KEY_ATTR_ICMPV6        2     2     4      8
  *  OVS_KEY_ATTR_ND           28    --     4     32
  *  -------------------------------------------------
- *  total                                       144
+ *  total                                       156
+ *
+ * We include some slack space in case the calculation isn't quite right or we
+ * add another field and forget to adjust this value.
  */
-#define ODPUTIL_FLOW_KEY_BYTES 144
+#define ODPUTIL_FLOW_KEY_BYTES 200
 
 /* A buffer with sufficient size and alignment to hold an nlattr-formatted flow
  * key.  An array of "struct nlattr" might not, in theory, be sufficiently

diff --git a/tests/odp.at b/tests/odp.at
index caedc94d3f9faf830fa490f0be33bd842c394ebb..9617af266e04b879daa998425badbccf272c7ae9 100644 (file)
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -46,6 +46,12 @@ s/$/)/' odp-base.txt
  echo '# Valid forms with tun_id and VLAN headers.'
  sed 's/^/tun_id(0xfedcba9876543210),/
 s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/
+s/$/)/' odp-base.txt
+
+ echo
+ echo '# Valid forms with QOS priority, tun_id, and VLAN headers.'
+ sed 's/^/priority(1234),tun_id(0xfedcba9876543210),/
+s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/
 s/$/)/' odp-base.txt
 
  echo

diff --git a/tests/test-odp.c b/tests/test-odp.c
index 2b4cfe19f85be85ff53c0198f3212f11f7ddf71e..dd80766f7c155249b36ca519cb8399bb32ef78da 100644 (file)
--- a/tests/test-odp.c
+++ b/tests/test-odp.c
@@ -28,6 +28,7 @@
 static int
 parse_keys(void)
 {
+    int exit_code = 0;
     struct ds in;
 
     ds_init(&in);
@@ -71,6 +72,12 @@ parse_keys(void)
         ofpbuf_init(&odp_key, 0);
         odp_flow_key_from_flow(&odp_key, &flow);
 
+        if (odp_key.size > ODPUTIL_FLOW_KEY_BYTES) {
+            printf ("too long: %zu > %d\n",
+                    odp_key.size, ODPUTIL_FLOW_KEY_BYTES);
+            exit_code = 1;
+        }
+
         /* Convert odp_key to string. */
         ds_init(&out);
         odp_flow_key_format(odp_key.data, odp_key.size, &out);
@@ -82,7 +89,7 @@ parse_keys(void)
     }
     ds_destroy(&in);
 
-    return 0;
+    return exit_code;
 }
 
 static int