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

[Jarno Rajahalme]

With a note below,

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

> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiettod at 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 at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev