On 10/12/2015 14:57, "Jarno Rajahalme" <ja...@ovn.org> wrote:
>With a note below,
>
>Acked-by: Jarno Rajahalme <ja...@ovn.org>
>
>> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto
>><diproiet...@vmware.com> wrote:
>>
>> 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_port_action() is called before commit_set_icmp_actions().
>>
>
>Maybe add: ³, due to ports using the same fields as icmp, causing
>commit_set_imp_action() seeing the fields as already committed."
I rephrased it in
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).
and pushed it to master and branch-2.5
Thanks!
>
>> 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.
>>
>> Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
>> ---
>> 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 90942c7..4db371d 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 8f286c3..38790ea 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:LOCA
>>L])
>>
>> 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(0
>>x0800),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(0
>>x0800),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
>> ])
>> --
>> 2.1.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>>
>>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm
>>an_listinfo_dev&d=BQIFaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=
>>SmB5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=AYzr3gKsVDquCLAT1_sp9T7F-Zh
>>6qkFit0yVO__bvW8&s=OO5PIhmhuILxvET-1xP7l5jQd7lmY-iG3Jot4pnBFXk&e=
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
