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

贺鹏 xnhp0320 at gmail.com
Fri Mar 19 08:42:23 UTC 2021


Hi,

Ping for this patch.

贺鹏 <xnhp0320 at gmail.com> 于2021年3月6日周六 上午10:37写道:
>
> Mark Gray <mark.d.gray at redhat.com> 于2021年3月5日周五 下午7:54写道:
> >
> > On 27/02/2021 09:34, 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.
> >
> > This looks good to me and all the unit-tests pass.
> >
> > There is some trailing whitespace. You can run
> > "./utilities/checkpatch.py" when submitting to catch them before the
> > 0-day robot does. Its a bit of a nit and I don't know if this won't get
> > committed because of it. That's a decision for a maintainer.
>
> thanks for your kind suggestion
> .
> better send a new version, but before that maybe there are
> some more suggestions from maintainers, and it's better
> to wait and then resend.
>
> > >
> > > 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);
> > > +        }
> > >      } else {
> > >          zone = ofc->zone_imm;
> > >      }
> > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > > index fb5b9a36d..8b84642e7 100644
> > > --- a/tests/system-traffic.at
> > > +++ b/tests/system-traffic.at
> > > @@ -1927,6 +1927,51 @@ tcp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=<cleared>,dport=<cleared>),reply=(src=
> > >  OVS_TRAFFIC_VSWITCHD_STOP
> > >  AT_CLEANUP
> > >
> > > +AT_SETUP([conntrack - zones from other field])
> > > +CHECK_CONNTRACK()
> > > +OVS_TRAFFIC_VSWITCHD_START()
> > > +
> > > +ADD_NAMESPACES(at_ns0, at_ns1)
> > > +
> > > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> > > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> > > +
> > > +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
> > > +AT_DATA([flows.txt], [dnl
> > > +priority=1,action=drop
> > > +priority=10,arp,action=normal
> > > +priority=10,icmp,action=normal
> > > +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
> > > +priority=100,in_port=2,ct_state=-trk,tcp,action=ct(table=0,zone=5)
> > > +priority=100,in_port=2,ct_state=+trk,ct_zone=5,tcp,action=1
> > > +])
> > > +
> > > +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> > > +
> > > +OVS_START_L7([at_ns1], [http])
> > > +
> > > +dnl HTTP requests from p0->p1 should work fine.
> > > +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log])
> > > +
> > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
> > > +tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),zone=5,protoinfo=(state=<cleared>)
> > > +])
> > > +
> > > +dnl This is to test when the zoneid is set by a field variable like NXM_NX_CT_ZONE, the OVS xlate should generate a megaflow with a form of
> > > +dnl "ct_zone(5), ...  actions: ct(commit, zone=5)". The match "ct_zone(5)" is needed as if we changes the zoneid into 15 in the following,
> > > +dnl the old "ct_zone(5), ... actions: ct(commit, zone=5)" megaflow will not get hit, and OVS will generate a new megaflow with the match "ct_zone(0xf)".
> > > +dnl This will make sure that the new packets are committing to zoneid 15 rather than old 5.
> > > +AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 | grep "+trk" | grep -q "ct_zone(0x5)" ], [0], [])
> > > +
> > > +AT_CHECK([ovs-ofctl mod-flows br0 'priority=100,ct_state=-trk,tcp,in_port="ovs-p0" actions=ct(table=0,zone=15)'])
> > > +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log])
> > > +AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 | grep "+trk" | grep -q "ct_zone(0xf)" ], [0], [])
> > > +
> > > +OVS_TRAFFIC_VSWITCHD_STOP
> > > +AT_CLEANUP
> > > +
> > > +
> > >  AT_SETUP([conntrack - multiple bridges])
> > >  CHECK_CONNTRACK()
> > >  OVS_TRAFFIC_VSWITCHD_START(
> > >
> >
>
>
> --
> hepeng



-- 
hepeng


More information about the dev mailing list