On Thu, Oct 5, 2023 at 8:49 PM Ilya Maximets <[email protected]> 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 <[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. > > 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 -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
