From efa6665e4d72e4d4f7cc971fb0df54278dba3e47 Mon Sep 17 00:00:00 2001 From: Daniele Di Proietto Date: Tue, 8 Dec 2015 15:44:51 -0800 Subject: [PATCH] odp-util: Commit ICMP set only for ICMP packets. commit_set_icmp_action() should do its job only if the packet is ICMP, otherwise there will be two problems: * A set ICMP action will be inserted in the ODP actions and the flow will be slow pathed. * The tp_src and tp_dst field will be unwildcarded. Normal TCP or UDP packets won't be impacted, because commit_set_icmp_action() is called after commit_set_port_action() and it will see the fields as already committed (TCP/UCP transport ports and ICMP code/type are stored in the same members in struct flow). MPLS packets though will hit the bug, causing a nonsensical set action (which will end up zeroing the transport source port) and an invalid mask to be generated. The commit also alters an MPLS testcase to trigger the bug. --- lib/odp-util.c | 10 ++++++++-- tests/mpls-xlate.at | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index 90942c7dd..4db371d3b 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -5651,12 +5651,18 @@ commit_set_icmp_action(const struct flow *flow, struct flow *base_flow, struct ovs_key_icmp key, mask, base; enum ovs_key_attr attr; + if (is_icmpv4(flow)) { + attr = OVS_KEY_ATTR_ICMP; + } else if (is_icmpv6(flow)) { + attr = OVS_KEY_ATTR_ICMPV6; + } else { + return 0; + } + get_icmp_key(flow, &key); get_icmp_key(base_flow, &base); get_icmp_key(&wc->masks, &mask); - attr = flow->dl_type == htons(ETH_TYPE_IP) ? OVS_KEY_ATTR_ICMP - : OVS_KEY_ATTR_ICMPV6; if (commit(attr, false, &key, &base, &mask, sizeof key, odp_actions)) { put_icmp_key(&base, base_flow); put_icmp_key(&mask, &wc->masks); diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at index 8f286c3a5..38790ea52 100644 --- a/tests/mpls-xlate.at +++ b/tests/mpls-xlate.at @@ -16,7 +16,7 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,dl_type=0x0800,acti AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 dl_type=0x8847,in_port=1,mpls_label=20,action=pop_mpls:0x0800,output:LOCAL]) dnl Test MPLS push -AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout]) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=17,tos=0,ttl=64,frag=no),udp(src=7777,dst=80)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], [Datapath actions: push_mpls(label=10,tc=0,ttl=64,bos=1,eth_type=0x8847),1 ])