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 <[email protected]>
> Acked-by: Simon Horman <[email protected]>
> ---
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
