Re: [ovs-dev] [PATCH OVS 1/1] tc flower: reorder tunnel encap/decap actions

[Simon Horman]

On Tue, Jan 23, 2018 at 02:08:42PM +, John Hurley wrote:
> The tc_flower conversion struct does not consider the order of actions.
> If an OvS rule matches on a tunnel (decap) and outputs to a new tunnel,
> the netlink conversion to TC will add the set tunnel key action before the
> unset, leading to an incorrect TC rule. This patch reorders the netlink
> generation to ensure a decap is done before an encap if both exist.
> 
> Signed-off-by: John Hurley 
> Reviewed-by: Simon Horman 

Thanks John,

I have applied to master and branch-2.9.

It looks like the fix is also needed for branch-2.8 but does not
apply cleanly there. Please consider posting a backport to that branch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH OVS 1/1] tc flower: reorder tunnel encap/decap actions

[Simon Horman]

2018/01/23 19:02 "Ben Pfaff" :

Simon, do you want to apply this?  I see that you reviewed it.


Sure, will do.

On Tue, Jan 23, 2018 at 02:08:42PM +, John Hurley wrote:
> The tc_flower conversion struct does not consider the order of actions.
> If an OvS rule matches on a tunnel (decap) and outputs to a new tunnel,
> the netlink conversion to TC will add the set tunnel key action before the
> unset, leading to an incorrect TC rule. This patch reorders the netlink
> generation to ensure a decap is done before an encap if both exist.
>
> Signed-off-by: John Hurley 
> Reviewed-by: Simon Horman 
> ---
>  lib/tc.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index bfe20ce..914465a 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -1379,6 +1379,11 @@ nl_msg_put_flower_acts(struct ofpbuf *request,
struct tc_flower *flower)
>  nl_msg_end_nested(request, act_offset);
>  }
>  }
> +if (flower->tunnel.tunnel) {
> +act_offset = nl_msg_start_nested(request, act_index++);
> +nl_msg_put_act_tunnel_key_release(request);
> +nl_msg_end_nested(request, act_offset);
> +}
>  if (flower->set.set) {
>  act_offset = nl_msg_start_nested(request, act_index++);
>  nl_msg_put_act_tunnel_key_set(request, flower->set.id,
> @@ -1389,11 +1394,6 @@ nl_msg_put_flower_acts(struct ofpbuf *request,
struct tc_flower *flower)
>flower->set.tp_dst);
>  nl_msg_end_nested(request, act_offset);
>  }
> -if (flower->tunnel.tunnel) {
> -act_offset = nl_msg_start_nested(request, act_index++);
> -nl_msg_put_act_tunnel_key_release(request);
> -nl_msg_end_nested(request, act_offset);
> -}
>  if (flower->vlan_pop) {
>  act_offset = nl_msg_start_nested(request, act_index++);
>  nl_msg_put_act_pop_vlan(request);
> --
> 1.9.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH OVS 1/1] tc flower: reorder tunnel encap/decap actions

[Ben Pfaff]

Simon, do you want to apply this?  I see that you reviewed it.

On Tue, Jan 23, 2018 at 02:08:42PM +, John Hurley wrote:
> The tc_flower conversion struct does not consider the order of actions.
> If an OvS rule matches on a tunnel (decap) and outputs to a new tunnel,
> the netlink conversion to TC will add the set tunnel key action before the
> unset, leading to an incorrect TC rule. This patch reorders the netlink
> generation to ensure a decap is done before an encap if both exist.
> 
> Signed-off-by: John Hurley 
> Reviewed-by: Simon Horman 
> ---
>  lib/tc.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/tc.c b/lib/tc.c
> index bfe20ce..914465a 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -1379,6 +1379,11 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct 
> tc_flower *flower)
>  nl_msg_end_nested(request, act_offset);
>  }
>  }
> +if (flower->tunnel.tunnel) {
> +act_offset = nl_msg_start_nested(request, act_index++);
> +nl_msg_put_act_tunnel_key_release(request);
> +nl_msg_end_nested(request, act_offset);
> +}
>  if (flower->set.set) {
>  act_offset = nl_msg_start_nested(request, act_index++);
>  nl_msg_put_act_tunnel_key_set(request, flower->set.id,
> @@ -1389,11 +1394,6 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct 
> tc_flower *flower)
>flower->set.tp_dst);
>  nl_msg_end_nested(request, act_offset);
>  }
> -if (flower->tunnel.tunnel) {
> -act_offset = nl_msg_start_nested(request, act_index++);
> -nl_msg_put_act_tunnel_key_release(request);
> -nl_msg_end_nested(request, act_offset);
> -}
>  if (flower->vlan_pop) {
>  act_offset = nl_msg_start_nested(request, act_index++);
>  nl_msg_put_act_pop_vlan(request);
> -- 
> 1.9.1
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH OVS 1/1] tc flower: reorder tunnel encap/decap actions

[John Hurley]

The tc_flower conversion struct does not consider the order of actions.
If an OvS rule matches on a tunnel (decap) and outputs to a new tunnel,
the netlink conversion to TC will add the set tunnel key action before the
unset, leading to an incorrect TC rule. This patch reorders the netlink
generation to ensure a decap is done before an encap if both exist.

Signed-off-by: John Hurley 
Reviewed-by: Simon Horman 
---
 lib/tc.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index bfe20ce..914465a 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1379,6 +1379,11 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct 
tc_flower *flower)
 nl_msg_end_nested(request, act_offset);
 }
 }
+if (flower->tunnel.tunnel) {
+act_offset = nl_msg_start_nested(request, act_index++);
+nl_msg_put_act_tunnel_key_release(request);
+nl_msg_end_nested(request, act_offset);
+}
 if (flower->set.set) {
 act_offset = nl_msg_start_nested(request, act_index++);
 nl_msg_put_act_tunnel_key_set(request, flower->set.id,
@@ -1389,11 +1394,6 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct 
tc_flower *flower)
   flower->set.tp_dst);
 nl_msg_end_nested(request, act_offset);
 }
-if (flower->tunnel.tunnel) {
-act_offset = nl_msg_start_nested(request, act_index++);
-nl_msg_put_act_tunnel_key_release(request);
-nl_msg_end_nested(request, act_offset);
-}
 if (flower->vlan_pop) {
 act_offset = nl_msg_start_nested(request, act_index++);
 nl_msg_put_act_pop_vlan(request);
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev