On 10/10/23 16:12, Ales Musil wrote:
> 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 <amu...@redhat.com>
> ---
> v4: Rebase on top of current master.
> Make the protection datapath wide.
> ---
> lib/ct-dpif.c | 27 +++++++++++++++++++++++++++
> lib/ct-dpif.h | 2 ++
> lib/dpctl.c | 10 ++++++++++
> ofproto/ofproto-dpif.c | 14 ++++++++++++++
> ofproto/ofproto-provider.h | 5 +++++
> ofproto/ofproto.c | 11 +++++++++++
> ofproto/ofproto.h | 2 ++
> tests/system-traffic.at | 36 ++++++++++++++++++++++++++++++++++++
> vswitchd/bridge.c | 9 +++++++++
> 9 files changed, 116 insertions(+)
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 686e95c92..a75a8c532 100644
> --- 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,25 @@ 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);
> + }
> + VLOG_INFO("The CT zone limit protection is %s for \"%s\".",
> + protected ? "enabled" : "disabled", 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
> index c90dc9476..feb8b166a 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -352,5 +352,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 *dpif, bool protected);
> +bool ct_dpif_is_zone_limit_protected(struct dpif *dpif);
No need for the 'dpif' variable name.
>
> #endif /* CT_DPIF_H */
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 7113c2c12..3627d37d1 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -2234,6 +2234,11 @@ 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)) {
> + error = EPERM;
Some meaningful message directing users to use ovs-vsctl instead
might be nice here.
> + goto error;
> + }
> +
> error = ct_dpif_set_limits(dpif, &zone_limits);
> if (!error) {
> ct_dpif_free_zone_limits(&zone_limits);
> @@ -2309,6 +2314,11 @@ dpctl_ct_del_limits(int argc, const char *argv[],
> }
> }
>
> + if (ct_dpif_is_zone_limit_protected(dpif)) {
> + error = EPERM;
And here.
> + 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
> index 4fdbf0ef0..4ea70f722 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5665,6 +5665,19 @@ 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)
> {
> @@ -6955,4 +6968,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
> index 33fb99280..e1d72b6df 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1925,6 +1925,11 @@ struct ofproto_class {
> /* Updates the CT zone limit for specified zone. */
> 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
> index 6df3f1b27..06624006a 100644
> --- 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
> index bba4a9e0e..0212ed062 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -388,6 +388,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
> index d2897feb6..00a682a7e 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -5274,6 +5274,42 @@ 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], [ignore])
> +
> +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], [ignore])
> +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 zone=0])
> +AT_CHECK([ovs-vsctl set-zone-default-limit $DP_TYPE limit=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], [ignore])
> +
> +dnl Delete all zones from DB, that should remove the protection.
> +AT_CHECK([ovs-vsctl del-zone-default-limit $DP_TYPE])
> +
> +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
> index 1e02cc8df..0868f27c3 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -729,6 +729,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);
> @@ -742,6 +743,8 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
> {
> struct ct_zone *ct_zone;
>
No need for the empty line here.
> + bool protected = false;
> +
> /* Add new 'ct_zone's or update existing 'ct_zone's based on the database
> * state. */
> for (size_t i = 0; i < dp_cfg->n_ct_zones; i++) {
> @@ -774,6 +777,8 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
>
> ct_zone->limit = desired_limit;
> ct_zone->last_used = idl_seqno;
> +
> + protected |= !!zone_cfg->limit;
Don't use bit operations on booleans.
> }
>
> /* Purge 'ct_zone's no longer found in the database. */
> @@ -804,6 +809,10 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
> }
>
> dp->ct_default_limit = default_limit;
> +
> + protected |= !!dp_cfg->ct_zone_default_limit;
Bit operation on a boolean.
> +
> + ofproto_ct_zone_limit_protection_update(dp->type, protected);
> }
>
> static void
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
