Message-ID: <9293ca82-e732-008e-afe8-90334afd9873@corigine.com>
Date:   Mon, 15 Mar 2021 17:50:25 +0200
From:   Louis Peens <louis.peens@...igine.com>
To:     Marcelo Leitner <mleitner@...hat.com>,
        Ilya Maximets <i.maximets@....org>
Cc:     "ovs-dev@...nvswitch.org" <ovs-dev@...nvswitch.org>,
        Paul Blakey <paulb@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Yinjun Zhang <yinjun.zhang@...igine.com>,
        Simon Horman <simon.horman@...ronome.com>, wenxu@...oud.cn
Subject: Re: [ovs-dev] tc-conntrack: inconsistent behaviour with icmpv6

Apologies for the horrible formatting on the previous mail, I had to move email
clients and have not figured it's settings out properly yet for mailing list use.
Hopefully this is better already. I think it's still mostly possible to follow, but let me
know if I should rather resend.

Regards
Louis

On 2021/03/15 17:29, Louis Peens wrote:
> Hi Marcelo
> 
> Thanks for taking time to take a look. I've replied inline - and also found
> a bit more info, although I'm not sure if it clears things up much. I do think
> that the main problem is the different upcall behaviour, I have not figured
> out what to do about it yet.
> 
> On 2021/03/13 00:06, Marcelo Leitner wrote:
>> Hi there,
>>
>> On Wed, Mar 10, 2021 at 12:06:52PM +0100, Ilya Maximets wrote:
>>> Hi, Louis.  Thanks for your report!
>>>
>>> Marcelo, Paul, could you, please, take a look?
>> Thanks for the ping.
>> +wenxu
>>
>>> Best regards, Ilya Maximets.
>>>
>>> On 3/10/21 8:51 AM, Louis Peens wrote:
>>>> Hi all
>>>>
>>>> We've recently encountered an interesting situation with OVS conntrack
>>>> when offloading to the TC datapath, and would like some feedback. Sorry
>>>> about the longish wall of text, but I'm trying to explain the problem
>>>> as clearly as possible. The very short summary is that there is a mismatch
>> Details are very welcomed, thanks for them.
>>
>>>> in behaviour between the OVS datapath and OVS+TC datapath, and we're
>>>> not sure how to resolve this. Here goes:
>>>>
>>>> We have a set of rules looking like this:
>>>> ovs-ofctl add-flow br0 "table=0,in_port=p1,ct_state=-trk,ipv6,actions=ct(table=1)"
>>>> ovs-ofctl add-flow br0 "table=0,in_port=p2,ct_state=-trk,ipv6,actions=ct(table=1)"
>>>> #post_ct flows"
>>>> ovs-ofctl add-flow br0 "table=1,in_port=p1,ct_state=+trk+new,ipv6,actions=ct(commit),output:p2"
>>>> ovs-ofctl add-flow br0 "table=1,in_port=p2,ct_state=+trk+new,ipv6,actions=ct(commit),output:p1"
>>>> ovs-ofctl add-flow br0 "table=1,in_port=p1,ct_state=+trk+est,ipv6,actions=output:p2"
>>>> ovs-ofctl add-flow br0 "table=1,in_port=p2,ct_state=+trk+est,ipv6,actions=output:p1"
>>>>
>>>> p1/p2 are the endpoints of two different veth pairs, just to keep this simple.
>>>> The rules above work well enough with UDP/TCP traffic, however ICMPv6 packets
>>>> (08:56:39.984375 IP6 2001:db8:0:f101::1 > ff02::1:ff00:2: ICMP6, neighbor solicitation, who has 2001:db8:0:f101::2, length 32)
>>>> breaks this somewhat. With TC offload disabled:
>>>>
>>>> ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=false
>>>>
>>>> we get the following datapath rules:
>>>>
>>>> ovs-appctl dpctl/dump-flows --names
>>>> recirc_id(0x1),in_port(p1),ct_state(-new-est+trk),eth(),eth_type(0x86dd),ipv6(frag=no), packets:2, bytes:172, used:1.329s, actions:drop
>>>> recirc_id(0),in_port(p1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no), packets:2, bytes:172, used:1.329s, actions:ct,recirc(0x1)
>>>>
>>>> This part is still fine, we do not have a rule for just matching +trk, so the
>>>> the drop rule is to be expected. The problem however is when we enable TC
>>>> offload:
>>>>
>>>> ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true
>>>>
>>>> This is the result in the datapath:
>>>>
>>>> ovs-appctl dpctl/dump-flows --names
>>>> ct_state(-trk),recirc_id(0),in_port(p1),eth_type(0x86dd),ipv6(frag=no), packets:2, bytes:144, used:0.920s, actions:ct,recirc(0x1)
>>>> recirc_id(0x1),in_port(p1),ct_state(-new-est-trk),eth(),eth_type(0x86dd),ipv6(frag=no), packets:1, bytes:86, used:0.928s, actions:drop
>>>> recirc_id(0x1),in_port(p1),ct_state(-new-est+trk),eth(),eth_type(0x86dd),ipv6(frag=no), packets:0, bytes:0, used:never, actions:drop
>>>>
>>>> Notice the installation of the two recirc rules, one with -trk and one with +trk,
>>>> with the -trk one being the rule that handles all the next packets. Further
>>>> investigation reveals that something like the following is happening:
>>>>
>>>> 1) The first packet arrives and is handled by the OVS datapath,
>> Hmm. This shouldn't happen if hw-offload=true, because the first rule
>> should be installed on tc datapath already. Or maybe you mean OVS
>> vswitchd when you referred to OVS datapath?
>>
>> What does  dpctl/dump-flows --names -m  gives in this situation, are
>> all flows installed on dp:tc?
> 
> Yes, packet would be handled by vswitchd here, triggering the installation of datapath
> flow rules. I think I may have mixed up packet handling and flow rule installation a bit.
> These are the expanded flows:
> ovs-appctl dpctl/dump-flows --more
> ufid:65c93f06-9f09-4b62-b309-951c36d3d98a, skb_priority(0/0),skb_mark(0/0),ct_state(0/0x20),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(p1),packet_type(ns=0/0,id=0/0),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=00:00:00:00:00:00/00:00:00:00:00:00),eth_type(0x86dd),ipv6(src=::/::,dst=::/::,label=0/0,proto=0/0,tclass=0/0,hlimit=0/0,frag=no), packets:1, bytes:72, used:2.080s, dp:tc, actions:ct,recirc(0x1)
> ufid:30fd8977-0bcc-41b1-8800-1262cea71005, recirc_id(0x1),dp_hash(0/0),skb_priority(0/0),in_port(p1),skb_mark(0/0),ct_state(0/0x23),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=00:00:00:00:00:00/00:00:00:00:00:00),eth_type(0x86dd),ipv6(src=::/::,dst=::/::,label=0/0,proto=0/0,tclass=0/0,hlimit=0/0,frag=no), packets:0, bytes:0, used:never, dp:ovs, actions:drop
> ufid:c176bac4-a42d-4e8b-99d9-bce386c1be4f, recirc_id(0x1),dp_hash(0/0),skb_priority(0/0),in_port(p1),skb_mark(0/0),ct_state(0x20/0x23),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=00:00:00:00:00:00/00:00:00:00:00:00),eth_type(0x86dd),ipv6(src=::/::,dst=::/::,label=0/0,proto=0/0,tclass=0/0,hlimit=0/0,frag=no), packets:0, bytes:0, used:never, dp:ovs, actions:drop
> 
> The pre-recirc rule is in tc, but the post-recirc rules are in OVS.
> 
>> triggering the installation of the two rules like in the non-offloaded
>>>>    case. So the recirc_id(0) rule gets installed into tc, and recirc_id(0x1)
>>>>    gets installed into the ovs datapath. This bit of code in the OVS module
>>>>    makes sure that +trk is set.
>>>>
>>>>     /* Update 'key' based on skb->_nfct.  If 'post_ct' is true, then OVS has
>>>>      * previously sent the packet to conntrack via the ct action.....
>>>>      * /
>>>>    static void ovs_ct_update_key(const struct sk_buff *skb,
>>>>                               const struct ovs_conntrack_info *info,
>>>>                               struct sw_flow_key *key, bool post_ct,
>>>>                               bool keep_nat_flags)
>>>>     {
>>>>             ...
>>>>             ct = nf_ct_get(skb, &ctinfo);
>>>>             if (ct) {//tracked
>>>>                     ...
>>>>             } else if (post_ct) {
>>>>                     state = OVS_CS_F_TRACKED | OVS_CS_F_INVALID;
>>>>                     if (info)
>>>>                             zone = &info->zone;
>>>>             }
>>>>             __ovs_ct_update_key(key, state, zone, ct);
>>>>
>>>>     }
>>>>     Obviously this is not the case when the packet was sent to conntrack
>>>>     via tc.
>>>>
>>>> 2) The second packet arrives, and now hits the rule installed in
>>>>    TC. However, TC does not handle ICMPv6 (Neighbor Solicitation), and explicitely
>>>>    clears the tracked bit (net/netfilter/nf_conntrack_proto_icmpv6.c):
>>>>
>>>>     int nf_conntrack_icmpv6_error(struct nf_conn *tmpl,
>>>>                                   struct sk_buff *skb,
>>>>                                   unsigned int dataoff,
>>>>                                   const struct nf_hook_state *state)
>>>>
>>>>     {
>>>>     ...
>>>>             type = icmp6h->icmp6_type - 130;
>>>>             if (type >= 0 && type < sizeof(noct_valid_new) &&
>>>>                 noct_valid_new[type]) {
>>>>                     nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
>>>>                     return NF_ACCEPT;
>>>>             }
>>>>     ...
>>>>     }
>>>>     (The above code gets triggered a few function calls down from act_ct.c)
>> I don't follow this part, and it seems it would affect ovs kernel
>> dp as well. Can you please elaborate on the call chain you're focusing
>> here?
> The chain I was referring to is:
> tcf_ct_act->nf_conntrack_in->nf_conntrack_handle_icmp->nf_conntrack_icmpv6_error
> However I'm not so sure anymore that this is relevant, and yes, would probably affect
> ovs as well.
>>>> 3) So now the packet does not hit the +trk rule after the recirc, and leads
>>>>    to the installation of the "recirc_id(0x1),..-trk" rule, since +trk wasn't
>>>>    set by TC.
>> If you meant vswitchd above, this can be the problem, yes.
>> ovs_ct_update_key() is updating the key, and AFAICT that's reflected
>> on the upcall. Which, then, it's fair to assume (I didn't check)
>> vswitchd does the same.
>>
>> But for tc, +trk+inv is synthetsized when tc is trying to match again
>> on this packet, when skb_flow_dissect_ct() in it will:
>>
>>        if (!ct) {
>>                key->ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED |
>>                                TCA_FLOWER_KEY_CT_FLAGS_INVALID;
>>                return;
>>        }
>>
>> Note that 'key' here is not part of the packet in any way. The only
>> information that is stored within the packet, is
>> qdisc_skb_cb(skb)->post_ct, which ovs kernel doesn't know about. So
>> this wouldn't be reflected on an upcall, causing vswitchd to not see
>> these flags.
>>
>> IOW, an upcall right after this flow:
>> ct_state(-trk),recirc_id(0),in_port(p1),eth_type(0x86dd),ipv6(frag=no), actions:ct,recirc(0x1)
>> can be different if it's from tc datapath or ovs kernel/vswitchd
>> regarding these flags in this case.
>>
>> Makes sense? I think we're mostly on the same page on this part,
>> actually.
> I think this is what it boils down to in the end yes. I did do some bisecting of the kernel tree in the mean time:
> Just before "7baf2429a1a9 net/sched: cls_flower add CT_FLAGS_INVALID flag support" all the rules end up in
> in dp:tc, but we have have -trk and +trk as explained. Then after the commit does look to be working as expected,
> rules get's installed in tc, and only +trk set:
> ovs-appctl dpctl/dump-flows --more
> ufid:e476d5a2-3133-405c-9826-ab911c2c3240, skb_priority(0/0),skb_mark(0/0),ct_state(0/0x20),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(p1),packet_type(ns=0/0,id=0/0),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=00:00:00:00:00:00/00:00:00:00:00:00),eth_type(0x86dd),ipv6(src=::/::,dst=::/::,label=0/0,proto=0/0,tclass=0/0,hlimit=0/0,frag=no), packets:1, bytes:72, used:1.890s, dp:tc, actions:ct,recirc(0x2)
> ufid:b8369209-3069-4280-9914-820d98a3a536, skb_priority(0/0),skb_mark(0/0),ct_state(0x20/0x23),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0x2),dp_hash(0/0),in_port(p1),packet_type(ns=0/0,id=0/0),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=00:00:00:00:00:00/00:00:00:00:00:00),eth_type(0x86dd),ipv6(src=::/::,dst=::/::,label=0/0,proto=0/0,tclass=0/0,hlimit=0/0,frag=no), packets:1, bytes:72, used:1.890s, dp:tc, actions:drop
> 
> Then things go south again with this commit:
> "1bcc51ac0731 net/sched: cls_flower: Reject invalid ct_state flags rules"
> This is the point where the recirc rule is rejected by tc, and leads to the installation
> of the two ovs rules as in the dump at the start of the email.
> 
> Also, not everything is good at this commit:
> "7baf2429a1a9 net/sched: cls_flower add CT_FLAGS_INVALID flag support"
> If I add userspace rules to also match on +inv so that it isn't wildcarded I
> (with the ovs patches for tc +inv support removed) I get the same two-recirc-rule
> behaviour. I do think that it matches the current theory on the upcall behaviour.
> We will keep on digging on our side as well, this did give some more avenues
> of thought, thanks.
> 
> Regards
> Louis
>> Thanks,
>> Marcelo
>>
>>>> This is now the point where we're a bit stuck and is hoping for some ideas
>>>> on how to best resolve this. A workaround is of course just to modify the
>>>> userspace rules to not send the icmp packets to conntrack and that should
>>>> work, but it is a workaround. I think this inconsistency between TC
>>>> offload and non-TC is quite undesirable, and could lead to some interesting
>>>> results, for instance this was first detected by the observation of packets
>>>> getting stuck in a loop in the datapath:
>>>>
>>>> recirc_id(0xe),...ct_state(0/0x20),....,in_port(eth9),eth_type(0x86dd),... ,dp:tc, actions:ct,recirc(0xe)
>>>>
>>>> Where the userspace rule was doing ct to the same table instead of moving to the next table:
>>>>
>>>> ovs-ofctl add-flow br0 "table=0,in_port=eth9,ct_state=-trk,ipv6,actions=ct(table=0)"
>>>>
>>>> So far we've not managed to think of a good way to resolve this in the code.
>>>> I don't think changing the kernel behaviour would be desirable, at least
>>>> not in that specific function as that is common conntrack code. I suspect
>>>> that ideally this is something we can try and address from the OVS side,
>>>> but at this moment I have no idea how this will be achieved, hence this
>>>> email.
>>>>
>>>> Looking forward to get some suggestions on this
>>>>
>>>> Regards
>>>> Louis Peens
>>>>
>>>> PS: Tested on:
>>>> net-next kernel:
>>>>     d310ec03a34e Merge tag 'perf-core-2021-02-17' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
>>>> OVS:
>>>>     "cdaa7e0fd dpif-netdev: Fix crash when add dp flow without in_port field."
>>>>         +
>>>>     "[ovs-dev] [PATCH v3 0/3] Add offload support for ct_state rpl and inv flags"
>>>>     (The behaviour before and after the patch series in terms of the problem
>>>>      above is the same. Whether the recirc rules end up in the ovs datapath or tc
>>>>      datapath doesn't really matter)
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev@...nvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>

Powered by blists - more mailing lists