diff mbox

[ovs-dev,4/9] odp-util: Commit ICMP set only for ICMP packets.

Message ID 1449714462-31831-5-git-send-email-diproiettod@vmware.com
State Accepted
Headers show

Commit Message

Daniele Di Proietto Dec. 10, 2015, 2:27 a.m. UTC 
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().

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 <diproiettod@vmware.com>
---
 lib/odp-util.c      | 10 ++++++++--
 tests/mpls-xlate.at |  2 +-
 2 files changed, 9 insertions(+), 3 deletions(-)

Comments

Jarno Rajahalme Dec. 10, 2015, 10:57 p.m. UTC | #1 
With a note below,

Acked-by: Jarno Rajahalme <jarno@ovn.org>

> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiettod@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."

> 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 <diproiettod@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: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
> ])
> -- 
> 2.1.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Daniele Di Proietto Dec. 11, 2015, 1:47 a.m. UTC | #2 
On 10/12/2015 14:57, "Jarno Rajahalme" <jarno@ovn.org> wrote:

>With a note below,
>
>Acked-by: Jarno Rajahalme <jarno@ovn.org>
>
>> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto
>><diproiettod@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 <diproiettod@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=
>
diff mbox

Patch

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: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
 ])