From 224b2ba1c53279f33eafe76ac46ffdf347f8a077 Mon Sep 17 00:00:00 2001

From: Numan Siddique <numans@ovn.org>

Date: Fri, 20 Nov 2020 16:22:53 +0530

Subject: [PATCH] Fix OVN update issue when ovn-controller is updated first

 from 20.06 to 20.09.

The commit in the Fixes tag changed the ct_commit signature from

ct_commit(..) to ct_commit{ ..}.  When ovn-controllers are updated

to a vesion which has this change and if ovn-northd is still not

udated to this version, then the logical flow with the action

ct_commit(..) will be rejected by ovn-controllers resulting in

datapath disruptions.  OVN recommends ovn-controller updates before

ovn-northd, but it is broken now.  This patch fixes this issue by

adding back the support for the old ct_commit(..).  We now support

both the versions of ct_commit.

Fixes: 6cfb44a76c61("Used nested actions in ct_commit)

CC: Mark Michelson <mmichels@redhat.com>

Signed-off-by: Numan Siddique <numans@ovn.org>

Acked-by: Flavio Fernandes <flavio@flaviof.com>

---

 include/ovn/actions.h |  10 ++-

 lib/actions.c         | 137 ++++++++++++++++++++++++++++++++++++++++--

 tests/ovn.at          |  44 ++++++++++++++

 utilities/ovn-trace.c |   3 +-

 4 files changed, 186 insertions(+), 8 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h

index 7ba24cd60..9c1ebf4aa 100644

--- a/include/ovn/actions.h

+++ b/include/ovn/actions.h

@@ -61,7 +61,8 @@ struct ovn_extend_table;

     OVNACT(EXCHANGE,          ovnact_move)            \

     OVNACT(DEC_TTL,           ovnact_null)            \

     OVNACT(CT_NEXT,           ovnact_ct_next)         \

-    OVNACT(CT_COMMIT,         ovnact_nest)            \

+    OVNACT(CT_COMMIT_V1,      ovnact_ct_commit_v1)    \

+    OVNACT(CT_COMMIT_V2,      ovnact_nest)            \

     OVNACT(CT_DNAT,           ovnact_ct_nat)          \

     OVNACT(CT_SNAT,           ovnact_ct_nat)          \

     OVNACT(CT_LB,             ovnact_ct_lb)           \

@@ -230,6 +231,13 @@ struct ovnact_ct_next {

     uint8_t ltable;                /* Logical table ID of next table. */

 };

+/* OVNACT_CT_COMMIT_V1. */

+struct ovnact_ct_commit_v1 {

+    struct ovnact ovnact;

+    uint32_t ct_mark, ct_mark_mask;

+    ovs_be128 ct_label, ct_label_mask;

+};

+

 /* OVNACT_CT_DNAT, OVNACT_CT_SNAT. */

 struct ovnact_ct_nat {

     struct ovnact ovnact;

diff --git a/lib/actions.c b/lib/actions.c

index 3219ab3be..4a9813218 100644

--- a/lib/actions.c

+++ b/lib/actions.c

@@ -627,16 +627,75 @@ ovnact_ct_next_free(struct ovnact_ct_next *a OVS_UNUSED)

 {

 }

+static void

+parse_ct_commit_v1_arg(struct action_context *ctx,

+                       struct ovnact_ct_commit_v1 *cc)

+{

+    if (lexer_match_id(ctx->lexer, "ct_mark")) {

+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {

+            return;

+        }

+        if (ctx->lexer->token.type == LEX_T_INTEGER) {

+            cc->ct_mark = ntohll(ctx->lexer->token.value.integer);

+            cc->ct_mark_mask = UINT32_MAX;

+        } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {

+            cc->ct_mark = ntohll(ctx->lexer->token.value.integer);

+            cc->ct_mark_mask = ntohll(ctx->lexer->token.mask.integer);

+        } else {

+            lexer_syntax_error(ctx->lexer, "expecting integer");

+            return;

+        }

+        lexer_get(ctx->lexer);

+    } else if (lexer_match_id(ctx->lexer, "ct_label")) {

+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {

+            return;

+        }

+        if (ctx->lexer->token.type == LEX_T_INTEGER) {

+            cc->ct_label = ctx->lexer->token.value.be128_int;

+            cc->ct_label_mask = OVS_BE128_MAX;

+        } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {

+            cc->ct_label = ctx->lexer->token.value.be128_int;

+            cc->ct_label_mask = ctx->lexer->token.mask.be128_int;

+        } else {

+            lexer_syntax_error(ctx->lexer, "expecting integer");

+            return;

+        }

+        lexer_get(ctx->lexer);

+    } else {

+        lexer_syntax_error(ctx->lexer, NULL);

+    }

+}

+

+static void

+parse_CT_COMMIT_V1(struct action_context *ctx)

+{

+    add_prerequisite(ctx, "ip");

+

+    struct ovnact_ct_commit_v1 *ct_commit =

+        ovnact_put_CT_COMMIT_V1(ctx->ovnacts);

+    if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {

+        while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {

+            parse_ct_commit_v1_arg(ctx, ct_commit);

+            if (ctx->lexer->error) {

+                return;

+            }

+            lexer_match(ctx->lexer, LEX_T_COMMA);

+        }

+    }

+}

+

 static void

 parse_CT_COMMIT(struct action_context *ctx)

 {

     if (ctx->lexer->token.type == LEX_T_LCURLY) {

-        parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip",

+        parse_nested_action(ctx, OVNACT_CT_COMMIT_V2, "ip",

                             WR_CT_COMMIT);

+    } else if (ctx->lexer->token.type == LEX_T_LPAREN) {

+        parse_CT_COMMIT_V1(ctx);

     } else {

         /* Add an empty nested action to allow for "ct_commit;" syntax */

         add_prerequisite(ctx, "ip");

-        struct ovnact_nest *on = ovnact_put(ctx->ovnacts, OVNACT_CT_COMMIT,

+        struct ovnact_nest *on = ovnact_put(ctx->ovnacts, OVNACT_CT_COMMIT_V2,

                                             OVNACT_ALIGN(sizeof *on));

         on->nested_len = 0;

         on->nested = NULL;

@@ -644,7 +703,73 @@ parse_CT_COMMIT(struct action_context *ctx)

 }

 static void

-format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s)

+format_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc, struct ds *s)

+{

+    ds_put_cstr(s, "ct_commit(");

+    if (cc->ct_mark_mask) {

+        ds_put_format(s, "ct_mark=%#"PRIx32, cc->ct_mark);

+        if (cc->ct_mark_mask != UINT32_MAX) {

+            ds_put_format(s, "/%#"PRIx32, cc->ct_mark_mask);

+        }

+    }

+    if (!ovs_be128_is_zero(cc->ct_label_mask)) {

+        if (ds_last(s) != '(') {

+            ds_put_cstr(s, ", ");

+        }

+

+        ds_put_format(s, "ct_label=");

+        ds_put_hex(s, &cc->ct_label, sizeof cc->ct_label);

+        if (!ovs_be128_equals(cc->ct_label_mask, OVS_BE128_MAX)) {

+            ds_put_char(s, '/');

+            ds_put_hex(s, &cc->ct_label_mask, sizeof cc->ct_label_mask);

+        }

+    }

+    if (!ds_chomp(s, '(')) {

+        ds_put_char(s, ')');

+    }

+    ds_put_char(s, ';');

+}

+

+static void

+encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc,

+                    const struct ovnact_encode_params *ep OVS_UNUSED,

+                    struct ofpbuf *ofpacts)

+{

+    struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);

+    ct->flags = NX_CT_F_COMMIT;

+    ct->recirc_table = NX_CT_RECIRC_NONE;

+    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);

+    ct->zone_src.ofs = 0;

+    ct->zone_src.n_bits = 16;

+

+    size_t set_field_offset = ofpacts->size;

+    ofpbuf_pull(ofpacts, set_field_offset);

+

+    if (cc->ct_mark_mask) {

+        const ovs_be32 value = htonl(cc->ct_mark);

+        const ovs_be32 mask = htonl(cc->ct_mark_mask);

+        ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), &value, &mask);

+    }

+

+    if (!ovs_be128_is_zero(cc->ct_label_mask)) {

+        ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), &cc->ct_label,

+                             &cc->ct_label_mask);

+    }

+

+    ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);

+    ct = ofpacts->header;

+    ofpact_finish(ofpacts, &ct->ofpact);

+}

+

+static void

+ovnact_ct_commit_v1_free(struct ovnact_ct_commit_v1 *cc OVS_UNUSED)

+{

+}

+

+

+

+static void

+format_CT_COMMIT_V2(const struct ovnact_nest *on, struct ds *s)

 {

     if (on->nested_len) {

         format_nested_action(on, "ct_commit", s);

@@ -654,9 +779,9 @@ format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s)

 }

 static void

-encode_CT_COMMIT(const struct ovnact_nest *on,

-                 const struct ovnact_encode_params *ep OVS_UNUSED,

-                 struct ofpbuf *ofpacts)

+encode_CT_COMMIT_V2(const struct ovnact_nest *on,

+                    const struct ovnact_encode_params *ep OVS_UNUSED,

+                    struct ofpbuf *ofpacts)

 {

     struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);

     ct->flags = NX_CT_F_COMMIT;

diff --git a/tests/ovn.at b/tests/ovn.at

index f56f8a696..396e60eeb 100644

--- a/tests/ovn.at

+++ b/tests/ovn.at

@@ -1102,6 +1102,50 @@ ct_commit { ct_label=0x181716151413121110090807060504030201; };

 ct_commit { ip4.dst = 192.168.0.1; };

     Field ip4.dst is not modifiable.

+# Legact ct_commit_v1 action.

+ct_commit();

+    formats as ct_commit;

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15])

+    has prereqs ip

+ct_commit(ct_mark=1);

+    formats as ct_commit(ct_mark=0x1);

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark))

+    has prereqs ip

+ct_commit(ct_mark=1/1);

+    formats as ct_commit(ct_mark=0x1/0x1);

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_mark))

+    has prereqs ip

+ct_commit(ct_label=1);

+    formats as ct_commit(ct_label=0x1);

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_label))

+    has prereqs ip

+ct_commit(ct_label=1/1);

+    formats as ct_commit(ct_label=0x1/0x1);

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_label))

+    has prereqs ip

+ct_commit(ct_mark=1, ct_label=2);

+    formats as ct_commit(ct_mark=0x1, ct_label=0x2);

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label))

+    has prereqs ip

+

+ct_commit(ct_label=0x01020304050607080910111213141516);

+    formats as ct_commit(ct_label=0x1020304050607080910111213141516);

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1020304050607080910111213141516->ct_label))

+    has prereqs ip

+ct_commit(ct_label=0x181716151413121110090807060504030201);

+    formats as ct_commit(ct_label=0x16151413121110090807060504030201);

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x16151413121110090807060504030201->ct_label))

+    has prereqs ip

+ct_commit(ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000);

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label))

+    has prereqs ip

+ct_commit(ct_label=18446744073709551615);

+    formats as ct_commit(ct_label=0xffffffffffffffff);

+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0xffffffffffffffff->ct_label))

+    has prereqs ip

+ct_commit(ct_label=18446744073709551616);

+    Decimal constants must be less than 2**64.

+

 ct_mark = 12345

     Field ct_mark is not modifiable.

 ct_label = 0xcafe

diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c

index cc1cd1b16..8421c0682 100644

--- a/utilities/ovn-trace.c

+++ b/utilities/ovn-trace.c

@@ -2334,7 +2334,8 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,

             execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, super);

             break;

-        case OVNACT_CT_COMMIT:

+        case OVNACT_CT_COMMIT_V1:

+        case OVNACT_CT_COMMIT_V2:

             /* Nothing to do. */

             break;

-- 

2.28.0