From 11f84dce005dd14a374ab2ef5f8c25bcf8285a36 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Tue, 15 May 2012 12:50:57 -0700 Subject: 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 Signed-off-by: Ben Pfaff --- lib/dpif-netdev.c | 1 + lib/odp-util.c | 5 ++++- lib/odp-util.h | 23 ++++++++++++++++++----- tests/odp.at | 6 ++++++ tests/test-odp.c | 9 ++++++++- 5 files changed, 37 insertions(+), 7 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 1dc98171..67b51896 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -49,6 +49,7 @@ #include "poll-loop.h" #include "random.h" #include "shash.h" +#include "sset.h" #include "timeval.h" #include "util.h" #include "vlog.h" diff --git a/lib/odp-util.c b/lib/odp-util.c index dd67ef62..7b725412 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -1155,7 +1155,10 @@ ovs_to_odp_frag(uint8_t ovs_frag) : OVS_FRAG_TYPE_NONE); } -/* 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 415db60b..a6adad5a 100644 --- 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 shash *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 shash *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 90a11920..3e7a8adb 100644 --- a/tests/odp.at +++ b/tests/odp.at @@ -45,6 +45,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 9ae897c5..46533ea9 100644 --- a/tests/test-odp.c +++ b/tests/test-odp.c @@ -27,6 +27,7 @@ int main(void) { + int exit_code = 0; struct ds in; ds_init(&in); @@ -85,6 +86,12 @@ main(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); @@ -96,5 +103,5 @@ main(void) } ds_destroy(&in); - return 0; + return exit_code; } -- cgit v1.2.3