[ovs-dev] [PATCH v3] ofproto-dpif-xlate: fix zone set from non-frozen-metadata fields

贺鹏 xnhp0320 at gmail.com
Fri May 21 03:00:32 UTC 2021


Hi, Ilya



Ilya Maximets <i.maximets at ovn.org> 于2021年5月19日周三 下午8:50写道:
>
> On 2/27/21 10:34 AM, Peng He wrote:
> > CT zone could be set from a field that is not included in frozen
> > metadata. Consider the example rules which are typically seen in
> > OpenStack security group rules:
> >
> > priority=100,in_port=1,tcp,ct_state=-trk,action=ct(zone=5,table=0)
> > priority=100,in_port=1,tcp,ct_state=+trk,action=ct(commit,zone=NXM_NX_CT_ZONE[]),2
> >
> > The zone is set from the first rule's ct action. These two rules will
> > generate two megaflows: the first one uses zone=5 to query the CT module,
> > the second one sets the zone-id from the first megaflow and commit to CT.
> >
> > The current implementation will generate a megaflow that does not use
> > ct_zone=5 as a match, but directly commit into the ct using zone=5, as zone is
> > set by an Imm not a field.
> >
> > Consider a situation that one changes the zone id (for example to 15)
> > in the first rule, however, still keep the second rule unchanged. During
> > this change, there is traffic hitting the two generated megaflows, the
> > revaldiator would revalidate all megaflows, however, the revalidator will
> > not change the second megaflow, because zone=5 is recorded in the
> > megaflow, so the xlate will still translate the commit action into zone=5,
> > and the new traffic will still commit to CT as zone=5, not zone=15,
> > resulting in taffic drops and other issues.
> >
> > Just like OVS set-field convention, if a field X is set by Y
> > (Y is a variable not an Imm), we should also mask Y as a match
> > in the generated megaflow. An exception is that if the zone-id is
> > set by the field that is included in the frozen state (i.e. regs) and this
> > upcall is a resume of a thawed xlate, the un-wildcarding can be skipped,
> > as the recirc_id is a hash of the values in these fields, and it will change
> > following the changes of these fields. When the recirc_id changes,
> > all megaflows with the old recirc id will be invalid later.
> >
> > Fixes: 07659514c3 ("Add support for connection tracking.")
> > Reported-by: Sai Su <susai.ss at bytedance.com>
> > Signed-off-by: Peng He <hepeng.0320 at bytedance.com>
> > ---
> >  include/openvswitch/meta-flow.h |  1 +
> >  lib/meta-flow.c                 | 13 ++++++++++
> >  ofproto/ofproto-dpif-xlate.c    | 12 +++++++++
> >  tests/system-traffic.at         | 45 +++++++++++++++++++++++++++++++++
> >  4 files changed, 71 insertions(+)
> >
> > diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
> > index 95e52e358..045dce8f5 100644
> > --- a/include/openvswitch/meta-flow.h
> > +++ b/include/openvswitch/meta-flow.h
> > @@ -2305,6 +2305,7 @@ void mf_set_flow_value_masked(const struct mf_field *,
> >                                const union mf_value *mask,
> >                                struct flow *);
> >  bool mf_is_tun_metadata(const struct mf_field *);
> > +bool mf_is_frozen_metadata(const struct mf_field *);
> >  bool mf_is_pipeline_field(const struct mf_field *);
> >  bool mf_is_set(const struct mf_field *, const struct flow *);
> >  void mf_mask_field(const struct mf_field *, struct flow_wildcards *);
> > diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> > index c808d205d..e03cd8d0c 100644
> > --- a/lib/meta-flow.c
> > +++ b/lib/meta-flow.c
> > @@ -1788,6 +1788,19 @@ mf_is_tun_metadata(const struct mf_field *mf)
> >             mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS;
> >  }
> >
> > +bool
> > +mf_is_frozen_metadata(const struct mf_field *mf)
> > +{
> > +    if (mf->id >= MFF_TUN_ID && mf->id <= MFF_IN_PORT_OXM) {
> > +        return true;
> > +    }
> > +
> > +    if (mf->id >= MFF_REG0 && mf->id < MFF_ETH_SRC) {
> > +        return true;
> > +    }
> > +    return false;
> > +}
> > +
> >  bool
> >  mf_is_pipeline_field(const struct mf_field *mf)
> >  {
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 7108c8a30..14d00db1e 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -6195,6 +6195,18 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc,
> >
> >      if (ofc->zone_src.field) {
> >          zone = mf_get_subfield(&ofc->zone_src, &ctx->xin->flow);
> > +        if (ctx->xin->frozen_state) {
> > +            /* If the upcall is a resume of a recirculation, we only need to
> > +             * unwildcard the fields that are not in the frozen_metadata, as
> > +             * when the rules update, OVS will generate a new recirc_id,
> > +             * which will invalidate the megaflow with old the recirc_id.
> > +             */
> > +            if (!mf_is_frozen_metadata(ofc->zone_src.field)) {
> > +                mf_mask_field(ofc->zone_src.field, ctx->wc);
> > +            }
> > +        } else {
> > +            mf_mask_field(ofc->zone_src.field, ctx->wc);
> > +        }
>
> IIUC, in current code above block is equal to just a single line:
>
>     mf_mask_field(ofc->zone_src.field, ctx->wc);
>
> That is because zone is not part of a frozen metadata, right?

Yes, basically this is the reason.
I think the original code only considers the case that the zone id is
set by IMM, not
by a reg.

>
> Can we just remove all this extra logic and unwildcard unconditionally?
> I understand that this code might save us one match in case someday
> zone will be part of a frozen metadata, but is that really important?
> Is there any harm in unwildcarding this field unconditionally?

I think the code can reduce the number of megaflows in case that the rule
sets the zone-id using IMM. But I think unconditionally un-wildcarding is also
OK.


>
> Best regards, Ilya Maximets.



-- 
hepeng


More information about the dev mailing list