| Message ID | 20201223112353.63526-1-xiangxia.m.yue@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,ovs] conntrack: Update the icmp stats accurately. | expand |
On 12/23/20 12:23 PM, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > 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 <diproiettod@vmware.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- Hi, Aaron. Could you, please, take a look at this patch? > lib/conntrack-icmp.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/conntrack-icmp.c b/lib/conntrack-icmp.c > index 9b7263011..7e24d90a5 100644 > --- a/lib/conntrack-icmp.c > +++ b/lib/conntrack-icmp.c > @@ -59,13 +59,16 @@ icmp_conn_update(struct conntrack *ct, struct conn *conn_, > struct dp_packet *pkt OVS_UNUSED, bool reply, long long now) > { > struct conn_icmp *conn = conn_icmp_cast(conn_); > + enum ct_update_res ret = CT_UPDATE_VALID; > > if (reply && conn->state == ICMPS_FIRST) { > conn->state = ICMPS_REPLY; > + } else if (conn->state == ICMPS_FIRST) { > + ret = CT_UPDATE_VALID_NEW; > } > > conn_update_expiration(ct, &conn->up, icmp_timeouts[conn->state], now); > - return CT_UPDATE_VALID; > + return ret; > } > > static bool >
Ilya Maximets <i.maximets@ovn.org> writes: > On 12/23/20 12:23 PM, xiangxia.m.yue@gmail.com wrote: >> From: Tonghao Zhang <xiangxia.m.yue@gmail.com> >> >> 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 <diproiettod@vmware.com> >> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> >> --- > > Hi, Aaron. Could you, please, take a look at this patch? Will do. >> lib/conntrack-icmp.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/lib/conntrack-icmp.c b/lib/conntrack-icmp.c >> index 9b7263011..7e24d90a5 100644 >> --- a/lib/conntrack-icmp.c >> +++ b/lib/conntrack-icmp.c >> @@ -59,13 +59,16 @@ icmp_conn_update(struct conntrack *ct, struct conn *conn_, >> struct dp_packet *pkt OVS_UNUSED, bool reply, long long now) >> { >> struct conn_icmp *conn = conn_icmp_cast(conn_); >> + enum ct_update_res ret = CT_UPDATE_VALID; >> >> if (reply && conn->state == ICMPS_FIRST) { >> conn->state = ICMPS_REPLY; >> + } else if (conn->state == ICMPS_FIRST) { >> + ret = CT_UPDATE_VALID_NEW; >> } >> >> conn_update_expiration(ct, &conn->up, icmp_timeouts[conn->state], now); >> - return CT_UPDATE_VALID; >> + return ret; >> } >> >> static bool >>
Hi Tonghao Xhang, xiangxia.m.yue@gmail.com writes: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > 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 <diproiettod@vmware.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> 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? diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 2824f879c1..767efb2ed3 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -5898,6 +5898,59 @@ ovs-appctl dpif/dump-flows br0 OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +# OFPROTO_CLEAR_DURATION([]) +# +# Clear the duration from the piped input which would differ from test to test +# +m4_define([OFPROTO_CLEAR_DURATION], [sed -e 's/duration=.*s,/duration=<cleared>,/g']) + +# OFPROTO_CLEAR_DURATION_PKTS([]) +# +# Clear the duration from the piped input which would differ from test to test +# +m4_define([OFPROTO_CLEAR_DURATION_PKTS], [sed -e 's/duration=.*s,/duration=<cleared>,/g' -e 's/n_packets=.*,/n_packets=<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(table=1) +table=0,priority=0 action=drop +table=1,priority=10 ct_state=+trk+new,ip,in_port=1 action=ct(commit,table=2) +table=1,priority=10 in_port=2 action=1 +table=2,priority=10 ct_state=+trk+new,in_port=1 action=2 +table=2,priority=10 ct_state=+trk+est action=drop +]) + +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) + +NS_EXEC([at_ns0], [ping 10.1.1.2 -c 1 -q]) + +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], + [0], [dnl + cookie=0x0, duration=<cleared>, table=2, n_packets=1, n_bytes=98, idle_age=0, priority=10,ct_state=+new+trk,in_port=1 actions=output:2 + cookie=0x0, duration=<cleared>, table=2, n_packets=0, n_bytes=0, idle_age=0, priority=10,ct_state=+est+trk actions=drop +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + +
On Fri, Jan 15, 2021 at 5:20 AM Aaron Conole <aconole@redhat.com> wrote: > > Hi Tonghao Xhang, > > xiangxia.m.yue@gmail.com writes: > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > 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 <diproiettod@vmware.com> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > 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 > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 2824f879c1..767efb2ed3 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -5898,6 +5898,59 @@ ovs-appctl dpif/dump-flows br0 > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +# OFPROTO_CLEAR_DURATION([]) > +# > +# Clear the duration from the piped input which would differ from test to test > +# > +m4_define([OFPROTO_CLEAR_DURATION], [sed -e 's/duration=.*s,/duration=<cleared>,/g']) > + > +# OFPROTO_CLEAR_DURATION_PKTS([]) > +# > +# Clear the duration from the piped input which would differ from test to test > +# > +m4_define([OFPROTO_CLEAR_DURATION_PKTS], [sed -e 's/duration=.*s,/duration=<cleared>,/g' -e 's/n_packets=.*,/n_packets=<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(table=1) > +table=0,priority=0 action=drop > +table=1,priority=10 ct_state=+trk+new,ip,in_port=1 action=ct(commit,table=2) > +table=1,priority=10 in_port=2 action=1 > +table=2,priority=10 ct_state=+trk+new,in_port=1 action=2 > +table=2,priority=10 ct_state=+trk+est action=drop > +]) > + > +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) > + > +NS_EXEC([at_ns0], [ping 10.1.1.2 -c 1 -q]) > + > +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], > + [0], [dnl > + cookie=0x0, duration=<cleared>, table=2, n_packets=1, n_bytes=98, idle_age=0, priority=10,ct_state=+new+trk,in_port=1 actions=output:2 > + cookie=0x0, duration=<cleared>, table=2, n_packets=0, n_bytes=0, idle_age=0, priority=10,ct_state=+est+trk actions=drop > +]) > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > + >
Tonghao Zhang <xiangxia.m.yue@gmail.com> writes: > On Fri, Jan 15, 2021 at 5:20 AM Aaron Conole <aconole@redhat.com> wrote: >> >> Hi Tonghao Xhang, >> >> xiangxia.m.yue@gmail.com writes: >> >> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> >> > >> > 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 <diproiettod@vmware.com> >> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> >> >> 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 > 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 + +
Aaron Conole <aconole@redhat.com> writes: > Tonghao Zhang <xiangxia.m.yue@gmail.com> writes: > >> On Fri, Jan 15, 2021 at 5:20 AM Aaron Conole <aconole@redhat.com> wrote: >>> >>> Hi Tonghao Xhang, >>> >>> xiangxia.m.yue@gmail.com writes: >>> >>> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> >>> > >>> > 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 <diproiettod@vmware.com> >>> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> >>> >>> 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 > + > +
On Tue, Jan 19, 2021 at 6:45 AM Aaron Conole <aconole@redhat.com> wrote: > > The fixes tag for this should be: > > Fixes: a867c010ee91 ("conntrack: Fix conntrack new state") Yes, commit a867c010ee91 ("conntrack: Fix conntrack new state") is the right one to fix. Commit a867c010ee91 only takes care of TCP, UDP (conntrack-others) but does not address ICMP properly. I checked Tonghao's patch and it does address the ICMP case properly. > 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']]) Thanks Aaron for adding this new system traffic test. It does work on my machine for both kernel and userspace conntrack. One suggestion for the test is that usually we add the testing m4 marco for system traffic test on system-common-macros.at. The rest of the test case looks good to me. Thanks, -Yi-Hung
On Wed, Jan 20, 2021 at 4:17 AM Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > On Tue, Jan 19, 2021 at 6:45 AM Aaron Conole <aconole@redhat.com> wrote: > > > > The fixes tag for this should be: > > > > Fixes: a867c010ee91 ("conntrack: Fix conntrack new state") > > Yes, commit a867c010ee91 ("conntrack: Fix conntrack new state") is the > right one to fix. Commit a867c010ee91 only takes care of TCP, UDP > (conntrack-others) but does not address ICMP properly. > > I checked Tonghao's patch and it does address the ICMP case properly. > > > > 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']]) > > Thanks Aaron for adding this new system traffic test. It does work on > my machine for both kernel and userspace conntrack. > > One suggestion for the test is that usually we add the testing m4 > marco for system traffic test on system-common-macros.at. The rest of > the test case looks good to me. Thanks everyone. please review v2, thanks. http://patchwork.ozlabs.org/project/openvswitch/patch/20210121091247.53292-1-xiangxia.m.yue@gmail.com/ > Thanks, > > -Yi-Hung
diff --git a/lib/conntrack-icmp.c b/lib/conntrack-icmp.c index 9b7263011..7e24d90a5 100644 --- a/lib/conntrack-icmp.c +++ b/lib/conntrack-icmp.c @@ -59,13 +59,16 @@ icmp_conn_update(struct conntrack *ct, struct conn *conn_, struct dp_packet *pkt OVS_UNUSED, bool reply, long long now) { struct conn_icmp *conn = conn_icmp_cast(conn_); + enum ct_update_res ret = CT_UPDATE_VALID; if (reply && conn->state == ICMPS_FIRST) { conn->state = ICMPS_REPLY; + } else if (conn->state == ICMPS_FIRST) { + ret = CT_UPDATE_VALID_NEW; } conn_update_expiration(ct, &conn->up, icmp_timeouts[conn->state], now); - return CT_UPDATE_VALID; + return ret; } static bool