On 10/6/23 07:31, Ales Musil wrote:
>
>
> On Thu, Oct 5, 2023 at 8:49 PM Ilya Maximets <[email protected]
> <mailto:[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] <mailto:[email protected]>>
> > Acked-by: Simon Horman <[email protected] <mailto:[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 <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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
