From b74a558d0a5e4c217f966d9615611e3dca1d6c23 Mon Sep 17 00:00:00 2001 From: Mark Michelson 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 --- 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