ct-dpif: Enforce CT zone limit protection.

Make sure that if any zone limit was set via DB all zones are forced to be set there also. This is done by tracking which datapath has zone limit protection and it is reflected in the dpctl command. If the datapath is protected the dpctl command will return permission error. Signed-off-by: Ales Musil <[email protected]> Signed-off-by: Ilya Maximets <[email protected]>
igsilya · Dec 5, 2023 · f56829b · f56829b
1 parent 854ffeb
commit f56829b
Show file tree

Hide file tree

Showing 9 changed files with 128 additions and 0 deletions.
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
@@ -23,6 +23,7 @@
 #include "openvswitch/ofp-ct.h"
 #include "openvswitch/ofp-parse.h"
 #include "openvswitch/vlog.h"
+#include "sset.h"
 
 VLOG_DEFINE_THIS_MODULE(ct_dpif);
 
@@ -32,6 +33,10 @@ struct flags {
     const char *name;
 };
 
+/* Protection for CT zone limit per datapath. */
+static struct sset ct_limit_protection =
+        SSET_INITIALIZER(&ct_limit_protection);
+
 static void ct_dpif_format_counters(struct ds *,
                                     const struct ct_dpif_counters *);
 static void ct_dpif_format_timestamp(struct ds *,
@@ -1064,3 +1069,23 @@ ct_dpif_get_features(struct dpif *dpif, enum ct_features *features)
             ? dpif->dpif_class->ct_get_features(dpif, features)
             : EOPNOTSUPP);
 }
+
+void
+ct_dpif_set_zone_limit_protection(struct dpif *dpif, bool protected)
+{
+    if (sset_contains(&ct_limit_protection, dpif->full_name) == protected) {
+        return;
+    }
+
+    if (protected) {
+        sset_add(&ct_limit_protection, dpif->full_name);
+    } else {
+        sset_find_and_delete(&ct_limit_protection, dpif->full_name);
+    }
+}
+
+bool
+ct_dpif_is_zone_limit_protected(struct dpif *dpif)
+{
+    return sset_contains(&ct_limit_protection, dpif->full_name);
+}
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
@@ -350,5 +350,7 @@ int ct_dpif_get_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
                                     uint16_t dl_type, uint8_t nw_proto,
                                     char **tp_name, bool *is_generic);
 int ct_dpif_get_features(struct dpif *dpif, enum ct_features *features);
+void ct_dpif_set_zone_limit_protection(struct dpif *, bool protected);
+bool ct_dpif_is_zone_limit_protected(struct dpif *);
 
 #endif /* CT_DPIF_H */
diff --git a/lib/dpctl.c b/lib/dpctl.c
@@ -2234,6 +2234,13 @@ dpctl_ct_set_limits(int argc, const char *argv[],
         ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0);
     }
 
+    if (ct_dpif_is_zone_limit_protected(dpif)) {
+        ds_put_cstr(&ds, "the zone limits are set via database, "
+                         "use 'ovs-vsctl set-zone-limit <...>' instead.");
+        error = EPERM;
+        goto error;
+    }
+
     error = ct_dpif_set_limits(dpif, &zone_limits);
     if (!error) {
         ct_dpif_free_zone_limits(&zone_limits);
@@ -2310,6 +2317,13 @@ dpctl_ct_del_limits(int argc, const char *argv[],
         }
     }
 
+    if (ct_dpif_is_zone_limit_protected(dpif)) {
+        ds_put_cstr(&ds, "the zone limits are set via database, "
+                         "use 'ovs-vsctl del-zone-limit <...>' instead.");
+        error = EPERM;
+        goto error;
+    }
+
     error = ct_dpif_del_limits(dpif, &zone_limits);
     if (!error) {
         goto out;

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
@@ -5673,6 +5673,18 @@ ct_zone_limits_commit(struct dpif_backer *backer)
     }
 }
 
+static void
+ct_zone_limit_protection_update(const char *datapath_type, bool protected)
+{
+    struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
+                                                 datapath_type);
+    if (!backer) {
+        return;
+    }
+
+    ct_dpif_set_zone_limit_protection(backer->dpif, protected);
+}
+
 static void
 get_datapath_cap(const char *datapath_type, struct smap *cap)
 {
@@ -6964,4 +6976,5 @@ const struct ofproto_class ofproto_dpif_class = {
     ct_set_zone_timeout_policy,
     ct_del_zone_timeout_policy,
     ct_zone_limit_update,
+    ct_zone_limit_protection_update,
 };
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
@@ -1929,6 +1929,11 @@ struct ofproto_class {
      * within proper range (0 - UINT32_MAX). */
     void (*ct_zone_limit_update)(const char *dp_type, int32_t zone,
                                  int64_t *limit);
+
+    /* Sets the CT zone limit protection to "protected" for the specified
+     * datapath type. */
+    void (*ct_zone_limit_protection_update)(const char *dp_type,
+                                            bool protected);
 };
 
 extern const struct ofproto_class ofproto_dpif_class;

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
@@ -1038,6 +1038,17 @@ ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id,
     }
 }
 
+void
+ofproto_ct_zone_limit_protection_update(const char *datapath_type,
+                                        bool protected)
+{
+    datapath_type = ofproto_normalize_type(datapath_type);
+    const struct ofproto_class *class = ofproto_class_find__(datapath_type);
+
+    if (class && class->ct_zone_limit_protection_update) {
+        class->ct_zone_limit_protection_update(datapath_type, protected);
+    }
+}
 
 /* Spanning Tree Protocol (STP) configuration. */
 

diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
@@ -386,6 +386,8 @@ void ofproto_ct_del_zone_timeout_policy(const char *datapath_type,
                                         uint16_t zone);
 void ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id,
                                   int64_t *limit);
+void ofproto_ct_zone_limit_protection_update(const char *datapath_type,
+                                             bool protected);
 void ofproto_get_datapath_cap(const char *datapath_type,
                               struct smap *dp_cap);
 

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
@@ -5397,6 +5397,55 @@ OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
 default limit=0
 zone=0,limit=3,count=0])
 
+dnl Try to overwrite the zone limit via dpctl command.
+AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=3,limit=5 zone=0,limit=5], [2], [ignore], [dnl
+ovs-vswitchd: the zone limits are set via database, dnl
+use 'ovs-vsctl set-zone-limit <...>' instead. (Operation not permitted)
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=0
+zone=0,limit=3,count=0
+])
+
+AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0], [2], [ignore], [dnl
+ovs-vswitchd: the zone limits are set via database, dnl
+use 'ovs-vsctl del-zone-limit <...>' instead. (Operation not permitted)
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=0
+zone=0,limit=3,count=0
+])
+
+AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE 0])
+AT_CHECK([ovs-vsctl set-zone-limit $DP_TYPE default 10])
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+])
+
+AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5], [2], [ignore], [dnl
+ovs-vswitchd: the zone limits are set via database, dnl
+use 'ovs-vsctl set-zone-limit <...>' instead. (Operation not permitted)
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+dnl Delete all zones from DB, that should remove the protection.
+AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE default])
+
+AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5])
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=15
+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=15
+])
+
 OVS_TRAFFIC_VSWITCHD_STOP(["dnl
 /could not create datapath/d
 /(Cannot allocate memory) on packet/d"])

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
@@ -740,6 +740,7 @@ datapath_destroy(struct datapath *dp)
                                          NULL);
         }
 
+        ofproto_ct_zone_limit_protection_update(dp->type, false);
         hmap_remove(&all_datapaths, &dp->node);
         hmap_destroy(&dp->ct_zones);
         free(dp->type);
@@ -752,6 +753,7 @@ static void
 ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
 {
     struct ct_zone *ct_zone;
+    bool protected = false;
 
     /* Add new 'ct_zone's or update existing 'ct_zone's based on the database
      * state. */
@@ -785,6 +787,8 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
         }
 
         ct_zone->last_used = idl_seqno;
+
+        protected = protected || !!zone_cfg->limit;
     }
 
     /* Purge 'ct_zone's no longer found in the database. */
@@ -805,6 +809,9 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
         dp->ct_zone_default_limit = default_limit;
     }
 
+    protected = protected || !!dp_cfg->ct_zone_default_limit;
+
+    ofproto_ct_zone_limit_protection_update(dp->type, protected);
 }
 
 static void