diff mbox series

[ovs-dev,ovs,v2] conntrack: Fix the icmp conntrack new state.

Message ID 20210121091247.53292-1-xiangxia.m.yue@gmail.com
State Accepted
Headers show
Series [ovs-dev,ovs,v2] conntrack: Fix the icmp conntrack new state. | expand

Commit Message

Tonghao Zhang Jan. 21, 2021, 9:12 a.m. UTC 
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 state to CS_ESTABLISHED before receiving reply or related packets.

Fixes: a867c010ee91 ("conntrack: Fix conntrack new state")
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
v2:
1. add test case
2. change the commit message, and title
3. change the fix tag
---
 lib/conntrack-icmp.c          |  5 +++-
 tests/system-common-macros.at |  6 +++++
 tests/system-traffic.at       | 44 +++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletion(-)

Comments

Yi-Hung Wei Jan. 21, 2021, 6:26 p.m. UTC | #1 
On Thu, Jan 21, 2021 at 1:15 AM <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 state to CS_ESTABLISHED before receiving reply or related packets.
>
> Fixes: a867c010ee91 ("conntrack: Fix conntrack new state")
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> v2:
> 1. add test case
> 2. change the commit message, and title
> 3. change the fix tag
> ---

Thanks for v2. LGTM.

Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>
Aaron Conole Jan. 22, 2021, 9:02 p.m. UTC | #2 
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 state to CS_ESTABLISHED before receiving reply or related packets.
>
> Fixes: a867c010ee91 ("conntrack: Fix conntrack new state")
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---

Thanks for the v2.

Acked-by: Aaron Conole <aconole@redhat.com>
Ilya Maximets Jan. 27, 2021, 4:58 p.m. UTC | #3 
On 1/21/21 10:12 AM, 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 state to CS_ESTABLISHED before receiving reply or related packets.
> 
> Fixes: a867c010ee91 ("conntrack: Fix conntrack new state")
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> v2:
> 1. add test case
> 2. change the commit message, and title
> 3. change the fix tag

Thanks, Tonghao, Aaron and Yi-Hung!
Applied to master and backported down to 2.13.

I had to also backport commit
d93c3111ccbf ("conntrack: Fix icmp conntrack state.") from master
since this one were missing on 2.13.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/conntrack-icmp.c b/lib/conntrack-icmp.c
index bf49f9a9fa93..b4029703988d 100644
--- a/lib/conntrack-icmp.c
+++ b/lib/conntrack-icmp.c
@@ -51,13 +51,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
diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 68c8774d1ac6..9d5e24a2922b 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -275,6 +275,12 @@  m4_define([OVS_START_L7],
    ]
 )
 
+# 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']])
+
 # OVS_CHECK_VXLAN()
 #
 # Do basic check for vxlan functionality, skip the test if it's not there.
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index d2a4dbffecbe..fb5b9a36d283 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -5927,6 +5927,50 @@  ovs-appctl dpif/dump-flows br0
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+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  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=10  ct_state=+est-new+trk,ct_zone=1,in_port=1 action=resubmit(,2)
+table=1,priority=0   action=drop
+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
+
 AT_BANNER([802.1ad])
 
 AT_SETUP([802.1ad - vlan_limit])