From b74a558d0a5e4c217f966d9615611e3dca1d6c23 Mon Sep 17 00:00:00 2001

From: Mark Michelson <mmichels@redhat.com>

Date: Mon, 27 Jul 2020 16:11:56 -0400

Subject: [PATCH 12/22] Used nested actions in ct_commit

ct_commit allows for ct_label and ct_mark to be set within. However,

there are some restrictions with the current implementation:

* It is not possible to address the indiviual bits within the ct_mark or

   ct_label.

* It is not possible to set these to the value of a register. Only

   explicit integer setting can be used.

With this change, ct_commit now can have arbitrary nested actions

inside. This makes it similar to how the "exec" option works in OVS's

ct() action.

ct_commit now also sets a writeability scope so that ct_mark and

ct_label are the only symbols that are writeable. The positive side

effect is that ct_mark and ct_label are no longer writeable except for

inside ct_commit.

In this commit, the only noticeable effect is that it allows for

slightly more expressive setting of ct_label.blocked. A future commit

will take further advantage of this.

Signed-off-by: Mark Michelson <mmichels@redhat.com>

---

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

 include/ovn/expr.h    |   1 +

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

 lib/logical-fields.c  |   9 ++--

 northd/ovn-northd.c   |   8 +--

 ovn-sb.xml            |  11 +++--

 tests/ovn.at          |  59 ++++++++++++----------

 7 files changed, 72 insertions(+), 135 deletions(-)

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

index 34ba0d880..636cb4bc1 100644

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

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

@@ -57,7 +57,7 @@ struct ovn_extend_table;

     OVNACT(EXCHANGE,          ovnact_move)            \

     OVNACT(DEC_TTL,           ovnact_null)            \

     OVNACT(CT_NEXT,           ovnact_ct_next)         \

-    OVNACT(CT_COMMIT,         ovnact_ct_commit)       \

+    OVNACT(CT_COMMIT,         ovnact_nest)            \

     OVNACT(CT_DNAT,           ovnact_ct_nat)          \

     OVNACT(CT_SNAT,           ovnact_ct_nat)          \

     OVNACT(CT_LB,             ovnact_ct_lb)           \

@@ -222,13 +222,6 @@ struct ovnact_ct_next {

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

 };

-/* OVNACT_CT_COMMIT. */

-struct ovnact_ct_commit {

-    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/include/ovn/expr.h b/include/ovn/expr.h

index 11bfdad5b..b34fb0e81 100644

--- a/include/ovn/expr.h

+++ b/include/ovn/expr.h

@@ -85,6 +85,7 @@ enum expr_level {

 enum expr_write_scope {

     WR_DEFAULT   = (1 << 0), /* Writeable at "global" level */

+    WR_CT_COMMIT = (1 << 1), /* Writeable in "ct_commit" action */

 };

 const char *expr_level_to_string(enum expr_level);

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

index 460ab0cf5..79ac79a95 100644

--- a/lib/actions.c

+++ b/lib/actions.c

@@ -200,6 +200,15 @@ struct action_context {

 static void parse_actions(struct action_context *, enum lex_type sentinel);

+static void parse_nested_action(struct action_context *ctx,

+                                enum ovnact_type type,

+                                const char *prereq,

+                                enum expr_write_scope scope);

+

+static void format_nested_action(const struct ovnact_nest *on,

+                                 const char *name,

+                                 struct ds *s);

+

 static bool

 action_parse_field(struct action_context *ctx,

                    int n_bits, bool rw, struct expr_field *f)

@@ -618,125 +627,42 @@ ovnact_ct_next_free(struct ovnact_ct_next *a OVS_UNUSED)

 {

 }

-static void

-parse_ct_commit_arg(struct action_context *ctx,

-                    struct ovnact_ct_commit *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(struct action_context *ctx)

 {

-    add_prerequisite(ctx, "ip");

-    struct ovnact_ct_commit *ct_commit = ovnact_put_CT_COMMIT(ctx->ovnacts);

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

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

-            parse_ct_commit_arg(ctx, ct_commit);

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

-                return;

-            }

-            lexer_match(ctx->lexer, LEX_T_COMMA);

-        }

-    }

+    parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip",

+                        WR_CT_COMMIT);

 }

 static void

-format_CT_COMMIT(const struct ovnact_ct_commit *cc, struct ds *s)

+format_CT_COMMIT(const struct ovnact_nest *on, 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, ';');

+    format_nested_action(on, "ct_commit", s);

 }

 static void

-encode_CT_COMMIT(const struct ovnact_ct_commit *cc,

+encode_CT_COMMIT(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;

     ct->recirc_table = NX_CT_RECIRC_NONE;

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

+    ct->zone_src.field = ep->is_switch

+        ? mf_from_id(MFF_LOG_CT_ZONE)

+        : mf_from_id(MFF_LOG_DNAT_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);

-    }

-

+    ovnacts_encode(on->nested, on->nested_len, ep, ofpacts);

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

     ct = ofpacts->header;

     ofpact_finish(ofpacts, &ct->ofpact);

 }

-

-static void

-ovnact_ct_commit_free(struct ovnact_ct_commit *cc OVS_UNUSED)

-{

-}

 static void

 parse_ct_nat(struct action_context *ctx, const char *name,

diff --git a/lib/logical-fields.c b/lib/logical-fields.c

index 8639523ea..fde53a47e 100644

--- a/lib/logical-fields.c

+++ b/lib/logical-fields.c

@@ -123,10 +123,13 @@ ovn_init_symtab(struct shash *symtab)

                              flags_str);

     /* Connection tracking state. */

-    expr_symtab_add_field(symtab, "ct_mark", MFF_CT_MARK, NULL, false);

+    expr_symtab_add_field_scoped(symtab, "ct_mark", MFF_CT_MARK, NULL, false,

+                                 WR_CT_COMMIT);

-    expr_symtab_add_field(symtab, "ct_label", MFF_CT_LABEL, NULL, false);

-    expr_symtab_add_subfield(symtab, "ct_label.blocked", NULL, "ct_label[0]");

+    expr_symtab_add_field_scoped(symtab, "ct_label", MFF_CT_LABEL, NULL, false,

+                                 WR_CT_COMMIT);

+    expr_symtab_add_subfield_scoped(symtab, "ct_label.blocked", NULL,

+                                    "ct_label[0]", WR_CT_COMMIT);

     expr_symtab_add_field(symtab, "ct_state", MFF_CT_STATE, NULL, false);

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c

index 6375aee8d..44e7d9365 100644

--- a/northd/ovn-northd.c

+++ b/northd/ovn-northd.c

@@ -5356,7 +5356,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,

             ds_clear(&match);

             ds_clear(&actions);

             ds_put_cstr(&match, "ct.est && ct_label.blocked == 0");

-            ds_put_cstr(&actions, "ct_commit(ct_label=1/1); ");

+            ds_put_cstr(&actions, "ct_commit { ct_label.blocked = 1; }; ");

             if (!strcmp(acl->action, "reject")) {

                 build_reject_acl_rules(od, lflows, stage, acl, &match,

                                        &actions, &acl->header_);

@@ -5880,9 +5880,11 @@ build_stateful(struct ovn_datapath *od, struct hmap *lflows, struct hmap *lbs)

      * any packet that makes it this far is part of a connection we

      * want to allow to continue. */

     ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,

-                  REGBIT_CONNTRACK_COMMIT" == 1", "ct_commit(ct_label=0/1); next;");

+                  REGBIT_CONNTRACK_COMMIT" == 1",

+                  "ct_commit { ct_label.blocked = 0; }; next;");

     ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100,

-                  REGBIT_CONNTRACK_COMMIT" == 1", "ct_commit(ct_label=0/1); next;");

+                  REGBIT_CONNTRACK_COMMIT" == 1",

+                  "ct_commit { ct_label.blocked = 0; }; next;");

     /* If REGBIT_CONNTRACK_NAT is set as 1, then packets should just be sent

      * through nat (without committing).

diff --git a/ovn-sb.xml b/ovn-sb.xml

index fc39b2d03..a74d9c3ea 100644

--- a/ovn-sb.xml

+++ b/ovn-sb.xml

@@ -1261,10 +1261,10 @@

-        
ct_commit;

-        
ct_commit(ct_mark=value[/mask]);

-        
ct_commit(ct_label=value[/mask]);

-        
ct_commit(ct_mark=value[/mask], ct_label=value[/mask]);

+        
ct_commit { };

+        
ct_commit { ct_mark=value[/mask]; };

+        
ct_commit { ct_label=value[/mask]; };

+        
ct_commit { ct_mark=value[/mask]; ct_label=value[/mask]; };

             Commit the flow to the connection tracking entry associated with it

@@ -1276,6 +1276,9 @@

             tracking entry. ct_mark is a 32-bit field.

             ct_label is a 128-bit field. The value[/mask]

             should be specified in hex string if more than 64bits are to be used.

+            Registers and other named fields can be used for value.

+            ct_mark and ct_label may be sub-addressed

+            in order to have specific bits set.

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

index 905112a8d..4c68b77d8 100644

--- a/tests/ovn.at

+++ b/tests/ovn.at

@@ -1045,51 +1045,60 @@ ct_next;

     has prereqs ip

 # ct_commit

-ct_commit;

+ct_commit { };

+    formats as ct_commit { drop; };

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

     has prereqs ip

-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);

+ct_commit { ct_mark=1; };

+    formats as ct_commit { ct_mark = 1; };

     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);

+ct_commit { ct_mark=1/1; };

+    formats as ct_commit { ct_mark = 1/1; };

     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);

+ct_commit { ct_label=1; };

+    formats as ct_commit { ct_label = 1; };

     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);

+ct_commit { ct_label=1/1; };

+    formats as ct_commit { ct_label = 1/1; };

     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);

+ct_commit { ct_mark=1; ct_label=2; };

+    formats as ct_commit { ct_mark = 1; ct_label = 2; };

     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);

+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);

+ct_commit { ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000; };

+    formats as 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);

+ct_commit { ct_label=18446744073709551615; };

+    formats as ct_commit { ct_label = 18446744073709551615; };

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

     has prereqs ip

-ct_commit(ct_label=18446744073709551616);

+ct_commit { ct_label[0..47] = 0x00000f040201; ct_label[48..63] = 0x0002; };

+    formats as ct_commit { ct_label[0..47] = 0xf040201; ct_label[48..63] = 0x2; };

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

+    has prereqs ip

+ct_commit { ct_label=18446744073709551616; };

     Decimal constants must be less than 2**64.

+ct_commit { ct_label=0x181716151413121110090807060504030201; };

+    141-bit constant is not compatible with 128-bit field ct_label.

+ct_commit { ip4.dst = 192.168.0.1; };

+    Field ip4.dst is not modifiable.

+

+ct_mark = 12345

+    Field ct_mark is not modifiable.

+ct_label = 0xcafe

+    Field ct_label is not modifiable.

+ct_label.blocked = 1/1

+    Field ct_label.blocked is not modifiable.

 # ct_dnat

 ct_dnat;

-- 

2.26.2