[ovs-dev] [PATCH v1] ofproto-dpif-xlate: fix zone set from non frozen metadata field
Mark Gray
mark.d.gray at redhat.com
Wed Feb 17 10:43:59 UTC 2021
On 17/02/2021 10:40, 贺鹏 wrote:
> Hi,
>
> Thanks for the review.
>
> Mark Gray <mark.d.gray at redhat.com> 于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 <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 | 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,
>>> &ctx->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.
>
> if you mean that we should strictly limit the un-wildcarded fields to
> the subfield
> of the zone_src, not all the subfields of it, then yes.
> I think here we only extended to the field pointed by zone_src, not
> 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(&ctx->xin->flow, ctx->odp_actions, ctx->wc);
>>> put_ct_label(&ctx->xin->flow, ctx->odp_actions, ctx->wc);
>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>>> index fb5b9a36d..bee50e530 100644
>>> --- a/tests/system-traffic.at
>>> +++ b/tests/system-traffic.at
>>> @@ -1927,6 +1927,45 @@ 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>)
>>> +])
>>> +
>>> +AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 | strip_used | grep "+trk" ], [0], [dnl
>>> +ct_state(+trk),ct_zone(0x5),recirc_id(0x2),in_port(ovs-p0),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no), packets:5, bytes:465, used:0.0s, flags:FP., actions:ct(commit,zone=5),ovs-p1
>>> +])
>>
>> This test fails for me. It looks like the order of the output from
>> dpctl/dump-flows is different and the recirc_id is different. Could
>> easily be an issue on my side but I am not sure what it is.
>>
>> +++ /home/magray/ovs/tests/system-kmod-testsuite.dir/at-groups/41/stdout
>> 2021-02-17 04:57:36.171766685 -0500
>> @@ -1,2 +1,2 @@
>> -ct_state(+trk),ct_zone(0x5),recirc_id(0x2),in_port(ovs-p0),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),
>> packets:5, bytes:465, used:0.0s, flags:FP., actions:ct(commit,zone=5),ovs-p1
>> +recirc_id(0x1),in_port(ovs-p0),ct_state(+trk),ct_zone(0x5),eth(),eth_type(0x0800),ipv4(proto=6,frag=no),
>> packets:5, bytes:465, used:0.0s, flags:FP., actions:ct(commit,zone=5),ovs-p1
>>
>
> yes, the problem is the recirc_id, looks like we need a similar
> function like stirp_used, (strip_recirc_id)
> The case is to make sure that the ct_zone(0x5) should show up in the match.
There are a few other changes as well (eth(), order of in_port)
>
>> It seems this test does not test the situation that you mention in the
>> commit message: "Consider a situation .. "? Can you add a check for that?
>
> The commit describes how we discover the bug, but essentially this is
> just following the convention that when you try to load Y to X, (both
> Y and X are variable, not a Imm)
> you should un-wildcard Y in xlate process. I would like to add the
> check in the test suites, but I do not know if the
> current test framework provides enough tools for that. Will try to add
> the check in v2.
>
>
>
>>
>>> +
>>> +OVS_TRAFFIC_VSWITCHD_STOP
>>> +AT_CLEANUP
>>> +
>>> +
>>> AT_SETUP([conntrack - multiple bridges])
>>> CHECK_CONNTRACK()
>>> OVS_TRAFFIC_VSWITCHD_START(
>>>
>>
>
>
More information about the dev
mailing list