Aaron Conole <[email protected]> writes:
> Tonghao Zhang <[email protected]> writes:
>
>> On Fri, Jan 15, 2021 at 5:20 AM Aaron Conole <[email protected]> wrote:
>>>
>>> Hi Tonghao Xhang,
>>>
>>> [email protected] writes:
>>>
>>> > From: Tonghao Zhang <[email protected]>
>>> >
>>> > The same icmp packet may traverse conntrack module more than once.
>>> > Or same icmp packets traverse contranck module in orderly.
>>> >
>>> > Don't change stats to CS_ESTABLISHED before receiving reply or related
>>> > packets.
>>> >
>>> > Fixes: b269a1229df2 ("conntrack: Track ICMP type and code.")
>>> > Cc: Daniele Di Proietto <[email protected]>
>>> > Signed-off-by: Tonghao Zhang <[email protected]>
>>>
>>> I'm not quite sure what you're seeing from the state machine side, based
>>> on the commit message. I've included a test case to try and reproduce
>>> your issue but it doesn't seem like I can based on your description.
>>>
>>> Can you modify it to show exactly what you are fixing, and include that
>>> with v2? As it stands, if I use this test, it passes and so doesn't
>>> seem like the packet metadata is changed on a second call through ct() -
>>> did I miss something? Maybe I'm looking at a wrong output somewhere?
>> Hi Aaron
>> I don't use the testsuite to reproduce it. but it's easy to reproduce
>> it with dpdk, and scapy tools.
>>
>> The openflow rules as below:
>> + /root/local/openvswitch-2.14/bin/ovs-ofctl add-flow br-int
>> 'priority:50000,table=0,in_port=rep0,ip,ct_state=-trk
>> action=set_field:1->xreg0,ct(table=0,zone=1)'
>> + /root/local/openvswitch-2.14/bin/ovs-ofctl add-flow br-int
>> 'priority:40000,table=0,in_port=rep0,ip,ct_state=+new-est-rel-inv+trk,ct_zone=1,icmp,ip_src=1.1.1.1
>> action=ct(commit,zone=1,table=1)'
>> + /root/local/openvswitch-2.14/bin/ovs-ofctl add-flow br-int
>> 'priority:50000,table=0,in_port=rep0,ip,ct_state=-new+est-rel-inv+trk,ct_zone=1
>> action=resubmit(,1)'
>> + /root/local/openvswitch-2.14/bin/ovs-ofctl add-flow br-int
>> 'priority:50000,table=1,xreg0=1,ip,ip_dst=1.1.1.2,ct_state=+new-est-rel-inv+trk,ct_zone=1,icmp
>> action=ct(commit,zone=1,table=2)'
>> + /root/local/openvswitch-2.14/bin/ovs-ofctl add-flow br-int
>> 'priority:50000,table=1,xreg0=1,ip,ip_dst=1.1.1.2,ct_state=-new+est-rel-inv+trk,ct_zone=1,icmp
>> action=resubmit(,2)'
>>
>> first packet:
>> sendp(Ether(dst="00:11:22:33:44:52")/IP(src="1.1.1.1",
>> dst="1.1.1.2")/ICMP(type=8, code=0,id=65535,seq=65535),
>> iface="p4p1_0")
>>
>> $ ovs-appctl dpctl/dump-flows
>> recirc_id(0xe),in_port(4),packet_type(ns=0,id=0),eth(dst=00:11:22:33:44:52),eth_type(0x0800),ipv4(dst=1.1.1.2,frag=no),
>> packets:0, bytes:0, used:never, actions:5
>> ct_state(+new-est-rel-inv+trk),ct_zone(0x1),recirc_id(0xd),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=1.1.1.2,proto=1,frag=no),
>> packets:0, bytes:0, used:never, actions:ct(commit,zone=1),recirc(0xe)
>> ct_state(+new-est-rel-inv+trk),ct_zone(0x1),recirc_id(0xc),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(src=1.1.1.1,proto=1,frag=no),
>> packets:0, bytes:0, used:never, actions:ct(commit,zone=1),recirc(0xd)
>> ct_state(-trk),recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
>> packets:0, bytes:0, used:never, actions:ct(zone=1),recirc(0xc)
>>
>> the second
>> sendp(Ether(dst="00:11:22:33:44:52")/IP(src="1.1.1.1",
>> dst="1.1.1.2")/ICMP(type=8, code=0,id=65535,seq=65535),
>> iface="p4p1_0")
>>
>> $ ovs-appctl dpctl/dump-flows
>> recirc_id(0xe),in_port(4),packet_type(ns=0,id=0),eth(dst=00:11:22:33:44:52),eth_type(0x0800),ipv4(dst=1.1.1.2,frag=no),
>> packets:0, bytes:0, used:never, actions:5
>> ct_state(+new-est-rel-inv+trk),ct_zone(0x1),recirc_id(0xd),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=1.1.1.2,proto=1,frag=no),
>> packets:0, bytes:0, used:never, actions:ct(commit,zone=1),recirc(0xe)
>> ct_state(+new-est-rel-inv+trk),ct_zone(0x1),recirc_id(0xc),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(src=1.1.1.1,proto=1,frag=no),
>> packets:0, bytes:0, used:never, actions:ct(commit,zone=1),recirc(0xd)
>> ct_state(-trk),recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
>> packets:1, bytes:52, used:1.114s, actions:ct(zone=1),recirc(0xc)
>>
>> # the ct_state is +est, it is not right.
>> ct_state(-new+est-rel-inv+trk),ct_zone(0x1),recirc_id(0xc),in_port(4),packet_type(ns=0,id=0),eth(dst=00:11:22:33:44:52),eth_type(0x0800),ipv4(dst=1.1.1.2,proto=1,frag=no),
>> packets:0, bytes:0, used:never, actions:5
>>
Forgot to mention that the commit message and subject should reflect
that these are fixing the ICMP 'new' state transition, rather than
calling this 'stats' - the behavior of the state machine is more
important than the side effect of which flow statistic is being updated.
>
> The fixes tag for this should be:
>
> Fixes: a867c010ee91 ("conntrack: Fix conntrack new state")
>
> Since that was the patch which attempted to make the behavior between
> kernel-userspace consistent.
>
> Please submit a v2 with the following diff (and run 'make
> check-system-userspace' and 'make check-kernel' to ensure proper
> coverage of the tests). This way, we don't break the behavior in the
> future (and can guarantee consistency). I did try this out and it
> worked for me.
>
> 122: conntrack - Multiple ICMP traverse ok
>
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -5898,6 +5898,58 @@ ovs-appctl dpif/dump-flows br0
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> +# OFPROTO_CLEAR_DURATION_IDLE([])
> +#
> +# Clear the duration from the piped input which would differ from test to
> test
> +#
> +m4_define([OFPROTO_CLEAR_DURATION_IDLE], [[sed -e
> 's/duration=.*s,/duration=<cleared>,/g' -e
> 's/idle_age=[0-9]*,/idle_age=<cleared>,/g']])
> +
> +AT_SETUP([conntrack - Multiple ICMP traverse])
> +dnl This tracks sending ICMP packets via conntrack multiple times for the
> +dnl same packet
> +CHECK_CONNTRACK()
> +OVS_TRAFFIC_VSWITCHD_START()
> +OVS_CHECK_CT_CLEAR()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02")
> +dnl setup ct flows
> +AT_DATA([flows.txt], [dnl
> +table=0,priority=10 arp action=normal
> +table=0,priority=10 ip,icmp,ct_state=-trk action=ct(zone=1,table=1)
> +table=0,priority=0 action=drop
> +table=1,priority=10 ct_state=-est+trk+new,ip,ct_zone=1,in_port=1
> action=ct(commit,table=2)
> +table=1,priority=0 ct_state=+est-new+trk,ct_zone=1,in_port=1
> action=resubmit(,2)
> +table=1,priority=10 in_port=2 action=1
> +table=2,priority=10 ct_state=+trk+new,in_port=1 action=drop
> +table=2,priority=10 ct_state=+trk+est action=drop
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +
> +# sending icmp pkts, first and second
> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 f0
> 00 00 01 01 01 08 00 45 00 00 1c 00 01 00 00 40 01 64 dc 0a 01 01 01 0a 01 01
> 02 08 00 f7 ff ff ff ff ff > /dev/null])
> +
> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 f0
> 00 00 01 01 01 08 00 45 00 00 1c 00 01 00 00 40 01 64 dc 0a 01 01 01 0a 01 01
> 02 08 00 f7 ff ff ff ff ff > /dev/null])
> +
> +sleep 1
> +
> +dnl ensure CT picked up the packet
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1)], [0], [dnl
> +icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=<cleared>,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=<cleared>,type=0,code=0)
> +])
> +
> +AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, |
> OFPROTO_CLEAR_DURATION_IDLE],
> + [0], [dnl
> + cookie=0x0, duration=<cleared>, table=2, n_packets=2, n_bytes=84,
> idle_age=<cleared>, priority=10,ct_state=+new+trk,in_port=1 actions=drop
> + cookie=0x0, duration=<cleared>, table=2, n_packets=0, n_bytes=0,
> idle_age=<cleared>, priority=10,ct_state=+est+trk actions=drop
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
