odp-util: Update ODPUTIL_FLOW_KEY_BYTES for current kernel flow format.
authorBen Pfaff <blp@nicira.com>
Tue, 15 May 2012 19:50:57 +0000 (12:50 -0700)
committerBen Pfaff <blp@nicira.com>
Fri, 25 May 2012 16:56:18 +0000 (09:56 -0700)
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>
lib/odp-util.c
lib/odp-util.h
tests/odp.at
tests/test-odp.c

index 8693d3c81baf635d64b789ec20bd6ec85c0141e7..0574c9f359b80463a9db2d717932334bc879a12e 100644 (file)
@@ -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)
 {
index d53f0836ba749bc8704edb839cc02d74b6e5ece3..f902b766dd1d8013314a87e9ed1822ace1426699 100644 (file)
@@ -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
index caedc94d3f9faf830fa490f0be33bd842c394ebb..9617af266e04b879daa998425badbccf272c7ae9 100644 (file)
@@ -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
index 2b4cfe19f85be85ff53c0198f3212f11f7ddf71e..dd80766f7c155249b36ca519cb8399bb32ef78da 100644 (file)
@@ -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