On Wed, Nov 3, 2021 at 10:34 AM Ilya Maximets <[email protected]> wrote: > > On 10/29/21 19:26, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > ofproto/ofproto-dpif-xlate.c | 9 ++++++++- > > tests/ofproto-dpif.at | 17 +++++++++++++++++ > > 2 files changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index 9d336bc6a6..e22a39a9f4 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 > > @@ -7192,6 +7193,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'. */ > > + a = ofpact_end(ofpacts, ofpacts_len); > > Hi, Numan. > > Looks like this change triggers heap-buffer-overflow. > See the ASAN report in ovsrobot's run for details.
Oops. Thanks for pointing this out. I missed running tests with libasan locally. Thanks Numan > > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
