| Message ID | 1516716522-23372-1-git-send-email-john.hurley@netronome.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,OVS,1/1] tc flower: reorder tunnel encap/decap actions | expand |
Simon, do you want to apply this? I see that you reviewed it. On Tue, Jan 23, 2018 at 02:08:42PM +0000, 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 <john.hurley@netronome.com> > Reviewed-by: Simon Horman <simon.horman@netronome.com> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
2018/01/23 19:02 "Ben Pfaff" <blp@ovn.org>: 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 +0000, 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 <john.hurley@netronome.com> > Reviewed-by: Simon Horman <simon.horman@netronome.com> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Jan 23, 2018 at 02:08:42PM +0000, 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 <john.hurley@netronome.com> > Reviewed-by: Simon Horman <simon.horman@netronome.com> 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.
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);