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...@bytedance.com> > Signed-off-by: Peng He <hepeng.0...@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? 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? Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev