| Message ID | 20211104235134.2147373-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] ofproto-dpif-xlate: Fix check_pkt_larger incomplete translation. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
numans@ovn.org writes: > From: Numan Siddique <numans@ovn.org> > > xlate_check_pkt_larger() sets ctx->exit to 'true' at the end > causing the translation to stop. This results in incomplete > datapath rules. > > For example, for the below OF rules configured on a bridge, > > table=0,in_port=1 > actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1) > table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4) > table=1,in_port=1,reg1=0x2 actions=output:2 > table=1,in_port=1,reg1=0x3 actions=output:4 > table=4,in_port=1 actions=output:3 > > the datapath flow should be > > check_pkt_len(size=200,gt(3),le(3)),2,4 > > But right now it is: > > check_pkt_len(size=200,gt(3),le(3)) > > Actions after the first resubmit(,1) in the first flow in table 0 > are never applied. This patch fixes this issue. > > Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger") > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2018365 > Reported-by: Ihar Hrachyshka <ihrachys@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- LGTM. Acked-by: Aaron Conole <aconole@redhat.com>
On 11/9/21 15:48, Aaron Conole wrote: > numans@ovn.org writes: > >> From: Numan Siddique <numans@ovn.org> >> >> xlate_check_pkt_larger() sets ctx->exit to 'true' at the end >> causing the translation to stop. This results in incomplete >> datapath rules. >> >> For example, for the below OF rules configured on a bridge, >> >> table=0,in_port=1 >> actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1) >> table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4) >> table=1,in_port=1,reg1=0x2 actions=output:2 >> table=1,in_port=1,reg1=0x3 actions=output:4 >> table=4,in_port=1 actions=output:3 >> >> the datapath flow should be >> >> check_pkt_len(size=200,gt(3),le(3)),2,4 >> >> But right now it is: >> >> check_pkt_len(size=200,gt(3),le(3)) >> >> Actions after the first resubmit(,1) in the first flow in table 0 >> are never applied. This patch fixes this issue. >> >> Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger") >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2018365 >> Reported-by: Ihar Hrachyshka <ihrachys@redhat.com> >> Signed-off-by: Numan Siddique <numans@ovn.org> >> --- > > LGTM. > > Acked-by: Aaron Conole <aconole@redhat.com> Thanks, Numan and Aaron! Applied. Backported down to 2.13. Best regards, Ilya Maximets.
On Wed, Nov 17, 2021, 7:06 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 11/9/21 15:48, Aaron Conole wrote: > > numans@ovn.org writes: > > > >> From: Numan Siddique <numans@ovn.org> > >> > >> xlate_check_pkt_larger() sets ctx->exit to 'true' at the end > >> causing the translation to stop. This results in incomplete > >> datapath rules. > >> > >> For example, for the below OF rules configured on a bridge, > >> > >> table=0,in_port=1 > >> > actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1) > >> table=1,in_port=1,reg1=0x1 > actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4) > >> table=1,in_port=1,reg1=0x2 actions=output:2 > >> table=1,in_port=1,reg1=0x3 actions=output:4 > >> table=4,in_port=1 actions=output:3 > >> > >> the datapath flow should be > >> > >> check_pkt_len(size=200,gt(3),le(3)),2,4 > >> > >> But right now it is: > >> > >> check_pkt_len(size=200,gt(3),le(3)) > >> > >> Actions after the first resubmit(,1) in the first flow in table 0 > >> are never applied. This patch fixes this issue. > >> > >> Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger") > >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2018365 > >> Reported-by: Ihar Hrachyshka <ihrachys@redhat.com> > >> Signed-off-by: Numan Siddique <numans@ovn.org> > >> --- > > > > LGTM. > > > > Acked-by: Aaron Conole <aconole@redhat.com> > > Thanks, Numan and Aaron! > > Applied. Backported down to 2.13. > Thanks Ilya and Aaron. Numan > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 9d336bc6a6..8ee6f5a4c6 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -6371,6 +6371,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, * then ctx->exit would be true. Reset to false so that we can * do flow translation for 'IF_LESS_EQUAL' case. finish_freezing() * would have taken care of Undoing the changes done for freeze. */ + bool old_exit = ctx->exit; ctx->exit = false; offset_attr = nl_msg_start_nested( @@ -6395,7 +6396,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, ctx->was_mpls = old_was_mpls; ctx->conntracked = old_conntracked; ctx->xin->flow = old_flow; - ctx->exit = true; + ctx->exit = old_exit; } static void @@ -6776,6 +6777,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, return; } + bool exit = false; OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { struct ofpact_controller *controller; const struct ofpact_metadata *metadata; @@ -6790,7 +6792,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, recirc_for_mpls(a, ctx); - if (ctx->exit) { + if (ctx->exit || exit) { /* Check if need to store the remaining actions for later * execution. */ if (ctx->freezing) { @@ -7192,6 +7194,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, ofpacts_len); xlate_check_pkt_larger(ctx, ofpact_get_CHECK_PKT_LARGER(a), remaining_acts, remaining_acts_len); + if (ctx->xbridge->support.check_pkt_len) { + /* If datapath supports check_pkt_len, then + * xlate_check_pkt_larger() does the translation for the + * ofpacts following 'a'. */ + exit = true; + } break; } } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 31fb163a91..f7c8f98ce5 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -11452,6 +11452,23 @@ Megaflow: recirc_id=0x3,eth,ip,in_port=1,nw_frag=no Datapath actions: 4 ]) +ovs-ofctl del-flows br0 + +AT_DATA([flows.txt], [dnl +table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1) +table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4) +table=1,in_port=1,reg1=0x2 actions=output:2 +table=1,in_port=1,reg1=0x3 actions=output:4 +table=4,in_port=1 actions=output:3 +]) + +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt]) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout]) +AT_CHECK([cat stdout | grep Datapath -B1], [0], [dnl +Megaflow: recirc_id=0,eth,ip,in_port=1,nw_frag=no +Datapath actions: check_pkt_len(size=200,gt(3),le(3)),2,4 +]) + OVS_VSWITCHD_STOP AT_CLEANUP