[ovs-dev] northd, controller: Commit flows dropped by ACLs to conntrack

This patch commits connections dropped/rejected by ACLs with label
(introduced in 0e0228be (northd: Add ACL label)) to the connection
tracking table. The dropped connections are committed in a separate
conntrack zone so that they can be managed independently and do not
interact with the connection tracking state of allowed connections.

Each logical switch is assigned a new conntrack zone for committing
dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE.
A new lflow action "ct_commit_drop" is introduced that commits flows
to connection tracking table in a zone identified by
MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action
and non-empty label translates to include "ct_commit_drop" in its
actions instead of simply dropping/rejecting the packet.

This provides a new approach to identify connections dropped by ACLs
besides the existing ACL logging and drop sampling approaches.

Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com>
---
 controller/ovn-controller.c  |  14 +++--
 controller/physical.c        |  32 ++++++++++-
 include/ovn/actions.h        |   1 +
 include/ovn/logical-fields.h |   1 +
 lib/actions.c                |  65 ++++++++++++++++++++++
 lib/ovn-util.c               |   4 +-
 lib/ovn-util.h               |   2 +-
 northd/northd.c              |  19 ++++++-
 northd/ovn-northd.8.xml      |  18 ++++++-
 ovn-nb.xml                   |   8 ++-
 ovn-sb.xml                   |  22 ++++++++
 tests/ovn-nbctl.at           |  10 ++--
 tests/ovn-northd.at          | 102 +++++++++++++++++++++--------------
 tests/ovn.at                 |  90 ++++++++++++++++++++++++++++++-
 utilities/ovn-nbctl.c        |   7 ---
 15 files changed, 323 insertions(+), 72 deletions(-)

References:  <20230113124423.242017-1-sangana.abhiram@nutanix.com>
 

Bleep bloop.  Greetings Abhiram Sangana, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 82 characters long (recommended limit is 79)
#412 FILE: ovn-sb.xml:1412:
        <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; };</code></dt>

WARNING: Line is 83 characters long (recommended limit is 79)
#413 FILE: ovn-sb.xml:1413:
        <dt><code>ct_commit_drop { ct_label=<var>value[/mask]</var>; };</code></dt>

WARNING: Line is 116 characters long (recommended limit is 79)
#414 FILE: ovn-sb.xml:1414:
        <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; ct_label=<var>value[/mask]</var>; };</code></dt>

Lines checked: 764, Warnings: 3, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

Hello Abhiram,

I haven't taken a close look at every line of the series, but I have two 
high-level questions/observations.

First, what is the benefit of using this compared to ACL logging or drop 
sampling?

Second, I think that if we are to accept this patch, the behavior needs 
to be optional, and it needs to be opt-in. In other words, someone needs 
to explicitly enable the behavior on the logical switch in order to 
commit dropped connections to CT. If the goal is to use this as a 
debugging tool, it should only be enabled when attempting to debug. I'm 
not knowledgeable about the inner workings of CT, but committing every 
dropped connection to CT sounds like a vector for a DOS exploit to me. 
Someone else can comment in case I'm completely wrong here.

Thanks,
Mark Michelson

On 1/13/23 07:44, Abhiram Sangana wrote:
> This patch commits connections dropped/rejected by ACLs with label
> (introduced in 0e0228be (northd: Add ACL label)) to the connection
> tracking table. The dropped connections are committed in a separate
> conntrack zone so that they can be managed independently and do not
> interact with the connection tracking state of allowed connections.
> 
> Each logical switch is assigned a new conntrack zone for committing
> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE.
> A new lflow action "ct_commit_drop" is introduced that commits flows
> to connection tracking table in a zone identified by
> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action
> and non-empty label translates to include "ct_commit_drop" in its
> actions instead of simply dropping/rejecting the packet.
> 
> This provides a new approach to identify connections dropped by ACLs
> besides the existing ACL logging and drop sampling approaches.
> 
> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com>
> ---
>   controller/ovn-controller.c  |  14 +++--
>   controller/physical.c        |  32 ++++++++++-
>   include/ovn/actions.h        |   1 +
>   include/ovn/logical-fields.h |   1 +
>   lib/actions.c                |  65 ++++++++++++++++++++++
>   lib/ovn-util.c               |   4 +-
>   lib/ovn-util.h               |   2 +-
>   northd/northd.c              |  19 ++++++-
>   northd/ovn-northd.8.xml      |  18 ++++++-
>   ovn-nb.xml                   |   8 ++-
>   ovn-sb.xml                   |  22 ++++++++
>   tests/ovn-nbctl.at           |  10 ++--
>   tests/ovn-northd.at          | 102 +++++++++++++++++++++--------------
>   tests/ovn.at                 |  90 ++++++++++++++++++++++++++++++-
>   utilities/ovn-nbctl.c        |   7 ---
>   15 files changed, 323 insertions(+), 72 deletions(-)
> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 351cf1c5b..c19b56838 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -734,8 +734,8 @@ update_ct_zones(const struct shash *binding_lports,
>       HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
>           /* XXX Add method to limit zone assignment to logical router
>            * datapaths with NAT */
> -        char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat");
> -        char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat");
> +        char *dnat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "dnat");
> +        char *snat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "snat");
>           sset_add(&all_users, dnat);
>           sset_add(&all_users, snat);
>   
> @@ -745,6 +745,14 @@ update_ct_zones(const struct shash *binding_lports,
>           }
>           free(dnat);
>           free(snat);
> +
> +        /* Zone for committing connections dropped by ACLs with labels. */
> +        if (ld->is_switch) {
> +            char *drop = alloc_ct_zone_key(
> +                &ld->datapath->header_.uuid, "drop");
> +            sset_add(&all_users, drop);
> +            free(drop);
> +        }
>       }
>   
>       /* Delete zones that do not exist in above sset. */
> @@ -2415,7 +2423,7 @@ ct_zones_datapath_binding_handler(struct engine_node *node, void *data)
>           /* Check if the requested snat zone has changed for the datapath
>            * or not.  If so, then fall back to full recompute of
>            * ct_zone engine. */
> -        char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, "snat");
> +        char *snat_dp_zone_key = alloc_ct_zone_key(&dp->header_.uuid, "snat");
>           struct simap_node *simap_node = simap_find(&ct_zones_data->current,
>                                                      snat_dp_zone_key);
>           free(snat_dp_zone_key);
> diff --git a/controller/physical.c b/controller/physical.c
> index 4dcf44e01..3c573c492 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -60,6 +60,7 @@ struct zone_ids {
>       int ct;                     /* MFF_LOG_CT_ZONE. */
>       int dnat;                   /* MFF_LOG_DNAT_ZONE. */
>       int snat;                   /* MFF_LOG_SNAT_ZONE. */
> +    int drop;                   /* MFF_LOG_ACL_DROP_ZONE. */
>   };
>   
>   struct tunnel {
> @@ -204,14 +205,18 @@ get_zone_ids(const struct sbrec_port_binding *binding,
>   
>       const struct uuid *key = &binding->datapath->header_.uuid;
>   
> -    char *dnat = alloc_nat_zone_key(key, "dnat");
> +    char *dnat = alloc_ct_zone_key(key, "dnat");
>       zone_ids.dnat = simap_get(ct_zones, dnat);
>       free(dnat);
>   
> -    char *snat = alloc_nat_zone_key(key, "snat");
> +    char *snat = alloc_ct_zone_key(key, "snat");
>       zone_ids.snat = simap_get(ct_zones, snat);
>       free(snat);
>   
> +    char *drop = alloc_ct_zone_key(key, "drop");
> +    zone_ids.drop = simap_get(ct_zones, drop);
> +    free(drop);
> +
>       return zone_ids;
>   }
>   
> @@ -830,6 +835,9 @@ put_zones_ofpacts(const struct zone_ids *zone_ids, struct ofpbuf *ofpacts_p)
>           if (zone_ids->snat) {
>               put_load(zone_ids->snat, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
>           }
> +        if (zone_ids->drop) {
> +            put_load(zone_ids->drop, MFF_LOG_ACL_DROP_ZONE, 0, 32, ofpacts_p);
> +        }
>       }
>   }
>   
> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key,
>                       pb->header_.uuid.parts[0], &match, ofpacts_p,
>                       &pb->header_.uuid);
>   
> +    if (zone_ids->drop) {
> +        /* Table 39, Priority 1.
> +         * =======================
> +         *
> +         * Clear the logical registers (for consistent behavior with packets
> +         * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */
> +        match_init_catchall(&match);
> +        ofpbuf_clear(ofpacts_p);
> +        match_set_metadata(&match, htonll(dp_key));
> +        for (int i = 0; i < MFF_N_LOG_REGS; i++) {
> +            if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) {
> +                put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p);
> +            }
> +        }
> +        put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p);
> +        ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 1,
> +                        pb->datapath->header_.uuid.parts[0], &match,
> +                        ofpacts_p, &pb->datapath->header_.uuid);
> +    }
> +
>       /* Table 39, Priority 100.
>        * =======================
>        *
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 6ca08b3c6..c7a6785d3 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -125,6 +125,7 @@ struct ovn_extend_table;
>       OVNACT(COMMIT_LB_AFF,     ovnact_commit_lb_aff)   \
>       OVNACT(CHK_LB_AFF,        ovnact_result)          \
>       OVNACT(SAMPLE,            ovnact_sample)          \
> +    OVNACT(CT_COMMIT_DROP,    ovnact_nest)            \
>   
>   /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
>   enum OVS_PACKED_ENUM ovnact_type {
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index a7b64ef67..c7ea9e0ce 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -47,6 +47,7 @@ enum ovn_controller_event {
>   #define MFF_LOG_REG0             MFF_REG0
>   #define MFF_LOG_LB_ORIG_DIP_IPV4 MFF_REG1
>   #define MFF_LOG_LB_ORIG_TP_DPORT MFF_REG2
> +#define MFF_LOG_ACL_DROP_ZONE    MFF_REG9
>   
>   #define MFF_LOG_XXREG0           MFF_XXREG0
>   #define MFF_LOG_LB_ORIG_DIP_IPV6 MFF_XXREG1
> diff --git a/lib/actions.c b/lib/actions.c
> index 2da5a696b..d86b1efbc 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -5210,6 +5210,69 @@ encode_CHK_LB_AFF(const struct ovnact_result *res,
>                              MLF_USE_LB_AFF_SESSION_BIT, ofpacts);
>   }
>   
> +static void
> +parse_ct_commit_drop(struct action_context *ctx)
> +{
> +    if (ctx->lexer->token.type == LEX_T_LCURLY) {
> +        parse_nested_action(ctx, OVNACT_CT_COMMIT_DROP, "ip",
> +                            WR_CT_COMMIT);
> +    } else {
> +        /* Add an empty nested action to allow for "ct_commit_drop;" syntax */
> +        add_prerequisite(ctx, "ip");
> +        struct ovnact_nest *on = ovnact_put(ctx->ovnacts,
> +                                            OVNACT_CT_COMMIT_DROP,
> +                                            OVNACT_ALIGN(sizeof *on));
> +        on->nested_len = 0;
> +        on->nested = NULL;
> +    }
> +}
> +
> +static void
> +format_CT_COMMIT_DROP(const struct ovnact_nest *on, struct ds *s)
> +{
> +    if (on->nested_len) {
> +        format_nested_action(on, "ct_commit_drop", s);
> +    } else {
> +        ds_put_cstr(s, "ct_commit_drop;");
> +    }
> +}
> +
> +static void
> +encode_CT_COMMIT_DROP(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_ACL_DROP_ZONE);
> +    ct->zone_src.ofs = 0;
> +    ct->zone_src.n_bits = 16;
> +
> +    /* If the datapath supports all-zero SNAT then use it to avoid tuple
> +     * collisions at commit time between NATed and firewalled-only sessions.
> +     */
> +    if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) {
> +        size_t nat_offset = ofpacts->size;
> +        ofpbuf_pull(ofpacts, nat_offset);
> +
> +        struct ofpact_nat *nat = ofpact_put_NAT(ofpacts);
> +        nat->flags = 0;
> +        nat->range_af = AF_UNSPEC;
> +        nat->flags |= NX_NAT_F_SRC;
> +        ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> +        ct = ofpacts->header;
> +    }
> +
> +    size_t set_field_offset = ofpacts->size;
> +    ofpbuf_pull(ofpacts, set_field_offset);
> +
> +    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);
> +}
> +
>   /* Parses an assignment or exchange or put_dhcp_opts action. */
>   static void
>   parse_set_action(struct action_context *ctx)
> @@ -5415,6 +5478,8 @@ parse_action(struct action_context *ctx)
>           parse_commit_lb_aff(ctx, ovnact_put_COMMIT_LB_AFF(ctx->ovnacts));
>       } else if (lexer_match_id(ctx->lexer, "sample")) {
>           parse_sample(ctx);
> +    } else if (lexer_match_id(ctx->lexer, "ct_commit_drop")) {
> +        parse_ct_commit_drop(ctx);
>       } else {
>           lexer_syntax_error(ctx->lexer, "expecting action");
>       }
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index f3665b89f..646aff6ef 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -443,12 +443,12 @@ split_addresses(const char *addresses, struct svec *ipv4_addrs,
>       destroy_lport_addresses(&laddrs);
>   }
>   
> -/* Allocates a key for NAT conntrack zone allocation for a provided
> +/* Allocates a key for conntrack zone allocation for a provided
>    * 'key' record and a 'type'.
>    *
>    * It is the caller's responsibility to free the allocated memory. */
>   char *
> -alloc_nat_zone_key(const struct uuid *key, const char *type)
> +alloc_ct_zone_key(const struct uuid *key, const char *type)
>   {
>       return xasprintf(UUID_FMT"_%s", UUID_ARGS(key), type);
>   }
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index cd19919cb..170685872 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -92,7 +92,7 @@ const char *find_lport_address(const struct lport_addresses *laddrs,
>   void split_addresses(const char *addresses, struct svec *ipv4_addrs,
>                        struct svec *ipv6_addrs);
>   
> -char *alloc_nat_zone_key(const struct uuid *key, const char *type);
> +char *alloc_ct_zone_key(const struct uuid *key, const char *type);
>   
>   const char *default_nb_db(void);
>   const char *default_sb_db(void);
> diff --git a/northd/northd.c b/northd/northd.c
> index 4751feab4..10a905e51 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -310,7 +310,7 @@ enum ovn_stage {
>    * +----+----------------------------------------------+---+-----------------------------------+
>    * | R8 |              LB_AFF_MATCH_PORT               |
>    * +----+----------------------------------------------+
> - * | R9 |                   UNUSED                     |
> + * | R9 |              ACL DROP CT ZONE                |
>    * +----+----------------------------------------------+
>    *
>    * Logical Router pipeline:
> @@ -6463,6 +6463,14 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
>               ds_clear(match);
>               ds_clear(actions);
>               ds_put_cstr(match, REGBIT_ACL_HINT_DROP " == 1");
> +            /* If the ACL has a label, we commit the packet in
> +             * a separate zone for debugging purposes before
> +             * rejecting/dropping it. */
> +            if (acl->label) {
> +                ds_put_format(actions, "ct_commit_drop { "
> +                              "ct_label.label = %"PRId64"; }; ",
> +                              acl->label);
> +            }
>               if (!strcmp(acl->action, "reject")) {
>                   build_reject_acl_rules(od, lflows, stage, acl, match,
>                                          actions, &acl->header_, meter_groups);
> @@ -6489,8 +6497,15 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
>               ds_clear(match);
>               ds_clear(actions);
>               ds_put_cstr(match, REGBIT_ACL_HINT_BLOCK " == 1");
> -            ds_put_format(actions, "ct_commit { %s = 1; }; ",
> +            ds_put_format(actions, "ct_commit { %s = 1; ",
>                             ct_blocked_match);
> +            /* Update ct_label.label to reflect the new policy matching
> +             * the connection. */
> +            if (acl->label) {
> +                ds_put_format(actions, "ct_label.label = %"PRId64"; ",
> +                              acl->label);
> +            }
> +            ds_put_cstr(actions, "}; ");
>               if (!strcmp(acl->action, "reject")) {
>                   build_reject_acl_rules(od, lflows, stage, acl, match,
>                                          actions, &acl->header_, meter_groups);
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 25a742c90..bcd07463d 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -727,13 +727,20 @@
>           action for TCP connections,<code>icmp4/icmp6</code> action
>           for UDP connections, and <code>sctp_abort {output &lt;-%gt; inport;
>           next(pipeline=egress,table=5);}</code> action for SCTP associations.
> +        Additionally, if the ACL has a <code>label</code>, the translated
> +        action includes <code>ct_commit_drop(ct_label.label=label)</code>.
>         </li>
>         <li>
>           Other ACLs translate to <code>drop;</code> for new or untracked
>           connections and <code>ct_commit(ct_label=1/1);</code> for known
>           connections.  Setting <code>ct_label</code> marks a connection
>           as one that was previously allowed, but should no longer be
> -        allowed due to a policy change.
> +        allowed due to a policy change. If the ACLs have <code>label</code>,
> +        then they instead translate to
> +        <code>ct_commit_drop(ct_label.label=label)</code> for new and
> +        untracked connections and
> +        <code>ct_commit(ct_label.blocked=1; ct_label.label=label);</code>
> +        for known connections.
>         </li>
>       </ul>
>   
> @@ -1077,13 +1084,20 @@
>           action for TCP connections,<code>icmp4/icmp6</code> action
>           for UDP connections, and <code>sctp_abort {output &lt;-%gt; inport;
>           next(pipeline=egress,table=5);}</code> action for SCTP associations.
> +        Additionally, if the ACL has a <code>label</code>, the translated
> +        action includes <code>ct_commit_drop(ct_label.label=label)</code>.
>         </li>
>         <li>
>           Other apply-after-lb ACLs translate to <code>drop;</code> for new
>           or untracked connections and <code>ct_commit(ct_label=1/1);</code> for
>           known connections.  Setting <code>ct_label</code> marks a connection
>           as one that was previously allowed, but should no longer be
> -        allowed due to a policy change.
> +        allowed due to a policy change. If the ACLs have <code>label</code>,
> +        then they instead translate to
> +        <code>ct_commit_drop(ct_label.label=label)</code> for new and
> +        untracked connections and
> +        <code>ct_commit(ct_label.blocked=1; ct_label.label=label);</code>
> +        for known connections.
>         </li>
>       </ul>
>   
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 1ba19e5f5..61389da8a 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2093,12 +2093,10 @@ or
>         <p>
>           Associates an identifier with the ACL.
>           The same value will be written to corresponding connection
> -        tracker entry. The value should be a valid 32-bit unsigned integer.
> -        This value can help in debugging from connection tracker side.
> +        tracking entry. The value should be a valid 32-bit unsigned integer.
> +        This value can help in debugging from connection tracking side.
>           For example, through this "label" we can backtrack to the ACL rule
> -        which is causing a "leaked" connection. Connection tracker entries are
> -        created only for allowed connections so the label is valid only
> -        for allow and allow-related actions.
> +        which is causing a "leaked" connection.
>         </p>
>       </column>
>       <column name="priority">
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index a77f8f4ef..f01f9a927 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -1408,6 +1408,28 @@
>             </p>
>           </dd>
>   
> +        <dt><code>ct_commit_drop { };</code></dt>
> +        <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; };</code></dt>
> +        <dt><code>ct_commit_drop { ct_label=<var>value[/mask]</var>; };</code></dt>
> +        <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; ct_label=<var>value[/mask]</var>; };</code></dt>
> +        <dd>
> +          <p>
> +            Commit the flow to a connection tracking entry in the
> +            logical switch DROP zone. This action is identical to
> +            <code>ct_commit</code> besides the connection tracking entry
> +            the flow is committed to.
> +          </p>
> +
> +          <p>
> +            This action is useful to commit connection tracking state for
> +            packets before they are dropped for debugging purposes.
> +            Committing the packets in a separate zone ensures that connection
> +            tracking state of dropped connections doesn't interact with the
> +            state of allowed connections and allows the DROP zone to be
> +            managed separately.
> +          </p>
> +        </dd>
> +
>           <dt><code>ct_dnat;</code></dt>
>           <dt><code>ct_dnat(<var>IP</var>);</code></dt>
>           <dd>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 8885ac9fc..b5a177563 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -222,17 +222,14 @@ ovn_nbctl_test_acl() {
>      AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 200 ip drop])
>      AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop])
>      AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 from-lport 70 icmp allow-related])
> -   AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp allow-related])
> +   AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp reject])
> +   AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip drop])
>   
>      dnl Add duplicated ACL
>      AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop], [1], [], [stderr])
>      AT_CHECK([grep 'already existed' stderr], [0], [ignore])
>      AT_CHECK([ovn-nbctl $2 --may-exist acl-add $1 to-lport 100 ip drop])
>   
> -   dnl Add invalid ACL label
> -   AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip drop], [1], [], [stderr])
> -   AT_CHECK([grep 'can only be set with actions' stderr], [0], [ignore])
> -
>      AT_CHECK([ovn-nbctl $2 --label=abcd acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr])
>      AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore])
>   
> @@ -250,7 +247,8 @@ from-lport    70 (icmp) allow-related label=1234
>     to-lport   500 (udp) drop log(name=test,severity=info)
>     to-lport   300 (tcp) drop
>     to-lport   100 (ip) drop
> -  to-lport    70 (icmp) allow-related label=1235
> +  to-lport    70 (icmp) reject label=1235
> +  to-lport    50 (ip) drop label=1234
>   ])
>   
>      dnl Delete in one direction.
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 56c1e6c2e..7b319be9d 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -4250,87 +4250,109 @@ check ovn-nbctl ls-add sw0
>   check ovn-nbctl lsp-add sw0 sw0p1
>   
>   check ovn-nbctl --wait=sb --label=1234 acl-add sw0 to-lport 1002 tcp allow-related
> +check ovn-nbctl --wait=sb --label=2345 acl-add sw0 to-lport 1001 ip drop
>   check ovn-nbctl --wait=sb --label=1234 acl-add sw0 from-lport 1002 tcp allow-related
> +check ovn-nbctl --wait=sb --label=2345 acl-add sw0 from-lport 1001 ip reject
>   
>   ovn-sbctl dump-flows sw0 > sw0flows
>   AT_CAPTURE_FILE([sw0flows])
>   
> -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table=?/'], [0], [dnl
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[10]] == 1) && ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; };  reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[9]] == 1) && ip), action=(ct_commit_drop { ct_label.label = 2345; };  reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
>   ])
> -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
>     table=??(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
>     table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
>     table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
>   ])
>   
> -AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[10]] == 1 && (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[9]] == 1 && (ip)), action=(ct_commit_drop { ct_label.label = 2345; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
>   ])
> -AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> -  table=7 (ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
>   ])
>   
> -# Add new ACL without label
> +# Add ACLs without labels
>   check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 udp allow-related
> +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1001 ip6 drop
>   check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 udp allow-related
> +check ovn-nbctl --wait=sb acl-add sw0 from-lport 1001 ip6 reject
>   
>   ovn-sbctl dump-flows sw0 > sw0flows
>   AT_CAPTURE_FILE([sw0flows])
>   
> -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table=?/'], [0], [dnl
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
> -])
> -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[10]] == 1) && ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; };  reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[10]] == 1) && ip6), action=(ct_commit { ct_mark.blocked = 1; };  reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[9]] == 1) && ip), action=(ct_commit_drop { ct_label.label = 2345; };  reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[9]] == 1) && ip6), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
> +])
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
>     table=??(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
>     table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
>     table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
>   ])
>   
> -AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[10]] == 1 && (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[10]] == 1 && (ip6)), action=(ct_commit { ct_mark.blocked = 1; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[9]] == 1 && (ip)), action=(ct_commit_drop { ct_label.label = 2345; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[9]] == 1 && (ip6)), action=(/* drop */)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
>   ])
> -AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> -  table=7 (ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
>   ])
>   
> -# Delete new ACL with label
> +# Delete ACLs with labels
>   check ovn-nbctl --wait=sb acl-del sw0 to-lport 1002 tcp
> +check ovn-nbctl --wait=sb acl-del sw0 to-lport 1001 ip
>   check ovn-nbctl --wait=sb acl-del sw0 from-lport 1002 tcp
> +check ovn-nbctl --wait=sb acl-del sw0 from-lport 1001 ip
>   
>   ovn-sbctl dump-flows sw0 > sw0flows
>   AT_CAPTURE_FILE([sw0flows])
>   
> -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table=?/'], [0], [dnl
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[10]] == 1) && ip6), action=(ct_commit { ct_mark.blocked = 1; };  reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[9]] == 1) && ip6), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
>   ])
> -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
>     table=??(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
>     table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
>     table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
>   ])
>   
> -AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[10]] == 1 && (ip6)), action=(ct_commit { ct_mark.blocked = 1; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[9]] == 1 && (ip6)), action=(/* drop */)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
>   ])
> -AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> -  table=7 (ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
>   ])
>   AT_CLEANUP
>   ])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index c537cba54..ad14a843a 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1319,6 +1319,58 @@ ct_label = 0xcafe
>   ct_label.blocked = 1/1
>       Field ct_label.blocked is not modifiable.
>   
> +# ct_commit_drop
> +ct_commit_drop;
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15])
> +    has prereqs ip
> +ct_commit_drop { };
> +    formats as ct_commit_drop;
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15])
> +    has prereqs ip
> +ct_commit_drop { ct_mark=1; };
> +    formats as ct_commit_drop { ct_mark = 1; };
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_mark))
> +    has prereqs ip
> +ct_commit_drop { ct_mark=1/1; };
> +    formats as ct_commit_drop { ct_mark = 1/1; };
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1/0x1->ct_mark))
> +    has prereqs ip
> +ct_commit_drop { ct_label=1; };
> +    formats as ct_commit_drop { ct_label = 1; };
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_label=1/1; };
> +    formats as ct_commit_drop { ct_label = 1/1; };
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1/0x1->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_mark=1; ct_label=2; };
> +    formats as ct_commit_drop { ct_mark = 1; ct_label = 2; };
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label))
> +    has prereqs ip
> +
> +ct_commit_drop { ct_label=0x01020304050607080910111213141516; };
> +    formats as ct_commit_drop { ct_label = 0x1020304050607080910111213141516; };
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1020304050607080910111213141516->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000; };
> +    formats as ct_commit_drop { ct_label = 0x1000000000000000000000000000000/0x1000000000000000000000000000000; };
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_label=18446744073709551615; };
> +    formats as ct_commit_drop { ct_label = 18446744073709551615; };
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0xffffffffffffffff->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_label[0..47] = 0x00000f040201; ct_label[48..63] = 0x0002; };
> +    formats as ct_commit_drop { ct_label[0..47] = 0xf040201; ct_label[48..63] = 0x2; };
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0xf040201/0xffffffffffff->ct_label,set_field:0x2000000000000/0xffff000000000000->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_label=18446744073709551616; };
> +    Decimal constants must be less than 2**64.
> +ct_commit_drop { ct_label=0x181716151413121110090807060504030201; };
> +    141-bit constant is not compatible with 128-bit field ct_label.
> +ct_commit_drop { ip4.dst = 192.168.0.1; };
> +    Field ip4.dst is not modifiable.
> +
>   # ct_dnat
>   ct_dnat;
>       encodes as ct(table=19,zone=NXM_NX_REG11[0..15],nat)
> @@ -33556,7 +33608,7 @@ ovn-nbctl lsp-add sw0 sw0-port1 -- lsp-set-addresses sw0-port1 "50:54:00:00:00:0
>   ovs-vsctl set interface p0 external-ids:iface-id=sw0-port0
>   ovs-vsctl set interface p1 external-ids:iface-id=sw0-port1
>   check ovn-nbctl --wait=hv sync
> -as hv1 ovs-ofctl dump-flows br-int  | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG1/load:0x?->NXM_NX_REG1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows1
> +as hv1 ovs-ofctl dump-flows br-int  | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG\([[19]]\)/load:0x?->NXM_NX_REG\1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows1
>   
>   ovs-vsctl set interface p0 external-ids:iface-id=foo0
>   ovs-vsctl set interface p1 external-ids:iface-id=foo1
> @@ -33583,7 +33635,7 @@ sleep 0.5
>   # Restart SB
>   AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)])
>   check ovn-nbctl --wait=hv sync
> -as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG1/load:0x?->NXM_NX_REG1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows2
> +as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG\([[19]]\)/load:0x?->NXM_NX_REG\1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows2
>   AT_CHECK([diff  offlows1 offlows2])
>   
>   ovn-nbctl ls-del sw0
> @@ -34608,3 +34660,37 @@ check_packets
>   OVN_CLEANUP([hv1])
>   AT_CLEANUP
>   ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - check logical_switch drop ct zone])
> +ovn_start
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +check ovn-nbctl ls-add ls0
> +check ovn-nbctl lsp-add ls0 lsp0
> +check ovn-nbctl lsp-set-addresses lsp0 "00:00:00:00:00:02 10.0.0.2"
> +
> +ovs-vsctl -- add-port br-int vif0 -- set interface vif0 external-ids:iface-id=lsp0
> +
> +# Wait for ovn-northd and ovn-controller to catch up.
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +# logical_switch should have a CT zone for connections dropped by labeled ACLs
> +ls_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=ls0)
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller ct-zone-list | \
> +grep ${ls_uuid}_drop -c], [0], [1
> +])
> +
> +# Check that register storing the drop ct zone is not cleared.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=39 | grep -w 'priority=1' | ofctl_strip_all], [0], [dnl
> + table=39, priority=1,metadata=0x1 actions=load:0->NXM_NX_REG0[[]],load:0->NXM_NX_REG1[[]],load:0->NXM_NX_REG2[[]],load:0->NXM_NX_REG3[[]],load:0->NXM_NX_REG4[[]],load:0->NXM_NX_REG5[[]],load:0->NXM_NX_REG6[[]],load:0->NXM_NX_REG7[[]],load:0->NXM_NX_REG8[[]],resubmit(,40)
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 9d4fb8c75..6062d8abd 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -2360,13 +2360,6 @@ nbctl_acl_add(struct ctl_context *ctx)
>       /* Set the ACL label */
>       const char *label = shash_find_data(&ctx->options, "--label");
>       if (label) {
> -      /* Ensure that the action is either allow or allow-related */
> -      if (strcmp(action, "allow") && strcmp(action, "allow-related")) {
> -        ctl_error(ctx, "label can only be set with actions \"allow\" or "
> -                  "\"allow-related\"");
> -        return;
> -      }
> -
>         int64_t label_value = 0;
>         error = parse_acl_label(label, &label_value);
>         if (error) {

Hi Mark,

Thanks for reviewing the patch.

> On 16 Jan 2023, at 21:34, Mark Michelson <mmichels@redhat.com> wrote:
> 
> Hello Abhiram,
> 
> I haven't taken a close look at every line of the series, but I have two high-level questions/observations.
> 
> First, what is the benefit of using this compared to ACL logging or drop sampling?

I had done some basic testing on ACL logging and exporting Netflow
records for OVN bridge. I noticed a significant load on ovs-vswitchd
when there are a large number of connections created in the bridge
(70-80% CPU usage with 1000 connections per sec for Netflow and
60-70% CPU with 1000 packets per sec for ACL logging), probably due to
large number of upcalls. I haven't tested specifically with drop
sampling but expected a similar cost if we use 100% sampling. The
alternative is to use metering with ACL logs or sampling part of the
connections before exporting them but we might miss some of the
connections this way.

With the conntrack approach, ovs-vswitchd would not be involved and
CMS can read the CT table or receive connection tracking events from
nf_conntrack kernel module via ctnetlink (a single event is generated
for each connection). We should be able to get all/most of the
connections dropped by ACLs. However, this approach is datapath
specific.

There was brief discussion regarding the same in the RFC thread -
[ovs-dev,RFC] northd, controller: Commit flows dropped by ACLs in a
separate CT zone - 
http://patchwork.ozlabs.org/project/ovn/patch/20221018153342.164530-1-sangana.abhiram@nutanix.com/

> 
> Second, I think that if we are to accept this patch, the behavior needs to be optional, and it needs to be opt-in. In other words, someone needs to explicitly enable the behavior on the logical switch in order to commit dropped connections to CT. If the goal is to use this as a debugging tool, it should only be enabled when attempting to debug. I'm not knowledgeable about the inner workings of CT, but committing every dropped connection to CT sounds like a vector for a DOS exploit to me. Someone else can comment in case I'm completely wrong here.
> 
While the DROP CT zone is created unconditionally for each logical
switch, connections are only committed if the ACL dropping/rejecting
them has label set. So, user can create drop/reject ACLs without
labels (default behavior today) if they don't want to use this
feature.

Yes, committing dropped connections is a potential DOS vector. I am
planning to send another patch that limits the size of the DROP CT
zone - ovs datapath already has support to limit the size of CT zones.
https://github.com/openvswitch/ovs/commit/cb2a5486a3a3756ee3868da0050d737c8989770c

> Thanks,
> Mark Michelson

Thanks,
Abhiram Sangana

> On 1/13/23 07:44, Abhiram Sangana wrote:
>> This patch commits connections dropped/rejected by ACLs with label
>> (introduced in 0e0228be (northd: Add ACL label)) to the connection
>> tracking table. The dropped connections are committed in a separate
>> conntrack zone so that they can be managed independently and do not
>> interact with the connection tracking state of allowed connections.
>> Each logical switch is assigned a new conntrack zone for committing
>> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE.
>> A new lflow action "ct_commit_drop" is introduced that commits flows
>> to connection tracking table in a zone identified by
>> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action
>> and non-empty label translates to include "ct_commit_drop" in its
>> actions instead of simply dropping/rejecting the packet.
>> This provides a new approach to identify connections dropped by ACLs
>> besides the existing ACL logging and drop sampling approaches.
>> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com>

Hi Mark,

I have replied to your comments. Can you please have a look when you get a chance?

Thanks,
Abhiram Sangana

> On 17 Jan 2023, at 12:37, Abhiram Sangana <sangana.abhiram@nutanix.com> wrote:
> 
> Hi Mark,
> 
> Thanks for reviewing the patch.
> 
>> On 16 Jan 2023, at 21:34, Mark Michelson <mmichels@redhat.com> wrote:
>> 
>> Hello Abhiram,
>> 
>> I haven't taken a close look at every line of the series, but I have two high-level questions/observations.
>> 
>> First, what is the benefit of using this compared to ACL logging or drop sampling?
> 
> I had done some basic testing on ACL logging and exporting Netflow
> records for OVN bridge. I noticed a significant load on ovs-vswitchd
> when there are a large number of connections created in the bridge
> (70-80% CPU usage with 1000 connections per sec for Netflow and
> 60-70% CPU with 1000 packets per sec for ACL logging), probably due to
> large number of upcalls. I haven't tested specifically with drop
> sampling but expected a similar cost if we use 100% sampling. The
> alternative is to use metering with ACL logs or sampling part of the
> connections before exporting them but we might miss some of the
> connections this way.
> 
> With the conntrack approach, ovs-vswitchd would not be involved and
> CMS can read the CT table or receive connection tracking events from
> nf_conntrack kernel module via ctnetlink (a single event is generated
> for each connection). We should be able to get all/most of the
> connections dropped by ACLs. However, this approach is datapath
> specific.
> 
> There was brief discussion regarding the same in the RFC thread -
> [ovs-dev,RFC] northd, controller: Commit flows dropped by ACLs in a
> separate CT zone - 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_project_ovn_patch_20221018153342.164530-2D1-2Dsangana.abhiram-40nutanix.com_&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=um7MDrMKGeU4Ozd9VvKIJQ5cVpVizh_lH5ILPVlt2zE&e= 
> 
>> 
>> Second, I think that if we are to accept this patch, the behavior needs to be optional, and it needs to be opt-in. In other words, someone needs to explicitly enable the behavior on the logical switch in order to commit dropped connections to CT. If the goal is to use this as a debugging tool, it should only be enabled when attempting to debug. I'm not knowledgeable about the inner workings of CT, but committing every dropped connection to CT sounds like a vector for a DOS exploit to me. Someone else can comment in case I'm completely wrong here.
>> 
> While the DROP CT zone is created unconditionally for each logical
> switch, connections are only committed if the ACL dropping/rejecting
> them has label set. So, user can create drop/reject ACLs without
> labels (default behavior today) if they don't want to use this
> feature.
> 
> Yes, committing dropped connections is a potential DOS vector. I am
> planning to send another patch that limits the size of the DROP CT
> zone - ovs datapath already has support to limit the size of CT zones.
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_commit_cb2a5486a3a3756ee3868da0050d737c8989770c&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=9jkE_C4UAA8pOWcIzNbymLs0ai6Am90PvwTE1oFjHCc&e= 
> 
>> Thanks,
>> Mark Michelson
> 
> Thanks,
> Abhiram Sangana
> 
>> On 1/13/23 07:44, Abhiram Sangana wrote:
>>> This patch commits connections dropped/rejected by ACLs with label
>>> (introduced in 0e0228be (northd: Add ACL label)) to the connection
>>> tracking table. The dropped connections are committed in a separate
>>> conntrack zone so that they can be managed independently and do not
>>> interact with the connection tracking state of allowed connections.
>>> Each logical switch is assigned a new conntrack zone for committing
>>> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE.
>>> A new lflow action "ct_commit_drop" is introduced that commits flows
>>> to connection tracking table in a zone identified by
>>> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action
>>> and non-empty label translates to include "ct_commit_drop" in its
>>> actions instead of simply dropping/rejecting the packet.
>>> This provides a new approach to identify connections dropped by ACLs
>>> besides the existing ACL logging and drop sampling approaches.
>>> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com>
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=ti2yvyC3UL6J5DeC-5EO7Et54ZMOzaaNNPp02UYpSCc&e=

On 1/25/23 05:36, Abhiram Sangana wrote:
> Hi Mark,
> 
> I have replied to your comments. Can you please have a look when you get a chance?
> 

I had a look at the code itself, and from a purely mechanical 
perspective, I can't see anything wrong with it. I have some high level 
comments down below, though.

> Thanks,
> Abhiram Sangana
> 
>> On 17 Jan 2023, at 12:37, Abhiram Sangana <sangana.abhiram@nutanix.com> wrote:
>>
>> Hi Mark,
>>
>> Thanks for reviewing the patch.
>>
>>> On 16 Jan 2023, at 21:34, Mark Michelson <mmichels@redhat.com> wrote:
>>>
>>> Hello Abhiram,
>>>
>>> I haven't taken a close look at every line of the series, but I have two high-level questions/observations.
>>>
>>> First, what is the benefit of using this compared to ACL logging or drop sampling?
>>
>> I had done some basic testing on ACL logging and exporting Netflow
>> records for OVN bridge. I noticed a significant load on ovs-vswitchd
>> when there are a large number of connections created in the bridge
>> (70-80% CPU usage with 1000 connections per sec for Netflow and
>> 60-70% CPU with 1000 packets per sec for ACL logging), probably due to
>> large number of upcalls. I haven't tested specifically with drop
>> sampling but expected a similar cost if we use 100% sampling. The
>> alternative is to use metering with ACL logs or sampling part of the
>> connections before exporting them but we might miss some of the
>> connections this way.
>>
>> With the conntrack approach, ovs-vswitchd would not be involved and
>> CMS can read the CT table or receive connection tracking events from
>> nf_conntrack kernel module via ctnetlink (a single event is generated
>> for each connection). We should be able to get all/most of the
>> connections dropped by ACLs. However, this approach is datapath
>> specific.
>>
>> There was brief discussion regarding the same in the RFC thread -
>> [ovs-dev,RFC] northd, controller: Commit flows dropped by ACLs in a
>> separate CT zone -
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_project_ovn_patch_20221018153342.164530-2D1-2Dsangana.abhiram-40nutanix.com_&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=um7MDrMKGeU4Ozd9VvKIJQ5cVpVizh_lH5ILPVlt2zE&e=
>>
>>>
>>> Second, I think that if we are to accept this patch, the behavior needs to be optional, and it needs to be opt-in. In other words, someone needs to explicitly enable the behavior on the logical switch in order to commit dropped connections to CT. If the goal is to use this as a debugging tool, it should only be enabled when attempting to debug. I'm not knowledgeable about the inner workings of CT, but committing every dropped connection to CT sounds like a vector for a DOS exploit to me. Someone else can comment in case I'm completely wrong here.
>>>
>> While the DROP CT zone is created unconditionally for each logical
>> switch, connections are only committed if the ACL dropping/rejecting
>> them has label set. So, user can create drop/reject ACLs without
>> labels (default behavior today) if they don't want to use this
>> feature.

IMO, this is not good enough. As a user I would be surprised to find 
that additional entries are added to conntrack simply because I created 
a label for my ACL. I think that another option needs to be created that 
specifically enables committing dropped packets to CT.

>>
>> Yes, committing dropped connections is a potential DOS vector. I am
>> planning to send another patch that limits the size of the DROP CT
>> zone - ovs datapath already has support to limit the size of CT zones.
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_commit_cb2a5486a3a3756ee3868da0050d737c8989770c&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=9jkE_C4UAA8pOWcIzNbymLs0ai6Am90PvwTE1oFjHCc&e=

If the explicit option to enable committing dropped packets to conntrack 
is added, then I think we can move forward without needing this OVS 
patch in place.

>>
>>> Thanks,
>>> Mark Michelson
>>
>> Thanks,
>> Abhiram Sangana
>>
>>> On 1/13/23 07:44, Abhiram Sangana wrote:
>>>> This patch commits connections dropped/rejected by ACLs with label
>>>> (introduced in 0e0228be (northd: Add ACL label)) to the connection
>>>> tracking table. The dropped connections are committed in a separate
>>>> conntrack zone so that they can be managed independently and do not
>>>> interact with the connection tracking state of allowed connections.
>>>> Each logical switch is assigned a new conntrack zone for committing
>>>> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE.
>>>> A new lflow action "ct_commit_drop" is introduced that commits flows
>>>> to connection tracking table in a zone identified by
>>>> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action
>>>> and non-empty label translates to include "ct_commit_drop" in its
>>>> actions instead of simply dropping/rejecting the packet.
>>>> This provides a new approach to identify connections dropped by ACLs
>>>> besides the existing ACL logging and drop sampling approaches.
>>>> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com>
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=ti2yvyC3UL6J5DeC-5EO7Et54ZMOzaaNNPp02UYpSCc&e=
>

> On 3 Feb 2023, at 16:08, Mark Michelson <mmichels@redhat.com> wrote:
> 
> On 1/25/23 05:36, Abhiram Sangana wrote:
>> Hi Mark,
>> I have replied to your comments. Can you please have a look when you get a chance?
> 
> I had a look at the code itself, and from a purely mechanical perspective, I can't see anything wrong with it. I have some high level comments down below, though.
> 
>> Thanks,
>> Abhiram Sangana
>>> On 17 Jan 2023, at 12:37, Abhiram Sangana <sangana.abhiram@nutanix.com> wrote:
>>> 
>>> Hi Mark,
>>> 
>>> Thanks for reviewing the patch.
>>> 
>>>> On 16 Jan 2023, at 21:34, Mark Michelson <mmichels@redhat.com> wrote:
>>>> 
>>>> Hello Abhiram,
>>>> 
>>>> I haven't taken a close look at every line of the series, but I have two high-level questions/observations.
>>>> 
>>>> First, what is the benefit of using this compared to ACL logging or drop sampling?
>>> 
>>> I had done some basic testing on ACL logging and exporting Netflow
>>> records for OVN bridge. I noticed a significant load on ovs-vswitchd
>>> when there are a large number of connections created in the bridge
>>> (70-80% CPU usage with 1000 connections per sec for Netflow and
>>> 60-70% CPU with 1000 packets per sec for ACL logging), probably due to
>>> large number of upcalls. I haven't tested specifically with drop
>>> sampling but expected a similar cost if we use 100% sampling. The
>>> alternative is to use metering with ACL logs or sampling part of the
>>> connections before exporting them but we might miss some of the
>>> connections this way.
>>> 
>>> With the conntrack approach, ovs-vswitchd would not be involved and
>>> CMS can read the CT table or receive connection tracking events from
>>> nf_conntrack kernel module via ctnetlink (a single event is generated
>>> for each connection). We should be able to get all/most of the
>>> connections dropped by ACLs. However, this approach is datapath
>>> specific.
>>> 
>>> There was brief discussion regarding the same in the RFC thread -
>>> [ovs-dev,RFC] northd, controller: Commit flows dropped by ACLs in a
>>> separate CT zone -
>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_project_ovn_patch_20221018153342.164530-2D1-2Dsangana.abhiram-40nutanix.com_&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=um7MDrMKGeU4Ozd9VvKIJQ5cVpVizh_lH5ILPVlt2zE&e=
>>> 
>>>> 
>>>> Second, I think that if we are to accept this patch, the behavior needs to be optional, and it needs to be opt-in. In other words, someone needs to explicitly enable the behavior on the logical switch in order to commit dropped connections to CT. If the goal is to use this as a debugging tool, it should only be enabled when attempting to debug. I'm not knowledgeable about the inner workings of CT, but committing every dropped connection to CT sounds like a vector for a DOS exploit to me. Someone else can comment in case I'm completely wrong here.
>>>> 
>>> While the DROP CT zone is created unconditionally for each logical
>>> switch, connections are only committed if the ACL dropping/rejecting
>>> them has label set. So, user can create drop/reject ACLs without
>>> labels (default behavior today) if they don't want to use this
>>> feature.
> 
> IMO, this is not good enough. As a user I would be surprised to find that additional entries are added to conntrack simply because I created a label for my ACL. I think that another option needs to be created that specifically enables committing dropped packets to CT.

Sure - should we add a global option to commit connections dropped
by ACLs or a per logical_switch option?

Also, given that `label` field was not supported for ACLs with action as
“drop” or “reject” before, should we bump the NB DB schema version?
> 
>>> 
>>> Yes, committing dropped connections is a potential DOS vector. I am
>>> planning to send another patch that limits the size of the DROP CT
>>> zone - ovs datapath already has support to limit the size of CT zones.
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_commit_cb2a5486a3a3756ee3868da0050d737c8989770c&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=9jkE_C4UAA8pOWcIzNbymLs0ai6Am90PvwTE1oFjHCc&e=
> 
> If the explicit option to enable committing dropped packets to conntrack is added, then I think we can move forward without needing this OVS patch in place.

Got it.

Thanks,
Abhiram Sangana
> 
>>> 
>>>> Thanks,
>>>> Mark Michelson
>>> 
>>> Thanks,
>>> Abhiram Sangana
>>> 
>>>> On 1/13/23 07:44, Abhiram Sangana wrote:
>>>>> This patch commits connections dropped/rejected by ACLs with label
>>>>> (introduced in 0e0228be (northd: Add ACL label)) to the connection
>>>>> tracking table. The dropped connections are committed in a separate
>>>>> conntrack zone so that they can be managed independently and do not
>>>>> interact with the connection tracking state of allowed connections.
>>>>> Each logical switch is assigned a new conntrack zone for committing
>>>>> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE.
>>>>> A new lflow action "ct_commit_drop" is introduced that commits flows
>>>>> to connection tracking table in a zone identified by
>>>>> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action
>>>>> and non-empty label translates to include "ct_commit_drop" in its
>>>>> actions instead of simply dropping/rejecting the packet.
>>>>> This provides a new approach to identify connections dropped by ACLs
>>>>> besides the existing ACL logging and drop sampling approaches.
>>>>> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com>
>>> 
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=RotwPXnAckhnfEzdWgnpPR0nnZ46Y0RYo-uUGaS4vXY&m=W3sqrwt4epeEOIKA16LGwz3gbzhOCAyFKDxFAK8KX5lJsEFMpVu09MyJTWxSNIpa&s=ti2yvyC3UL6J5DeC-5EO7Et54ZMOzaaNNPp02UYpSCc&e=