diff mbox series

[ovs-dev,v3,3/3] netlink, netdev: Enforce CT limit protection.

Message ID 20231002103347.101274-4-amusil@redhat.com
State Changes Requested
Headers show
Series Expose CT limit via DB | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ales Musil Oct. 2, 2023, 10:33 a.m. UTC 
Enforce the CT limit protection, it ensures that
any CT limit value that was set by forced operation,
currently the DB CT limit, will be protected against
overwrite from other sources, e.g. the dpctl command.

Signed-off-by: Ales Musil <amusil@redhat.com>
Acked-by: Simon Horman <horms@ovn.org>
---
v3: Rebase on top of current master.
    Add ack from Simon and fix the missing '.'.
---
 lib/conntrack.c         | 51 ++++++++++++++++++++++++++---------------
 lib/conntrack.h         |  5 ++--
 lib/ct-dpif.c           |  9 ++++----
 lib/ct-dpif.h           |  5 ++--
 lib/dpctl.c             |  4 ++--
 lib/dpif-netdev.c       | 12 ++++++----
 lib/dpif-netlink.c      | 37 ++++++++++++++++++++++++++----
 lib/dpif-provider.h     | 13 +++++++----
 ofproto/ofproto-dpif.c  |  6 +++--
 tests/system-traffic.at | 28 ++++++++++++++++++++++
 10 files changed, 126 insertions(+), 44 deletions(-)

Comments

Ilya Maximets Oct. 5, 2023, 6:50 p.m. UTC | #1 
On 10/2/23 12:33, Ales Musil wrote:
> Enforce the CT limit protection, it ensures that
> any CT limit value that was set by forced operation,
> currently the DB CT limit, will be protected against
> overwrite from other sources, e.g. the dpctl command.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> Acked-by: Simon Horman <horms@ovn.org>
> ---
> v3: Rebase on top of current master.
>     Add ack from Simon and fix the missing '.'.
> ---
>  lib/conntrack.c         | 51 ++++++++++++++++++++++++++---------------
>  lib/conntrack.h         |  5 ++--
>  lib/ct-dpif.c           |  9 ++++----
>  lib/ct-dpif.h           |  5 ++--
>  lib/dpctl.c             |  4 ++--
>  lib/dpif-netdev.c       | 12 ++++++----
>  lib/dpif-netlink.c      | 37 ++++++++++++++++++++++++++----
>  lib/dpif-provider.h     | 13 +++++++----
>  ofproto/ofproto-dpif.c  |  6 +++--
>  tests/system-traffic.at | 28 ++++++++++++++++++++++
>  10 files changed, 126 insertions(+), 44 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 47a443fba..b5b5d4a4c 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -85,6 +85,7 @@ enum ct_alg_ctl_type {
>  struct zone_limit {
>      struct cmap_node node;
>      struct conntrack_zone_limit czl;
> +    bool limit_protected;
>  };
>  
>  static bool conn_key_extract(struct conntrack *, struct dp_packet *,
> @@ -344,17 +345,13 @@ zone_limit_get(struct conntrack *ct, int32_t zone)
>  }
>  
>  static int
> -zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
> +zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit,
> +                  bool limit_protected)
>      OVS_REQUIRES(ct->ct_lock)
>  {
> -    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> -
> -    if (zl) {
> -        return 0;
> -    }
> -
>      if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
> -        zl = xzalloc(sizeof *zl);
> +        struct zone_limit *zl = xzalloc(sizeof *zl);

An empty line after the variable declarations.

> +        zl->limit_protected = limit_protected;
>          zl->czl.limit = limit;
>          zl->czl.zone = zone;
>          zl->czl.zone_limit_seq = ct->zone_limit_seq++;
> @@ -366,18 +363,28 @@ zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
>      }
>  }
>  
> +static inline bool
> +can_update_zone_limit(struct zone_limit *zl, bool force)
> +{
> +    return !(zl && zl->limit_protected && !force);
> +}
> +
>  int
> -zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
> +zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit,
> +                  bool force)
>  {
>      int err = 0;
> -    struct zone_limit *zl = zone_limit_lookup(ct, zone);
> -    if (zl) {
> +    ovs_mutex_lock(&ct->ct_lock);
> +
> +    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> +    if (!can_update_zone_limit(zl, force)) {
> +        err = EPERM;
> +    } else if (zl) {
>          zl->czl.limit = limit;
> +        zl->limit_protected = force;
>          VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
>      } else {
> -        ovs_mutex_lock(&ct->ct_lock);
> -        err = zone_limit_create(ct, zone, limit);
> -        ovs_mutex_unlock(&ct->ct_lock);
> +        err = zone_limit_create(ct, zone, limit, force);
>          if (!err) {
>              VLOG_INFO("Created zone limit of %u for zone %d", limit, zone);
>          } else {
> @@ -385,6 +392,8 @@ zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
>                        zone);
>          }
>      }
> +
> +    ovs_mutex_unlock(&ct->ct_lock);
>      return err;
>  }
>  
> @@ -398,20 +407,24 @@ zone_limit_clean(struct conntrack *ct, struct zone_limit *zl)
>  }
>  
>  int
> -zone_limit_delete(struct conntrack *ct, uint16_t zone)
> +zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force)
>  {
> +    int err = 0;
>      ovs_mutex_lock(&ct->ct_lock);
> +
>      struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> -    if (zl) {
> +    if (!can_update_zone_limit(zl, force)) {
> +        err = EPERM;
> +    } else if (zl) {
>          zone_limit_clean(ct, zl);
> -        ovs_mutex_unlock(&ct->ct_lock);
>          VLOG_INFO("Deleted zone limit for zone %d", zone);
>      } else {
> -        ovs_mutex_unlock(&ct->ct_lock);
>          VLOG_INFO("Attempted delete of non-existent zone limit: zone %d",
>                    zone);
>      }
> -    return 0;
> +
> +    ovs_mutex_unlock(&ct->ct_lock);
> +    return err;
>  }
>  
>  static void
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 57d5159b6..a58a800f9 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -153,7 +153,8 @@ bool conntrack_get_tcp_seq_chk(struct conntrack *ct);
>  struct ipf *conntrack_ipf_ctx(struct conntrack *ct);
>  struct conntrack_zone_limit zone_limit_get(struct conntrack *ct,
>                                             int32_t zone);
> -int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit);
> -int zone_limit_delete(struct conntrack *ct, uint16_t zone);
> +int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit,
> +                      bool force);
> +int zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force);
>  
>  #endif /* conntrack.h */
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index f59c6e560..335ba09f9 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -399,11 +399,11 @@ ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled)
>  
>  int
>  ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
> -                   const struct ovs_list *zone_limits)
> +                   const struct ovs_list *zone_limits, bool force)
>  {
>      return (dpif->dpif_class->ct_set_limits
>              ? dpif->dpif_class->ct_set_limits(dpif, default_limit,
> -                                              zone_limits)
> +                                              zone_limits, force)
>              : EOPNOTSUPP);
>  }
>  
> @@ -420,10 +420,11 @@ ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
>  }
>  
>  int
> -ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *zone_limits)
> +ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *zone_limits,
> +                   bool force)
>  {
>      return (dpif->dpif_class->ct_del_limits
> -            ? dpif->dpif_class->ct_del_limits(dpif, zone_limits)
> +            ? dpif->dpif_class->ct_del_limits(dpif, zone_limits, force)
>              : EOPNOTSUPP);
>  }
>  
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 0b728b529..0b74ec463 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -308,10 +308,11 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns);
>  int ct_dpif_set_tcp_seq_chk(struct dpif *dpif, bool enabled);
>  int ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled);
>  int ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
> -                       const struct ovs_list *);
> +                       const struct ovs_list *, bool enforced);
>  int ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
>                         const struct ovs_list *, struct ovs_list *);
> -int ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *);
> +int ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *,
> +                       bool enforced);
>  int ct_dpif_sweep(struct dpif *, uint32_t *ms);
>  int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
>  int ct_dpif_ipf_set_min_frag(struct dpif *, bool v6, uint32_t min_frag);
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index cd12625a1..e498c29ce 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -2233,7 +2233,7 @@ dpctl_ct_set_limits(int argc, const char *argv[],
>          ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0);
>      }
>  
> -    error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits);
> +    error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits, false);

Hmm.  Just noticed the difference of the default limit configuration.
I would assume that the default limit is the most importnat thing
to be configured.  But we do not have a way to set it via database.
Should we add anorther column into Datapath table for that?  The feature
seems incomplete without it.

>      if (!error) {
>          ct_dpif_free_zone_limits(&zone_limits);
>          dpif_close(dpif);
> @@ -2300,7 +2300,7 @@ dpctl_ct_del_limits(int argc, const char *argv[],
>          goto error;
>      }
>  
> -    error = ct_dpif_del_limits(dpif, &zone_limits);
> +    error = ct_dpif_del_limits(dpif, &zone_limits, false);
>      if (!error) {
>          goto out;
>      } else {
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 157694bcf..90a48baa6 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9447,12 +9447,14 @@ dpif_netdev_ct_get_sweep_interval(struct dpif *dpif, uint32_t *ms)
>  static int
>  dpif_netdev_ct_set_limits(struct dpif *dpif,
>                             const uint32_t *default_limits,
> -                           const struct ovs_list *zone_limits)
> +                           const struct ovs_list *zone_limits,
> +                           bool force)
>  {
>      int err = 0;
>      struct dp_netdev *dp = get_dp_netdev(dpif);
>      if (default_limits) {
> -        err = zone_limit_update(dp->conntrack, DEFAULT_ZONE, *default_limits);
> +        err = zone_limit_update(dp->conntrack, DEFAULT_ZONE, *default_limits,
> +                                force);
>          if (err != 0) {
>              return err;
>          }
> @@ -9461,7 +9463,7 @@ dpif_netdev_ct_set_limits(struct dpif *dpif,
>      struct ct_dpif_zone_limit *zone_limit;
>      LIST_FOR_EACH (zone_limit, node, zone_limits) {
>          err = zone_limit_update(dp->conntrack, zone_limit->zone,
> -                                zone_limit->limit);
> +                                zone_limit->limit, force);
>          if (err != 0) {
>              break;
>          }
> @@ -9512,13 +9514,13 @@ dpif_netdev_ct_get_limits(struct dpif *dpif,
>  
>  static int
>  dpif_netdev_ct_del_limits(struct dpif *dpif,
> -                           const struct ovs_list *zone_limits)
> +                          const struct ovs_list *zone_limits, bool force)
>  {
>      int err = 0;
>      struct dp_netdev *dp = get_dp_netdev(dpif);
>      struct ct_dpif_zone_limit *zone_limit;
>      LIST_FOR_EACH (zone_limit, node, zone_limits) {
> -        err = zone_limit_delete(dp->conntrack, zone_limit->zone);
> +        err = zone_limit_delete(dp->conntrack, zone_limit->zone, force);
>          if (err != 0) {
>              break;
>          }
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 9194971d3..8ef5ce87f 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -250,6 +250,10 @@ static int ovs_ct_limit_family;
>   * Initialized by dpif_netlink_init(). */
>  static unsigned int ovs_vport_mcgroup;
>  
> +/* CT limit protection, must be global for all 'struct dpif_netlink'
> + * instances. */
> +static unsigned long ct_limit_protection[BITMAP_N_LONGS(UINT16_MAX)] = {0};

While it appers to make sense on paper to have a global bitmap across
all instances of dpif-netlink, there is no real need for that.  Primarely
because the 'datapaths' column in the 'Open_vSwitch' table can't contain
more than one datapath of the same type.

This means that we don't need protection to be implemented separately
for dpif-netdev and dpif-netlink.  It can be handled on the backer
level in ofproto-dpif, the same place where zones and their timeout
policies are handled.

> +
>  /* If true, tunnel devices are created using OVS compat/genetlink.
>   * If false, tunnel devices are created with rtnetlink and using light weight
>   * tunnels. If we fail to create the tunnel the rtnetlink+LWT, then we fallback
> @@ -3358,15 +3362,35 @@ dpif_netlink_ct_flush(struct dpif *dpif OVS_UNUSED, const uint16_t *zone,
>      }
>  }
>  
> +static int
> +update_zone_limit_protection(const struct ovs_list *limits, bool force)
> +{
> +    struct ct_dpif_zone_limit *zone_limit;
> +    LIST_FOR_EACH (zone_limit, node, limits) {
> +        if (bitmap_is_set(ct_limit_protection, zone_limit->zone) &&
> +            !force) {
> +            return EPERM;
> +        }
> +        bitmap_set(ct_limit_protection, zone_limit->zone, force);

Forced once, the zone is forever protected, even if removed from the
database, IIUC.  A test case for that would be nice.

> +    }
> +
> +    return 0;
> +}
> +
>  static int
>  dpif_netlink_ct_set_limits(struct dpif *dpif OVS_UNUSED,
>                             const uint32_t *default_limits,
> -                           const struct ovs_list *zone_limits)
> +                           const struct ovs_list *zone_limits, bool force)
>  {
>      if (ovs_ct_limit_family < 0) {
>          return EOPNOTSUPP;
>      }
>  
> +    int err = update_zone_limit_protection(zone_limits, force);
> +    if (err) {
> +        return err;
> +    }
> +
>      struct ofpbuf *request = ofpbuf_new(NL_DUMP_BUFSIZE);
>      nl_msg_put_genlmsghdr(request, 0, ovs_ct_limit_family,
>                            NLM_F_REQUEST | NLM_F_ECHO, OVS_CT_LIMIT_CMD_SET,
> @@ -3399,7 +3423,7 @@ dpif_netlink_ct_set_limits(struct dpif *dpif OVS_UNUSED,
>      }
>      nl_msg_end_nested(request, opt_offset);
>  
> -    int err = nl_transact(NETLINK_GENERIC, request, NULL);
> +    err = nl_transact(NETLINK_GENERIC, request, NULL);
>      ofpbuf_delete(request);
>      return err;
>  }
> @@ -3508,12 +3532,17 @@ out:
>  
>  static int
>  dpif_netlink_ct_del_limits(struct dpif *dpif OVS_UNUSED,
> -                           const struct ovs_list *zone_limits)
> +                           const struct ovs_list *zone_limits, bool force)
>  {
>      if (ovs_ct_limit_family < 0) {
>          return EOPNOTSUPP;
>      }
>  
> +    int err = update_zone_limit_protection(zone_limits, force);
> +    if (err) {
> +        return err;
> +    }
> +
>      struct ofpbuf *request = ofpbuf_new(NL_DUMP_BUFSIZE);
>      nl_msg_put_genlmsghdr(request, 0, ovs_ct_limit_family,
>              NLM_F_REQUEST | NLM_F_ECHO, OVS_CT_LIMIT_CMD_DEL,
> @@ -3537,7 +3566,7 @@ dpif_netlink_ct_del_limits(struct dpif *dpif OVS_UNUSED,
>          nl_msg_end_nested(request, opt_offset);
>      }
>  
> -    int err = nl_transact(NETLINK_GENERIC, request, NULL);
> +    err = nl_transact(NETLINK_GENERIC, request, NULL);
>  
>      ofpbuf_delete(request);
>      return err;
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 1b822cb07..6c292856f 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -521,9 +521,11 @@ struct dpif_class {
>      /* Sets the max connections allowed per zone according to 'zone_limits',
>       * a list of 'struct ct_dpif_zone_limit' entries (the 'count' member
>       * is not used when setting limits).  If 'default_limit' is not NULL,
> -     * modifies the default limit to '*default_limit'. */
> +     * modifies the default limit to '*default_limit'. If 'force' is set

Double spaces after a period.

> +     * to 'true' it will overwrite current configuration, otherwise it can
> +     * return 'EPERM' if the limit is already enforced for this zone. */

This is not fully true.  The function will fail to set any limits if
any one of them is protected.  Probbaly not a desireable behavior though.

>      int (*ct_set_limits)(struct dpif *, const uint32_t *default_limit,
> -                         const struct ovs_list *zone_limits);
> +                         const struct ovs_list *zone_limits, bool force);
>  
>      /* Looks up the default per zone limit and stores that in
>       * 'default_limit'.  Look up the per zone limits for all zones in
> @@ -536,8 +538,11 @@ struct dpif_class {
>                           struct ovs_list *zone_limits_out);
>  
>      /* Deletes per zone limit of all zones specified in 'zone_limits', a
> -     * list of 'struct ct_dpif_zone_limit' entries. */
> -    int (*ct_del_limits)(struct dpif *, const struct ovs_list *zone_limits);
> +     * list of 'struct ct_dpif_zone_limit' entries. If 'force' is set

Double spaces for consistency.

> +     * to 'true' it will remove current configuration, otherwise it
> +     * returns 'EPERM' if the limit is already enforced for this zone.*/
> +    int (*ct_del_limits)(struct dpif *, const struct ovs_list *zone_limits,
> +                         bool force);
>  
>      /* Connection tracking timeout policy */
>  
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 55eaeefa3..96149ad8d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5653,12 +5653,14 @@ static void
>  ct_zone_limits_commit(struct dpif_backer *backer)
>  {
>      if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
> -        ct_dpif_set_limits(backer->dpif, NULL, &backer->ct_zone_limits_to_add);
> +        ct_dpif_set_limits(backer->dpif, NULL, &backer->ct_zone_limits_to_add,
> +                           true);

Might be beter to break the line after the second argument instead.

>          ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
>      }
>  
>      if (!ovs_list_is_empty(&backer->ct_zone_limits_to_del)) {
> -        ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del);
> +        ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del,
> +                           true);
>          ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
>      }
>  }
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 537db66e0..8eb609675 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -5238,6 +5238,34 @@ OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
>  default limit=10
>  zone=0,limit=3,count=0])
>  
> +dnl Try to overwrite the zone limit via dpctl command.
> +AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=0,limit=5], [2], [ignore], [ignore])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +zone=0,limit=3,count=0
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0], [2], [ignore], [ignore])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +zone=0,limit=3,count=0
> +])
> +
> +dnl Set limit for zone that is not in DB via dpctl command.
> +AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=1,limit=5])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +zone=0,limit=3,count=0
> +zone=1,limit=5,count=0
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=1])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +zone=0,limit=3,count=0
> +])
> +
>  OVS_TRAFFIC_VSWITCHD_STOP(["dnl
>  /could not create datapath/d
>  /(Cannot allocate memory) on packet/d
Ales Musil Oct. 6, 2023, 5:31 a.m. UTC | #2 
On Thu, Oct 5, 2023 at 8:49 PM Ilya Maximets <i.maximets@ovn.org> wrote:

> On 10/2/23 12:33, Ales Musil wrote:
> > Enforce the CT limit protection, it ensures that
> > any CT limit value that was set by forced operation,
> > currently the DB CT limit, will be protected against
> > overwrite from other sources, e.g. the dpctl command.
> >
> > Signed-off-by: Ales Musil <amusil@redhat.com>
> > Acked-by: Simon Horman <horms@ovn.org>
> > ---
> > v3: Rebase on top of current master.
> >     Add ack from Simon and fix the missing '.'.
> > ---
> >  lib/conntrack.c         | 51 ++++++++++++++++++++++++++---------------
> >  lib/conntrack.h         |  5 ++--
> >  lib/ct-dpif.c           |  9 ++++----
> >  lib/ct-dpif.h           |  5 ++--
> >  lib/dpctl.c             |  4 ++--
> >  lib/dpif-netdev.c       | 12 ++++++----
> >  lib/dpif-netlink.c      | 37 ++++++++++++++++++++++++++----
> >  lib/dpif-provider.h     | 13 +++++++----
> >  ofproto/ofproto-dpif.c  |  6 +++--
> >  tests/system-traffic.at | 28 ++++++++++++++++++++++
> >  10 files changed, 126 insertions(+), 44 deletions(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 47a443fba..b5b5d4a4c 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -85,6 +85,7 @@ enum ct_alg_ctl_type {
> >  struct zone_limit {
> >      struct cmap_node node;
> >      struct conntrack_zone_limit czl;
> > +    bool limit_protected;
> >  };
> >
> >  static bool conn_key_extract(struct conntrack *, struct dp_packet *,
> > @@ -344,17 +345,13 @@ zone_limit_get(struct conntrack *ct, int32_t zone)
> >  }
> >
> >  static int
> > -zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
> > +zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit,
> > +                  bool limit_protected)
> >      OVS_REQUIRES(ct->ct_lock)
> >  {
> > -    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> > -
> > -    if (zl) {
> > -        return 0;
> > -    }
> > -
> >      if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
> > -        zl = xzalloc(sizeof *zl);
> > +        struct zone_limit *zl = xzalloc(sizeof *zl);
>
> An empty line after the variable declarations.
>
> > +        zl->limit_protected = limit_protected;
> >          zl->czl.limit = limit;
> >          zl->czl.zone = zone;
> >          zl->czl.zone_limit_seq = ct->zone_limit_seq++;
> > @@ -366,18 +363,28 @@ zone_limit_create(struct conntrack *ct, int32_t
> zone, uint32_t limit)
> >      }
> >  }
> >
> > +static inline bool
> > +can_update_zone_limit(struct zone_limit *zl, bool force)
> > +{
> > +    return !(zl && zl->limit_protected && !force);
> > +}
> > +
> >  int
> > -zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
> > +zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit,
> > +                  bool force)
> >  {
> >      int err = 0;
> > -    struct zone_limit *zl = zone_limit_lookup(ct, zone);
> > -    if (zl) {
> > +    ovs_mutex_lock(&ct->ct_lock);
> > +
> > +    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> > +    if (!can_update_zone_limit(zl, force)) {
> > +        err = EPERM;
> > +    } else if (zl) {
> >          zl->czl.limit = limit;
> > +        zl->limit_protected = force;
> >          VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
> >      } else {
> > -        ovs_mutex_lock(&ct->ct_lock);
> > -        err = zone_limit_create(ct, zone, limit);
> > -        ovs_mutex_unlock(&ct->ct_lock);
> > +        err = zone_limit_create(ct, zone, limit, force);
> >          if (!err) {
> >              VLOG_INFO("Created zone limit of %u for zone %d", limit,
> zone);
> >          } else {
> > @@ -385,6 +392,8 @@ zone_limit_update(struct conntrack *ct, int32_t
> zone, uint32_t limit)
> >                        zone);
> >          }
> >      }
> > +
> > +    ovs_mutex_unlock(&ct->ct_lock);
> >      return err;
> >  }
> >
> > @@ -398,20 +407,24 @@ zone_limit_clean(struct conntrack *ct, struct
> zone_limit *zl)
> >  }
> >
> >  int
> > -zone_limit_delete(struct conntrack *ct, uint16_t zone)
> > +zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force)
> >  {
> > +    int err = 0;
> >      ovs_mutex_lock(&ct->ct_lock);
> > +
> >      struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> > -    if (zl) {
> > +    if (!can_update_zone_limit(zl, force)) {
> > +        err = EPERM;
> > +    } else if (zl) {
> >          zone_limit_clean(ct, zl);
> > -        ovs_mutex_unlock(&ct->ct_lock);
> >          VLOG_INFO("Deleted zone limit for zone %d", zone);
> >      } else {
> > -        ovs_mutex_unlock(&ct->ct_lock);
> >          VLOG_INFO("Attempted delete of non-existent zone limit: zone
> %d",
> >                    zone);
> >      }
> > -    return 0;
> > +
> > +    ovs_mutex_unlock(&ct->ct_lock);
> > +    return err;
> >  }
> >
> >  static void
> > diff --git a/lib/conntrack.h b/lib/conntrack.h
> > index 57d5159b6..a58a800f9 100644
> > --- a/lib/conntrack.h
> > +++ b/lib/conntrack.h
> > @@ -153,7 +153,8 @@ bool conntrack_get_tcp_seq_chk(struct conntrack *ct);
> >  struct ipf *conntrack_ipf_ctx(struct conntrack *ct);
> >  struct conntrack_zone_limit zone_limit_get(struct conntrack *ct,
> >                                             int32_t zone);
> > -int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t
> limit);
> > -int zone_limit_delete(struct conntrack *ct, uint16_t zone);
> > +int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t
> limit,
> > +                      bool force);
> > +int zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force);
> >
> >  #endif /* conntrack.h */
> > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> > index f59c6e560..335ba09f9 100644
> > --- a/lib/ct-dpif.c
> > +++ b/lib/ct-dpif.c
> > @@ -399,11 +399,11 @@ ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool
> *enabled)
> >
> >  int
> >  ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
> > -                   const struct ovs_list *zone_limits)
> > +                   const struct ovs_list *zone_limits, bool force)
> >  {
> >      return (dpif->dpif_class->ct_set_limits
> >              ? dpif->dpif_class->ct_set_limits(dpif, default_limit,
> > -                                              zone_limits)
> > +                                              zone_limits, force)
> >              : EOPNOTSUPP);
> >  }
> >
> > @@ -420,10 +420,11 @@ ct_dpif_get_limits(struct dpif *dpif, uint32_t
> *default_limit,
> >  }
> >
> >  int
> > -ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list
> *zone_limits)
> > +ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list
> *zone_limits,
> > +                   bool force)
> >  {
> >      return (dpif->dpif_class->ct_del_limits
> > -            ? dpif->dpif_class->ct_del_limits(dpif, zone_limits)
> > +            ? dpif->dpif_class->ct_del_limits(dpif, zone_limits, force)
> >              : EOPNOTSUPP);
> >  }
> >
> > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> > index 0b728b529..0b74ec463 100644
> > --- a/lib/ct-dpif.h
> > +++ b/lib/ct-dpif.h
> > @@ -308,10 +308,11 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t
> *nconns);
> >  int ct_dpif_set_tcp_seq_chk(struct dpif *dpif, bool enabled);
> >  int ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled);
> >  int ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
> > -                       const struct ovs_list *);
> > +                       const struct ovs_list *, bool enforced);
> >  int ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
> >                         const struct ovs_list *, struct ovs_list *);
> > -int ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *);
> > +int ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *,
> > +                       bool enforced);
> >  int ct_dpif_sweep(struct dpif *, uint32_t *ms);
> >  int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
> >  int ct_dpif_ipf_set_min_frag(struct dpif *, bool v6, uint32_t min_frag);
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index cd12625a1..e498c29ce 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > @@ -2233,7 +2233,7 @@ dpctl_ct_set_limits(int argc, const char *argv[],
> >          ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0);
> >      }
> >
> > -    error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits);
> > +    error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits,
> false);
>
> Hmm.  Just noticed the difference of the default limit configuration.
> I would assume that the default limit is the most importnat thing
> to be configured.  But we do not have a way to set it via database.
> Should we add anorther column into Datapath table for that?  The feature
> seems incomplete without it.
>
> >      if (!error) {
> >          ct_dpif_free_zone_limits(&zone_limits);
> >          dpif_close(dpif);
> > @@ -2300,7 +2300,7 @@ dpctl_ct_del_limits(int argc, const char *argv[],
> >          goto error;
> >      }
> >
> > -    error = ct_dpif_del_limits(dpif, &zone_limits);
> > +    error = ct_dpif_del_limits(dpif, &zone_limits, false);
> >      if (!error) {
> >          goto out;
> >      } else {
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 157694bcf..90a48baa6 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -9447,12 +9447,14 @@ dpif_netdev_ct_get_sweep_interval(struct dpif
> *dpif, uint32_t *ms)
> >  static int
> >  dpif_netdev_ct_set_limits(struct dpif *dpif,
> >                             const uint32_t *default_limits,
> > -                           const struct ovs_list *zone_limits)
> > +                           const struct ovs_list *zone_limits,
> > +                           bool force)
> >  {
> >      int err = 0;
> >      struct dp_netdev *dp = get_dp_netdev(dpif);
> >      if (default_limits) {
> > -        err = zone_limit_update(dp->conntrack, DEFAULT_ZONE,
> *default_limits);
> > +        err = zone_limit_update(dp->conntrack, DEFAULT_ZONE,
> *default_limits,
> > +                                force);
> >          if (err != 0) {
> >              return err;
> >          }
> > @@ -9461,7 +9463,7 @@ dpif_netdev_ct_set_limits(struct dpif *dpif,
> >      struct ct_dpif_zone_limit *zone_limit;
> >      LIST_FOR_EACH (zone_limit, node, zone_limits) {
> >          err = zone_limit_update(dp->conntrack, zone_limit->zone,
> > -                                zone_limit->limit);
> > +                                zone_limit->limit, force);
> >          if (err != 0) {
> >              break;
> >          }
> > @@ -9512,13 +9514,13 @@ dpif_netdev_ct_get_limits(struct dpif *dpif,
> >
> >  static int
> >  dpif_netdev_ct_del_limits(struct dpif *dpif,
> > -                           const struct ovs_list *zone_limits)
> > +                          const struct ovs_list *zone_limits, bool
> force)
> >  {
> >      int err = 0;
> >      struct dp_netdev *dp = get_dp_netdev(dpif);
> >      struct ct_dpif_zone_limit *zone_limit;
> >      LIST_FOR_EACH (zone_limit, node, zone_limits) {
> > -        err = zone_limit_delete(dp->conntrack, zone_limit->zone);
> > +        err = zone_limit_delete(dp->conntrack, zone_limit->zone, force);
> >          if (err != 0) {
> >              break;
> >          }
> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > index 9194971d3..8ef5ce87f 100644
> > --- a/lib/dpif-netlink.c
> > +++ b/lib/dpif-netlink.c
> > @@ -250,6 +250,10 @@ static int ovs_ct_limit_family;
> >   * Initialized by dpif_netlink_init(). */
> >  static unsigned int ovs_vport_mcgroup;
> >
> > +/* CT limit protection, must be global for all 'struct dpif_netlink'
> > + * instances. */
> > +static unsigned long ct_limit_protection[BITMAP_N_LONGS(UINT16_MAX)] =
> {0};
>
> While it appers to make sense on paper to have a global bitmap across
> all instances of dpif-netlink, there is no real need for that.  Primarely
> because the 'datapaths' column in the 'Open_vSwitch' table can't contain
> more than one datapath of the same type.
>
> This means that we don't need protection to be implemented separately
> for dpif-netdev and dpif-netlink.  It can be handled on the backer
> level in ofproto-dpif, the same place where zones and their timeout
> policies are handled.
>
>
It can't, it would be much easier if it could be stored in the backer,
however dpctl doesn't have access to the backer instance (if it does I have
missed how to achieve that). So dpctl wouldn't have a way to check because
it calls directly "struct dpif_class".


> > +
> >  /* If true, tunnel devices are created using OVS compat/genetlink.
> >   * If false, tunnel devices are created with rtnetlink and using light
> weight
> >   * tunnels. If we fail to create the tunnel the rtnetlink+LWT, then we
> fallback
> > @@ -3358,15 +3362,35 @@ dpif_netlink_ct_flush(struct dpif *dpif
> OVS_UNUSED, const uint16_t *zone,
> >      }
> >  }
> >
> > +static int
> > +update_zone_limit_protection(const struct ovs_list *limits, bool force)
> > +{
> > +    struct ct_dpif_zone_limit *zone_limit;
> > +    LIST_FOR_EACH (zone_limit, node, limits) {
> > +        if (bitmap_is_set(ct_limit_protection, zone_limit->zone) &&
> > +            !force) {
> > +            return EPERM;
> > +        }
> > +        bitmap_set(ct_limit_protection, zone_limit->zone, force);
>
> Forced once, the zone is forever protected, even if removed from the
> database, IIUC.  A test case for that would be nice.
>
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int
> >  dpif_netlink_ct_set_limits(struct dpif *dpif OVS_UNUSED,
> >                             const uint32_t *default_limits,
> > -                           const struct ovs_list *zone_limits)
> > +                           const struct ovs_list *zone_limits, bool
> force)
> >  {
> >      if (ovs_ct_limit_family < 0) {
> >          return EOPNOTSUPP;
> >      }
> >
> > +    int err = update_zone_limit_protection(zone_limits, force);
> > +    if (err) {
> > +        return err;
> > +    }
> > +
> >      struct ofpbuf *request = ofpbuf_new(NL_DUMP_BUFSIZE);
> >      nl_msg_put_genlmsghdr(request, 0, ovs_ct_limit_family,
> >                            NLM_F_REQUEST | NLM_F_ECHO,
> OVS_CT_LIMIT_CMD_SET,
> > @@ -3399,7 +3423,7 @@ dpif_netlink_ct_set_limits(struct dpif *dpif
> OVS_UNUSED,
> >      }
> >      nl_msg_end_nested(request, opt_offset);
> >
> > -    int err = nl_transact(NETLINK_GENERIC, request, NULL);
> > +    err = nl_transact(NETLINK_GENERIC, request, NULL);
> >      ofpbuf_delete(request);
> >      return err;
> >  }
> > @@ -3508,12 +3532,17 @@ out:
> >
> >  static int
> >  dpif_netlink_ct_del_limits(struct dpif *dpif OVS_UNUSED,
> > -                           const struct ovs_list *zone_limits)
> > +                           const struct ovs_list *zone_limits, bool
> force)
> >  {
> >      if (ovs_ct_limit_family < 0) {
> >          return EOPNOTSUPP;
> >      }
> >
> > +    int err = update_zone_limit_protection(zone_limits, force);
> > +    if (err) {
> > +        return err;
> > +    }
> > +
> >      struct ofpbuf *request = ofpbuf_new(NL_DUMP_BUFSIZE);
> >      nl_msg_put_genlmsghdr(request, 0, ovs_ct_limit_family,
> >              NLM_F_REQUEST | NLM_F_ECHO, OVS_CT_LIMIT_CMD_DEL,
> > @@ -3537,7 +3566,7 @@ dpif_netlink_ct_del_limits(struct dpif *dpif
> OVS_UNUSED,
> >          nl_msg_end_nested(request, opt_offset);
> >      }
> >
> > -    int err = nl_transact(NETLINK_GENERIC, request, NULL);
> > +    err = nl_transact(NETLINK_GENERIC, request, NULL);
> >
> >      ofpbuf_delete(request);
> >      return err;
> > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> > index 1b822cb07..6c292856f 100644
> > --- a/lib/dpif-provider.h
> > +++ b/lib/dpif-provider.h
> > @@ -521,9 +521,11 @@ struct dpif_class {
> >      /* Sets the max connections allowed per zone according to
> 'zone_limits',
> >       * a list of 'struct ct_dpif_zone_limit' entries (the 'count' member
> >       * is not used when setting limits).  If 'default_limit' is not
> NULL,
> > -     * modifies the default limit to '*default_limit'. */
> > +     * modifies the default limit to '*default_limit'. If 'force' is set
>
> Double spaces after a period.
>
> > +     * to 'true' it will overwrite current configuration, otherwise it
> can
> > +     * return 'EPERM' if the limit is already enforced for this zone. */
>
> This is not fully true.  The function will fail to set any limits if
> any one of them is protected.  Probbaly not a desireable behavior though.
>

I'm not really sure if it can be done better except for a better
explanation. The command is designed in a way to set multiple zones at
once, and we have no way to check before. Considering that I'm afraid that
we don't have a choice and have to fail the whole command if any of the
zones is protected.


>
> >      int (*ct_set_limits)(struct dpif *, const uint32_t *default_limit,
> > -                         const struct ovs_list *zone_limits);
> > +                         const struct ovs_list *zone_limits, bool
> force);
> >
> >      /* Looks up the default per zone limit and stores that in
> >       * 'default_limit'.  Look up the per zone limits for all zones in
> > @@ -536,8 +538,11 @@ struct dpif_class {
> >                           struct ovs_list *zone_limits_out);
> >
> >      /* Deletes per zone limit of all zones specified in 'zone_limits', a
> > -     * list of 'struct ct_dpif_zone_limit' entries. */
> > -    int (*ct_del_limits)(struct dpif *, const struct ovs_list
> *zone_limits);
> > +     * list of 'struct ct_dpif_zone_limit' entries. If 'force' is set
>
> Double spaces for consistency.
>
> > +     * to 'true' it will remove current configuration, otherwise it
> > +     * returns 'EPERM' if the limit is already enforced for this zone.*/
> > +    int (*ct_del_limits)(struct dpif *, const struct ovs_list
> *zone_limits,
> > +                         bool force);
> >
> >      /* Connection tracking timeout policy */
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 55eaeefa3..96149ad8d 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -5653,12 +5653,14 @@ static void
> >  ct_zone_limits_commit(struct dpif_backer *backer)
> >  {
> >      if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
> > -        ct_dpif_set_limits(backer->dpif, NULL,
> &backer->ct_zone_limits_to_add);
> > +        ct_dpif_set_limits(backer->dpif, NULL,
> &backer->ct_zone_limits_to_add,
> > +                           true);
>
> Might be beter to break the line after the second argument instead.
>
> >          ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
> >      }
> >
> >      if (!ovs_list_is_empty(&backer->ct_zone_limits_to_del)) {
> > -        ct_dpif_del_limits(backer->dpif,
> &backer->ct_zone_limits_to_del);
> > +        ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del,
> > +                           true);
> >          ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
> >      }
> >  }
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 537db66e0..8eb609675 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -5238,6 +5238,34 @@ OVS_WAIT_UNTIL_EQUAL([ovs-appctl
> dpctl/ct-get-limits], [dnl
> >  default limit=10
> >  zone=0,limit=3,count=0])
> >
> > +dnl Try to overwrite the zone limit via dpctl command.
> > +AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=0,limit=5], [2],
> [ignore], [ignore])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=10
> > +zone=0,limit=3,count=0
> > +])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0], [2], [ignore],
> [ignore])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=10
> > +zone=0,limit=3,count=0
> > +])
> > +
> > +dnl Set limit for zone that is not in DB via dpctl command.
> > +AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=1,limit=5])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=10
> > +zone=0,limit=3,count=0
> > +zone=1,limit=5,count=0
> > +])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=1])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=10
> > +zone=0,limit=3,count=0
> > +])
> > +
> >  OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> >  /could not create datapath/d
> >  /(Cannot allocate memory) on packet/d
>
>

Thanks,
Ales
Ilya Maximets Oct. 9, 2023, 9:44 a.m. UTC | #3 
On 10/6/23 07:31, Ales Musil wrote:
> 
> 
> On Thu, Oct 5, 2023 at 8:49 PM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote:
> 
>     On 10/2/23 12:33, Ales Musil wrote:
>     > Enforce the CT limit protection, it ensures that
>     > any CT limit value that was set by forced operation,
>     > currently the DB CT limit, will be protected against
>     > overwrite from other sources, e.g. the dpctl command.
>     >
>     > Signed-off-by: Ales Musil <amusil@redhat.com <mailto:amusil@redhat.com>>
>     > Acked-by: Simon Horman <horms@ovn.org <mailto:horms@ovn.org>>
>     > ---
>     > v3: Rebase on top of current master.
>     >     Add ack from Simon and fix the missing '.'.
>     > ---
>     >  lib/conntrack.c         | 51 ++++++++++++++++++++++++++---------------
>     >  lib/conntrack.h         |  5 ++--
>     >  lib/ct-dpif.c           |  9 ++++----
>     >  lib/ct-dpif.h           |  5 ++--
>     >  lib/dpctl.c             |  4 ++--
>     >  lib/dpif-netdev.c       | 12 ++++++----
>     >  lib/dpif-netlink.c      | 37 ++++++++++++++++++++++++++----
>     >  lib/dpif-provider.h     | 13 +++++++----
>     >  ofproto/ofproto-dpif.c  |  6 +++--
>     >  tests/system-traffic.at <http://system-traffic.at> | 28 ++++++++++++++++++++++
>     >  10 files changed, 126 insertions(+), 44 deletions(-)
>     >
>     > diff --git a/lib/conntrack.c b/lib/conntrack.c
>     > index 47a443fba..b5b5d4a4c 100644
>     > --- a/lib/conntrack.c
>     > +++ b/lib/conntrack.c
>     > @@ -85,6 +85,7 @@ enum ct_alg_ctl_type {
>     >  struct zone_limit {
>     >      struct cmap_node node;
>     >      struct conntrack_zone_limit czl;
>     > +    bool limit_protected;
>     >  };
>     > 
>     >  static bool conn_key_extract(struct conntrack *, struct dp_packet *,
>     > @@ -344,17 +345,13 @@ zone_limit_get(struct conntrack *ct, int32_t zone)
>     >  }
>     > 
>     >  static int
>     > -zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
>     > +zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit,
>     > +                  bool limit_protected)
>     >      OVS_REQUIRES(ct->ct_lock)
>     >  {
>     > -    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
>     > -
>     > -    if (zl) {
>     > -        return 0;
>     > -    }
>     > -
>     >      if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
>     > -        zl = xzalloc(sizeof *zl);
>     > +        struct zone_limit *zl = xzalloc(sizeof *zl);
> 
>     An empty line after the variable declarations.
> 
>     > +        zl->limit_protected = limit_protected;
>     >          zl->czl.limit = limit;
>     >          zl->czl.zone = zone;
>     >          zl->czl.zone_limit_seq = ct->zone_limit_seq++;
>     > @@ -366,18 +363,28 @@ zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
>     >      }
>     >  }
>     > 
>     > +static inline bool
>     > +can_update_zone_limit(struct zone_limit *zl, bool force)
>     > +{
>     > +    return !(zl && zl->limit_protected && !force);
>     > +}
>     > +
>     >  int
>     > -zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
>     > +zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit,
>     > +                  bool force)
>     >  {
>     >      int err = 0;
>     > -    struct zone_limit *zl = zone_limit_lookup(ct, zone);
>     > -    if (zl) {
>     > +    ovs_mutex_lock(&ct->ct_lock);
>     > +
>     > +    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
>     > +    if (!can_update_zone_limit(zl, force)) {
>     > +        err = EPERM;
>     > +    } else if (zl) {
>     >          zl->czl.limit = limit;
>     > +        zl->limit_protected = force;
>     >          VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
>     >      } else {
>     > -        ovs_mutex_lock(&ct->ct_lock);
>     > -        err = zone_limit_create(ct, zone, limit);
>     > -        ovs_mutex_unlock(&ct->ct_lock);
>     > +        err = zone_limit_create(ct, zone, limit, force);
>     >          if (!err) {
>     >              VLOG_INFO("Created zone limit of %u for zone %d", limit, zone);
>     >          } else {
>     > @@ -385,6 +392,8 @@ zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
>     >                        zone);
>     >          }
>     >      }
>     > +
>     > +    ovs_mutex_unlock(&ct->ct_lock);
>     >      return err;
>     >  }
>     > 
>     > @@ -398,20 +407,24 @@ zone_limit_clean(struct conntrack *ct, struct zone_limit *zl)
>     >  }
>     > 
>     >  int
>     > -zone_limit_delete(struct conntrack *ct, uint16_t zone)
>     > +zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force)
>     >  {
>     > +    int err = 0;
>     >      ovs_mutex_lock(&ct->ct_lock);
>     > +
>     >      struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
>     > -    if (zl) {
>     > +    if (!can_update_zone_limit(zl, force)) {
>     > +        err = EPERM;
>     > +    } else if (zl) {
>     >          zone_limit_clean(ct, zl);
>     > -        ovs_mutex_unlock(&ct->ct_lock);
>     >          VLOG_INFO("Deleted zone limit for zone %d", zone);
>     >      } else {
>     > -        ovs_mutex_unlock(&ct->ct_lock);
>     >          VLOG_INFO("Attempted delete of non-existent zone limit: zone %d",
>     >                    zone);
>     >      }
>     > -    return 0;
>     > +
>     > +    ovs_mutex_unlock(&ct->ct_lock);
>     > +    return err;
>     >  }
>     > 
>     >  static void
>     > diff --git a/lib/conntrack.h b/lib/conntrack.h
>     > index 57d5159b6..a58a800f9 100644
>     > --- a/lib/conntrack.h
>     > +++ b/lib/conntrack.h
>     > @@ -153,7 +153,8 @@ bool conntrack_get_tcp_seq_chk(struct conntrack *ct);
>     >  struct ipf *conntrack_ipf_ctx(struct conntrack *ct);
>     >  struct conntrack_zone_limit zone_limit_get(struct conntrack *ct,
>     >                                             int32_t zone);
>     > -int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit);
>     > -int zone_limit_delete(struct conntrack *ct, uint16_t zone);
>     > +int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit,
>     > +                      bool force);
>     > +int zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force);
>     > 
>     >  #endif /* conntrack.h */
>     > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
>     > index f59c6e560..335ba09f9 100644
>     > --- a/lib/ct-dpif.c
>     > +++ b/lib/ct-dpif.c
>     > @@ -399,11 +399,11 @@ ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled)
>     > 
>     >  int
>     >  ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
>     > -                   const struct ovs_list *zone_limits)
>     > +                   const struct ovs_list *zone_limits, bool force)
>     >  {
>     >      return (dpif->dpif_class->ct_set_limits
>     >              ? dpif->dpif_class->ct_set_limits(dpif, default_limit,
>     > -                                              zone_limits)
>     > +                                              zone_limits, force)
>     >              : EOPNOTSUPP);
>     >  }
>     > 
>     > @@ -420,10 +420,11 @@ ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
>     >  }
>     > 
>     >  int
>     > -ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *zone_limits)
>     > +ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *zone_limits,
>     > +                   bool force)
>     >  {
>     >      return (dpif->dpif_class->ct_del_limits
>     > -            ? dpif->dpif_class->ct_del_limits(dpif, zone_limits)
>     > +            ? dpif->dpif_class->ct_del_limits(dpif, zone_limits, force)
>     >              : EOPNOTSUPP);
>     >  }
>     > 
>     > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
>     > index 0b728b529..0b74ec463 100644
>     > --- a/lib/ct-dpif.h
>     > +++ b/lib/ct-dpif.h
>     > @@ -308,10 +308,11 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns);
>     >  int ct_dpif_set_tcp_seq_chk(struct dpif *dpif, bool enabled);
>     >  int ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled);
>     >  int ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
>     > -                       const struct ovs_list *);
>     > +                       const struct ovs_list *, bool enforced);
>     >  int ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
>     >                         const struct ovs_list *, struct ovs_list *);
>     > -int ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *);
>     > +int ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *,
>     > +                       bool enforced);
>     >  int ct_dpif_sweep(struct dpif *, uint32_t *ms);
>     >  int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
>     >  int ct_dpif_ipf_set_min_frag(struct dpif *, bool v6, uint32_t min_frag);
>     > diff --git a/lib/dpctl.c b/lib/dpctl.c
>     > index cd12625a1..e498c29ce 100644
>     > --- a/lib/dpctl.c
>     > +++ b/lib/dpctl.c
>     > @@ -2233,7 +2233,7 @@ dpctl_ct_set_limits(int argc, const char *argv[],
>     >          ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0);
>     >      }
>     > 
>     > -    error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits);
>     > +    error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits, false);
> 
>     Hmm.  Just noticed the difference of the default limit configuration.
>     I would assume that the default limit is the most importnat thing
>     to be configured.  But we do not have a way to set it via database.
>     Should we add anorther column into Datapath table for that?  The feature
>     seems incomplete without it.
> 
>     >      if (!error) {
>     >          ct_dpif_free_zone_limits(&zone_limits);
>     >          dpif_close(dpif);
>     > @@ -2300,7 +2300,7 @@ dpctl_ct_del_limits(int argc, const char *argv[],
>     >          goto error;
>     >      }
>     > 
>     > -    error = ct_dpif_del_limits(dpif, &zone_limits);
>     > +    error = ct_dpif_del_limits(dpif, &zone_limits, false);
>     >      if (!error) {
>     >          goto out;
>     >      } else {
>     > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>     > index 157694bcf..90a48baa6 100644
>     > --- a/lib/dpif-netdev.c
>     > +++ b/lib/dpif-netdev.c
>     > @@ -9447,12 +9447,14 @@ dpif_netdev_ct_get_sweep_interval(struct dpif *dpif, uint32_t *ms)
>     >  static int
>     >  dpif_netdev_ct_set_limits(struct dpif *dpif,
>     >                             const uint32_t *default_limits,
>     > -                           const struct ovs_list *zone_limits)
>     > +                           const struct ovs_list *zone_limits,
>     > +                           bool force)
>     >  {
>     >      int err = 0;
>     >      struct dp_netdev *dp = get_dp_netdev(dpif);
>     >      if (default_limits) {
>     > -        err = zone_limit_update(dp->conntrack, DEFAULT_ZONE, *default_limits);
>     > +        err = zone_limit_update(dp->conntrack, DEFAULT_ZONE, *default_limits,
>     > +                                force);
>     >          if (err != 0) {
>     >              return err;
>     >          }
>     > @@ -9461,7 +9463,7 @@ dpif_netdev_ct_set_limits(struct dpif *dpif,
>     >      struct ct_dpif_zone_limit *zone_limit;
>     >      LIST_FOR_EACH (zone_limit, node, zone_limits) {
>     >          err = zone_limit_update(dp->conntrack, zone_limit->zone,
>     > -                                zone_limit->limit);
>     > +                                zone_limit->limit, force);
>     >          if (err != 0) {
>     >              break;
>     >          }
>     > @@ -9512,13 +9514,13 @@ dpif_netdev_ct_get_limits(struct dpif *dpif,
>     > 
>     >  static int
>     >  dpif_netdev_ct_del_limits(struct dpif *dpif,
>     > -                           const struct ovs_list *zone_limits)
>     > +                          const struct ovs_list *zone_limits, bool force)
>     >  {
>     >      int err = 0;
>     >      struct dp_netdev *dp = get_dp_netdev(dpif);
>     >      struct ct_dpif_zone_limit *zone_limit;
>     >      LIST_FOR_EACH (zone_limit, node, zone_limits) {
>     > -        err = zone_limit_delete(dp->conntrack, zone_limit->zone);
>     > +        err = zone_limit_delete(dp->conntrack, zone_limit->zone, force);
>     >          if (err != 0) {
>     >              break;
>     >          }
>     > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>     > index 9194971d3..8ef5ce87f 100644
>     > --- a/lib/dpif-netlink.c
>     > +++ b/lib/dpif-netlink.c
>     > @@ -250,6 +250,10 @@ static int ovs_ct_limit_family;
>     >   * Initialized by dpif_netlink_init(). */
>     >  static unsigned int ovs_vport_mcgroup;
>     > 
>     > +/* CT limit protection, must be global for all 'struct dpif_netlink'
>     > + * instances. */
>     > +static unsigned long ct_limit_protection[BITMAP_N_LONGS(UINT16_MAX)] = {0};
> 
>     While it appers to make sense on paper to have a global bitmap across
>     all instances of dpif-netlink, there is no real need for that.  Primarely
>     because the 'datapaths' column in the 'Open_vSwitch' table can't contain
>     more than one datapath of the same type.
> 
>     This means that we don't need protection to be implemented separately
>     for dpif-netdev and dpif-netlink.  It can be handled on the backer
>     level in ofproto-dpif, the same place where zones and their timeout
>     policies are handled.
> 
> 
> It can't, it would be much easier if it could be stored in the backer, however dpctl doesn't have access to the backer instance (if it does I have missed how to achieve that). So dpctl wouldn't have a way to check because it calls directly "struct dpif_class".


Hrm.  How about we just simplify the protection to a single flag?
i.e. if users configured anything through the database, they will
have to configure everything through the database.  We could, for
example, create a global flag in ct-dpif.c[1], export functions to
set and check the value.  Set from bridge.c and check from dpctl.c.

This will also solve the problem with only setting limits if only
one of them is protected.

What do you think?

Best regards, Ilya Maximets.

[1] not in dpctl.c, because it would be awkward to include dpctl.h
    in bridge.c.
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 47a443fba..b5b5d4a4c 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -85,6 +85,7 @@  enum ct_alg_ctl_type {
 struct zone_limit {
     struct cmap_node node;
     struct conntrack_zone_limit czl;
+    bool limit_protected;
 };
 
 static bool conn_key_extract(struct conntrack *, struct dp_packet *,
@@ -344,17 +345,13 @@  zone_limit_get(struct conntrack *ct, int32_t zone)
 }
 
 static int
-zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
+zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit,
+                  bool limit_protected)
     OVS_REQUIRES(ct->ct_lock)
 {
-    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
-
-    if (zl) {
-        return 0;
-    }
-
     if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
-        zl = xzalloc(sizeof *zl);
+        struct zone_limit *zl = xzalloc(sizeof *zl);
+        zl->limit_protected = limit_protected;
         zl->czl.limit = limit;
         zl->czl.zone = zone;
         zl->czl.zone_limit_seq = ct->zone_limit_seq++;
@@ -366,18 +363,28 @@  zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
     }
 }
 
+static inline bool
+can_update_zone_limit(struct zone_limit *zl, bool force)
+{
+    return !(zl && zl->limit_protected && !force);
+}
+
 int
-zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
+zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit,
+                  bool force)
 {
     int err = 0;
-    struct zone_limit *zl = zone_limit_lookup(ct, zone);
-    if (zl) {
+    ovs_mutex_lock(&ct->ct_lock);
+
+    struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
+    if (!can_update_zone_limit(zl, force)) {
+        err = EPERM;
+    } else if (zl) {
         zl->czl.limit = limit;
+        zl->limit_protected = force;
         VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
     } else {
-        ovs_mutex_lock(&ct->ct_lock);
-        err = zone_limit_create(ct, zone, limit);
-        ovs_mutex_unlock(&ct->ct_lock);
+        err = zone_limit_create(ct, zone, limit, force);
         if (!err) {
             VLOG_INFO("Created zone limit of %u for zone %d", limit, zone);
         } else {
@@ -385,6 +392,8 @@  zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
                       zone);
         }
     }
+
+    ovs_mutex_unlock(&ct->ct_lock);
     return err;
 }
 
@@ -398,20 +407,24 @@  zone_limit_clean(struct conntrack *ct, struct zone_limit *zl)
 }
 
 int
-zone_limit_delete(struct conntrack *ct, uint16_t zone)
+zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force)
 {
+    int err = 0;
     ovs_mutex_lock(&ct->ct_lock);
+
     struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
-    if (zl) {
+    if (!can_update_zone_limit(zl, force)) {
+        err = EPERM;
+    } else if (zl) {
         zone_limit_clean(ct, zl);
-        ovs_mutex_unlock(&ct->ct_lock);
         VLOG_INFO("Deleted zone limit for zone %d", zone);
     } else {
-        ovs_mutex_unlock(&ct->ct_lock);
         VLOG_INFO("Attempted delete of non-existent zone limit: zone %d",
                   zone);
     }
-    return 0;
+
+    ovs_mutex_unlock(&ct->ct_lock);
+    return err;
 }
 
 static void
diff --git a/lib/conntrack.h b/lib/conntrack.h
index 57d5159b6..a58a800f9 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -153,7 +153,8 @@  bool conntrack_get_tcp_seq_chk(struct conntrack *ct);
 struct ipf *conntrack_ipf_ctx(struct conntrack *ct);
 struct conntrack_zone_limit zone_limit_get(struct conntrack *ct,
                                            int32_t zone);
-int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit);
-int zone_limit_delete(struct conntrack *ct, uint16_t zone);
+int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit,
+                      bool force);
+int zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force);
 
 #endif /* conntrack.h */
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index f59c6e560..335ba09f9 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -399,11 +399,11 @@  ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled)
 
 int
 ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
-                   const struct ovs_list *zone_limits)
+                   const struct ovs_list *zone_limits, bool force)
 {
     return (dpif->dpif_class->ct_set_limits
             ? dpif->dpif_class->ct_set_limits(dpif, default_limit,
-                                              zone_limits)
+                                              zone_limits, force)
             : EOPNOTSUPP);
 }
 
@@ -420,10 +420,11 @@  ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
 }
 
 int
-ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *zone_limits)
+ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *zone_limits,
+                   bool force)
 {
     return (dpif->dpif_class->ct_del_limits
-            ? dpif->dpif_class->ct_del_limits(dpif, zone_limits)
+            ? dpif->dpif_class->ct_del_limits(dpif, zone_limits, force)
             : EOPNOTSUPP);
 }
 
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 0b728b529..0b74ec463 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -308,10 +308,11 @@  int ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns);
 int ct_dpif_set_tcp_seq_chk(struct dpif *dpif, bool enabled);
 int ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled);
 int ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
-                       const struct ovs_list *);
+                       const struct ovs_list *, bool enforced);
 int ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
                        const struct ovs_list *, struct ovs_list *);
-int ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *);
+int ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *,
+                       bool enforced);
 int ct_dpif_sweep(struct dpif *, uint32_t *ms);
 int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
 int ct_dpif_ipf_set_min_frag(struct dpif *, bool v6, uint32_t min_frag);
diff --git a/lib/dpctl.c b/lib/dpctl.c
index cd12625a1..e498c29ce 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -2233,7 +2233,7 @@  dpctl_ct_set_limits(int argc, const char *argv[],
         ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0);
     }
 
-    error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits);
+    error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits, false);
     if (!error) {
         ct_dpif_free_zone_limits(&zone_limits);
         dpif_close(dpif);
@@ -2300,7 +2300,7 @@  dpctl_ct_del_limits(int argc, const char *argv[],
         goto error;
     }
 
-    error = ct_dpif_del_limits(dpif, &zone_limits);
+    error = ct_dpif_del_limits(dpif, &zone_limits, false);
     if (!error) {
         goto out;
     } else {
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 157694bcf..90a48baa6 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -9447,12 +9447,14 @@  dpif_netdev_ct_get_sweep_interval(struct dpif *dpif, uint32_t *ms)
 static int
 dpif_netdev_ct_set_limits(struct dpif *dpif,
                            const uint32_t *default_limits,
-                           const struct ovs_list *zone_limits)
+                           const struct ovs_list *zone_limits,
+                           bool force)
 {
     int err = 0;
     struct dp_netdev *dp = get_dp_netdev(dpif);
     if (default_limits) {
-        err = zone_limit_update(dp->conntrack, DEFAULT_ZONE, *default_limits);
+        err = zone_limit_update(dp->conntrack, DEFAULT_ZONE, *default_limits,
+                                force);
         if (err != 0) {
             return err;
         }
@@ -9461,7 +9463,7 @@  dpif_netdev_ct_set_limits(struct dpif *dpif,
     struct ct_dpif_zone_limit *zone_limit;
     LIST_FOR_EACH (zone_limit, node, zone_limits) {
         err = zone_limit_update(dp->conntrack, zone_limit->zone,
-                                zone_limit->limit);
+                                zone_limit->limit, force);
         if (err != 0) {
             break;
         }
@@ -9512,13 +9514,13 @@  dpif_netdev_ct_get_limits(struct dpif *dpif,
 
 static int
 dpif_netdev_ct_del_limits(struct dpif *dpif,
-                           const struct ovs_list *zone_limits)
+                          const struct ovs_list *zone_limits, bool force)
 {
     int err = 0;
     struct dp_netdev *dp = get_dp_netdev(dpif);
     struct ct_dpif_zone_limit *zone_limit;
     LIST_FOR_EACH (zone_limit, node, zone_limits) {
-        err = zone_limit_delete(dp->conntrack, zone_limit->zone);
+        err = zone_limit_delete(dp->conntrack, zone_limit->zone, force);
         if (err != 0) {
             break;
         }
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 9194971d3..8ef5ce87f 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -250,6 +250,10 @@  static int ovs_ct_limit_family;
  * Initialized by dpif_netlink_init(). */
 static unsigned int ovs_vport_mcgroup;
 
+/* CT limit protection, must be global for all 'struct dpif_netlink'
+ * instances. */
+static unsigned long ct_limit_protection[BITMAP_N_LONGS(UINT16_MAX)] = {0};
+
 /* If true, tunnel devices are created using OVS compat/genetlink.
  * If false, tunnel devices are created with rtnetlink and using light weight
  * tunnels. If we fail to create the tunnel the rtnetlink+LWT, then we fallback
@@ -3358,15 +3362,35 @@  dpif_netlink_ct_flush(struct dpif *dpif OVS_UNUSED, const uint16_t *zone,
     }
 }
 
+static int
+update_zone_limit_protection(const struct ovs_list *limits, bool force)
+{
+    struct ct_dpif_zone_limit *zone_limit;
+    LIST_FOR_EACH (zone_limit, node, limits) {
+        if (bitmap_is_set(ct_limit_protection, zone_limit->zone) &&
+            !force) {
+            return EPERM;
+        }
+        bitmap_set(ct_limit_protection, zone_limit->zone, force);
+    }
+
+    return 0;
+}
+
 static int
 dpif_netlink_ct_set_limits(struct dpif *dpif OVS_UNUSED,
                            const uint32_t *default_limits,
-                           const struct ovs_list *zone_limits)
+                           const struct ovs_list *zone_limits, bool force)
 {
     if (ovs_ct_limit_family < 0) {
         return EOPNOTSUPP;
     }
 
+    int err = update_zone_limit_protection(zone_limits, force);
+    if (err) {
+        return err;
+    }
+
     struct ofpbuf *request = ofpbuf_new(NL_DUMP_BUFSIZE);
     nl_msg_put_genlmsghdr(request, 0, ovs_ct_limit_family,
                           NLM_F_REQUEST | NLM_F_ECHO, OVS_CT_LIMIT_CMD_SET,
@@ -3399,7 +3423,7 @@  dpif_netlink_ct_set_limits(struct dpif *dpif OVS_UNUSED,
     }
     nl_msg_end_nested(request, opt_offset);
 
-    int err = nl_transact(NETLINK_GENERIC, request, NULL);
+    err = nl_transact(NETLINK_GENERIC, request, NULL);
     ofpbuf_delete(request);
     return err;
 }
@@ -3508,12 +3532,17 @@  out:
 
 static int
 dpif_netlink_ct_del_limits(struct dpif *dpif OVS_UNUSED,
-                           const struct ovs_list *zone_limits)
+                           const struct ovs_list *zone_limits, bool force)
 {
     if (ovs_ct_limit_family < 0) {
         return EOPNOTSUPP;
     }
 
+    int err = update_zone_limit_protection(zone_limits, force);
+    if (err) {
+        return err;
+    }
+
     struct ofpbuf *request = ofpbuf_new(NL_DUMP_BUFSIZE);
     nl_msg_put_genlmsghdr(request, 0, ovs_ct_limit_family,
             NLM_F_REQUEST | NLM_F_ECHO, OVS_CT_LIMIT_CMD_DEL,
@@ -3537,7 +3566,7 @@  dpif_netlink_ct_del_limits(struct dpif *dpif OVS_UNUSED,
         nl_msg_end_nested(request, opt_offset);
     }
 
-    int err = nl_transact(NETLINK_GENERIC, request, NULL);
+    err = nl_transact(NETLINK_GENERIC, request, NULL);
 
     ofpbuf_delete(request);
     return err;
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 1b822cb07..6c292856f 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -521,9 +521,11 @@  struct dpif_class {
     /* Sets the max connections allowed per zone according to 'zone_limits',
      * a list of 'struct ct_dpif_zone_limit' entries (the 'count' member
      * is not used when setting limits).  If 'default_limit' is not NULL,
-     * modifies the default limit to '*default_limit'. */
+     * modifies the default limit to '*default_limit'. If 'force' is set
+     * to 'true' it will overwrite current configuration, otherwise it can
+     * return 'EPERM' if the limit is already enforced for this zone. */
     int (*ct_set_limits)(struct dpif *, const uint32_t *default_limit,
-                         const struct ovs_list *zone_limits);
+                         const struct ovs_list *zone_limits, bool force);
 
     /* Looks up the default per zone limit and stores that in
      * 'default_limit'.  Look up the per zone limits for all zones in
@@ -536,8 +538,11 @@  struct dpif_class {
                          struct ovs_list *zone_limits_out);
 
     /* Deletes per zone limit of all zones specified in 'zone_limits', a
-     * list of 'struct ct_dpif_zone_limit' entries. */
-    int (*ct_del_limits)(struct dpif *, const struct ovs_list *zone_limits);
+     * list of 'struct ct_dpif_zone_limit' entries. If 'force' is set
+     * to 'true' it will remove current configuration, otherwise it
+     * returns 'EPERM' if the limit is already enforced for this zone.*/
+    int (*ct_del_limits)(struct dpif *, const struct ovs_list *zone_limits,
+                         bool force);
 
     /* Connection tracking timeout policy */
 
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 55eaeefa3..96149ad8d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5653,12 +5653,14 @@  static void
 ct_zone_limits_commit(struct dpif_backer *backer)
 {
     if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
-        ct_dpif_set_limits(backer->dpif, NULL, &backer->ct_zone_limits_to_add);
+        ct_dpif_set_limits(backer->dpif, NULL, &backer->ct_zone_limits_to_add,
+                           true);
         ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
     }
 
     if (!ovs_list_is_empty(&backer->ct_zone_limits_to_del)) {
-        ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del);
+        ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del,
+                           true);
         ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
     }
 }
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 537db66e0..8eb609675 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -5238,6 +5238,34 @@  OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
 default limit=10
 zone=0,limit=3,count=0])
 
+dnl Try to overwrite the zone limit via dpctl command.
+AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=0,limit=5], [2], [ignore], [ignore])
+
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+zone=0,limit=3,count=0
+])
+
+AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0], [2], [ignore], [ignore])
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+zone=0,limit=3,count=0
+])
+
+dnl Set limit for zone that is not in DB via dpctl command.
+AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=1,limit=5])
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+zone=0,limit=3,count=0
+zone=1,limit=5,count=0
+])
+
+AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=1])
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+zone=0,limit=3,count=0
+])
+
 OVS_TRAFFIC_VSWITCHD_STOP(["dnl
 /could not create datapath/d
 /(Cannot allocate memory) on packet/d