Re: [ovs-dev] [PATCH v1] ofproto-dpif-xlate: fix zone set from non frozen metadata field
Hi, I've already sent a v2 patch.
Mark Gray 于2021年2月17日周三 下午6:44写道：
>
> On 17/02/2021 10:40, 贺鹏 wrote:
> > Hi,
> >
> > Thanks for the review.
> >
> > Mark Gray 于2021年2月17日周三 下午6:13写道：
> >>
> >> I'm not too familiar with this code but I have some comments.
> >>
> >> On 15/02/2021 09:50, Peng He wrote:
> >>> CT zone could be set from a field that is not included in frozen
> >>> metedata. Consider the belowing cases which is normally used in
> >>
> >> Nits:
> >>
> >> s/metedata/metadata
> >> s/belowing cases which is/cases below which are
> >
> > sorry for the typo, will fix it in the next version
> >
> >>
> >>> 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 set zone from the first megaflow and commit to CT.
> >>>
> >>> The current implementation will generate a megaflow which 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 action, if the field X is set by Y, we should also
> >>> mask Y as a match in the generated megaflow. An exception is that, if the
> >>> zone
> >>> is set by the field that is included in the frozen state and this upcall
> >>> is
> >>> a resume of a thawed xlate, the masking can be skipped, as if one changes
> >>> the previous rule, the generated recirc_id will be changed, and all
> >>> megaflows
> >>> with the old recirc id will be invalid later, i.e. the revalidator will
> >>> not reuse the old megaflows at all.
> >>>
> >>> Fixes: 07659514c3 ("Add support for connection tracking.")
> >>> Reported-by: Sai Su
> >>> Signed-off-by: Peng He
> >>> ---
> >>> include/openvswitch/meta-flow.h | 1 +
> >>> lib/meta-flow.c | 13 +++
> >>> ofproto/ofproto-dpif-xlate.c| 15 +
> >>> tests/system-traffic.at | 39 +
> >>> 4 files changed, 68 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..5d1f029fd 100644
> >>> --- a/ofproto/ofproto-dpif-xlate.c
> >>> +++ b/ofproto/ofproto-dpif-xlate.c
> >>> @@ -6212,6 +6212,21 @@ compose_conntrack_action(struct xlate_ctx *ctx,
> >>> struct ofpact_conntrack *ofc,
> >>> >xin->flow, ctx->wc, zone);
> >>> }
> >>> }
> >>> +
> >>> +if (ofc->zone_src.field) {
> >>> +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
> >>> +
Re: [ovs-dev] [PATCH v1] ofproto-dpif-xlate: fix zone set from non frozen metadata field
On 17/02/2021 10:40, 贺鹏 wrote:
> Hi,
>
> Thanks for the review.
>
> Mark Gray 于2021年2月17日周三 下午6:13写道：
>>
>> I'm not too familiar with this code but I have some comments.
>>
>> On 15/02/2021 09:50, Peng He wrote:
>>> CT zone could be set from a field that is not included in frozen
>>> metedata. Consider the belowing cases which is normally used in
>>
>> Nits:
>>
>> s/metedata/metadata
>> s/belowing cases which is/cases below which are
>
> sorry for the typo, will fix it in the next version
>
>>
>>> 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 set zone from the first megaflow and commit to CT.
>>>
>>> The current implementation will generate a megaflow which 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 action, if the field X is set by Y, we should also
>>> mask Y as a match in the generated megaflow. An exception is that, if the
>>> zone
>>> is set by the field that is included in the frozen state and this upcall is
>>> a resume of a thawed xlate, the masking can be skipped, as if one changes
>>> the previous rule, the generated recirc_id will be changed, and all
>>> megaflows
>>> with the old recirc id will be invalid later, i.e. the revalidator will
>>> not reuse the old megaflows at all.
>>>
>>> Fixes: 07659514c3 ("Add support for connection tracking.")
>>> Reported-by: Sai Su
>>> Signed-off-by: Peng He
>>> ---
>>> include/openvswitch/meta-flow.h | 1 +
>>> lib/meta-flow.c | 13 +++
>>> ofproto/ofproto-dpif-xlate.c| 15 +
>>> tests/system-traffic.at | 39 +
>>> 4 files changed, 68 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..5d1f029fd 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -6212,6 +6212,21 @@ compose_conntrack_action(struct xlate_ctx *ctx,
>>> struct ofpact_conntrack *ofc,
>>> >xin->flow, ctx->wc, zone);
>>> }
>>> }
>>> +
>>> +if (ofc->zone_src.field) {
>>> +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);
>> Is this
Re: [ovs-dev] [PATCH v1] ofproto-dpif-xlate: fix zone set from non frozen metadata field
Hi,
Thanks for the review.
Mark Gray 于2021年2月17日周三 下午6:13写道：
>
> I'm not too familiar with this code but I have some comments.
>
> On 15/02/2021 09:50, Peng He wrote:
> > CT zone could be set from a field that is not included in frozen
> > metedata. Consider the belowing cases which is normally used in
>
> Nits:
>
> s/metedata/metadata
> s/belowing cases which is/cases below which are
sorry for the typo, will fix it in the next version
>
> > 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 set zone from the first megaflow and commit to CT.
> >
> > The current implementation will generate a megaflow which 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 action, if the field X is set by Y, we should also
> > mask Y as a match in the generated megaflow. An exception is that, if the
> > zone
> > is set by the field that is included in the frozen state and this upcall is
> > a resume of a thawed xlate, the masking can be skipped, as if one changes
> > the previous rule, the generated recirc_id will be changed, and all
> > megaflows
> > with the old recirc id will be invalid later, i.e. the revalidator will
> > not reuse the old megaflows at all.
> >
> > Fixes: 07659514c3 ("Add support for connection tracking.")
> > Reported-by: Sai Su
> > Signed-off-by: Peng He
> > ---
> > include/openvswitch/meta-flow.h | 1 +
> > lib/meta-flow.c | 13 +++
> > ofproto/ofproto-dpif-xlate.c| 15 +
> > tests/system-traffic.at | 39 +
> > 4 files changed, 68 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..5d1f029fd 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -6212,6 +6212,21 @@ compose_conntrack_action(struct xlate_ctx *ctx,
> > struct ofpact_conntrack *ofc,
> > >xin->flow, ctx->wc, zone);
> > }
> > }
> > +
> > +if (ofc->zone_src.field) {
> > +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);
> Is this the only field we should check and un-wildcard here. This
Re: [ovs-dev] [PATCH v1] ofproto-dpif-xlate: fix zone set from non frozen metadata field
I'm not too familiar with this code but I have some comments.
On 15/02/2021 09:50, Peng He wrote:
> CT zone could be set from a field that is not included in frozen
> metedata. Consider the belowing cases which is normally used in
Nits:
s/metedata/metadata
s/belowing cases which is/cases below which are
> 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 set zone from the first megaflow and commit to CT.
>
> The current implementation will generate a megaflow which 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 action, if the field X is set by Y, we should also
> mask Y as a match in the generated megaflow. An exception is that, if the zone
> is set by the field that is included in the frozen state and this upcall is
> a resume of a thawed xlate, the masking can be skipped, as if one changes
> the previous rule, the generated recirc_id will be changed, and all megaflows
> with the old recirc id will be invalid later, i.e. the revalidator will
> not reuse the old megaflows at all.
>
> Fixes: 07659514c3 ("Add support for connection tracking.")
> Reported-by: Sai Su
> Signed-off-by: Peng He
> ---
> include/openvswitch/meta-flow.h | 1 +
> lib/meta-flow.c | 13 +++
> ofproto/ofproto-dpif-xlate.c| 15 +
> tests/system-traffic.at | 39 +
> 4 files changed, 68 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..5d1f029fd 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6212,6 +6212,21 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct
> ofpact_conntrack *ofc,
> >xin->flow, ctx->wc, zone);
> }
> }
> +
> +if (ofc->zone_src.field) {
> +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);
Is this the only field we should check and un-wildcard here. This seems
like it would be applicable across other fields.
> +}
> +} else {
> +mf_mask_field(ofc->zone_src.field, ctx->wc);
> +}
> +}
Add a new line after the bracket
> nl_msg_put_u16(ctx->odp_actions, OVS_CT_ATTR_ZONE, zone);
> put_ct_mark(>xin->flow, ctx->odp_actions, ctx->wc);
>
Re: [ovs-dev] [PATCH v1] ofproto-dpif-xlate: fix zone set from non frozen metadata field
Bleep bloop. Greetings Peng He, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Peng He needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He Lines checked: 162, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
