diff mbox series

[ovs-dev] conntrack: Fix conn_type need be checked when remove rev_conn.

Message ID 1505123379-12144-1-git-send-email-wangyunjian@huawei.com
State Changes Requested
Headers show
Series [ovs-dev] conntrack: Fix conn_type need be checked when remove rev_conn. | expand

Commit Message

wangyunjian Sept. 11, 2017, 9:49 a.m. UTC 
From: Yunjian Wang <wangyunjian@huawei.com>

The rev_conn need will be removed, only when conn_type is CT_CONN_TYPE_UN_NAT.
This crash will be triggered when remove conn in ct-clean thread.

Reported-by: Lili Huang <huanglili.huang@huawei.com>
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 lib/conntrack.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Darrell Ball Sept. 12, 2017, 4:37 a.m. UTC | #1 
We cannot merge this patch.

Can you provide answers to the questions I asked here

https://mail.openvswitch.org/pipermail/ovs-discuss/2017-September/045308.html

Thanks Darrell


On Mon, Sep 11, 2017 at 2:49 AM, w00273186 <wangyunjian@huawei.com> wrote:

> From: Yunjian Wang <wangyunjian@huawei.com>
>
> The rev_conn need will be removed, only when conn_type is
> CT_CONN_TYPE_UN_NAT.
> This crash will be triggered when remove conn in ct-clean thread.
>
> Reported-by: Lili Huang <huanglili.huang@huawei.com>
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
>  lib/conntrack.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 419cb1d..c1adb56 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -684,9 +684,10 @@ nat_clean(struct conntrack *ct, struct conn *conn,
>
>      /* In the unlikely event, rev conn was recreated, then skip
>       * rev_conn cleanup. */
> -    if (rev_conn && (!nat_conn_key_node ||
> -                     conn_key_cmp(&nat_conn_key_node->value,
> -                                  &rev_conn->rev_key))) {
> +    if (rev_conn &&
> +        (rev_conn->conn_type == CT_CONN_TYPE_UN_NAT) &&
> +        (!nat_conn_key_node || conn_key_cmp(&nat_conn_key_node->value,
> +                                            &rev_conn->rev_key))) {
>          hmap_remove(&ct->buckets[bucket_rev_conn].connections,
>                      &rev_conn->node);
>          free(rev_conn);
> --
> 1.8.3.1
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
wangyunjian Sept. 23, 2017, 9:35 a.m. UTC | #2 
We use hping3 to send random tcp, udp from VM1 to VM2. We met a issue that conn_lookup( ) will find
rev_conn->conn_type = CT_CONN_TYPE_DEFAULT in nat_clean().

Is this rev_conn->conn_type = CT_CONN_TYPE_DEFAULT ok?

I think the rev_conn->conn_type need to be CT_CONN_TYPE_UN_NAT.

From: Darrell Ball [mailto:dlu998@gmail.com]
Sent: Tuesday, September 12, 2017 12:38 PM
To: wangyunjian <wangyunjian@huawei.com>
Cc: ovs dev <dev@openvswitch.org>; Huanglili (lee) <huanglili.huang@huawei.com>; blp@ovs.org
Subject: Re: [ovs-dev] [PATCH] conntrack: Fix conn_type need be checked when remove rev_conn.

We cannot merge this patch.

Can you provide answers to the questions I asked here

https://mail.openvswitch.org/pipermail/ovs-discuss/2017-September/045308.html

Thanks Darrell


On Mon, Sep 11, 2017 at 2:49 AM, w00273186 <wangyunjian@huawei.com<mailto:wangyunjian@huawei.com>> wrote:
From: Yunjian Wang <wangyunjian@huawei.com<mailto:wangyunjian@huawei.com>>

The rev_conn need will be removed, only when conn_type is CT_CONN_TYPE_UN_NAT.
This crash will be triggered when remove conn in ct-clean thread.

Reported-by: Lili Huang <huanglili.huang@huawei.com<mailto:huanglili.huang@huawei.com>>
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com<mailto:wangyunjian@huawei.com>>
---
 lib/conntrack.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 419cb1d..c1adb56 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -684,9 +684,10 @@ nat_clean(struct conntrack *ct, struct conn *conn,

     /* In the unlikely event, rev conn was recreated, then skip
      * rev_conn cleanup. */
-    if (rev_conn && (!nat_conn_key_node ||
-                     conn_key_cmp(&nat_conn_key_node->value,
-                                  &rev_conn->rev_key))) {
+    if (rev_conn &&
+        (rev_conn->conn_type == CT_CONN_TYPE_UN_NAT) &&
+        (!nat_conn_key_node || conn_key_cmp(&nat_conn_key_node->value,
+                                            &rev_conn->rev_key))) {
         hmap_remove(&ct->buckets[bucket_rev_conn].connections,
                     &rev_conn->node);
         free(rev_conn);
--
1.8.3.1
Darrell Ball Sept. 27, 2017, 1:02 a.m. UTC | #3 
On 9/23/17, 2:36 AM, "ovs-dev-bounces@openvswitch.org on behalf of wangyunjian" <ovs-dev-bounces@openvswitch.org on behalf of wangyunjian@huawei.com> wrote:

    We use hping3 to send random tcp, udp from VM1 to VM2. We met a issue that conn_lookup( ) will find
    rev_conn->conn_type = CT_CONN_TYPE_DEFAULT in nat_clean().

[Darrell] So you are sending traffic in one direction between 2 OVS ports ?
               What is the associated packet src/dst IP and src/dst ports, conn key src/dst addresses, src/dst ports
               and reverse conn key src/dst addresses, src/dst ports causing the problem?
               Is it the TCP or UDP traffic related ?
               
    Is this rev_conn->conn_type = CT_CONN_TYPE_DEFAULT ok?

[Darrell] I asked for information related to such a possible case, but no details were provided yet – see link below.
               Furthermore, the other information provided was not consistent, as mentioned earlier.
               The data would need to be from a non-instrumented version of the code.
               
    I think the rev_conn->conn_type need to be CT_CONN_TYPE_UN_NAT.


    From: Darrell Ball [mailto:dlu998@gmail.com]

    Sent: Tuesday, September 12, 2017 12:38 PM
    To: wangyunjian <wangyunjian@huawei.com>
    Cc: ovs dev <dev@openvswitch.org>; Huanglili (lee) <huanglili.huang@huawei.com>; blp@ovs.org
    Subject: Re: [ovs-dev] [PATCH] conntrack: Fix conn_type need be checked when remove rev_conn.
    
    We cannot merge this patch.
    
    Can you provide answers to the questions I asked here
    
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddiscuss_2017-2DSeptember_045308.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=lN8LgYzgffaggReqxu_7a9QKH74RKoTbK0Faru94z7o&s=l8K4j912IB0MotzNP8RaUgLFTrO4BI9QTggLebSzhoY&e= 
    
    Thanks Darrell
    
    
    On Mon, Sep 11, 2017 at 2:49 AM, w00273186 <wangyunjian@huawei.com<mailto:wangyunjian@huawei.com>> wrote:
    From: Yunjian Wang <wangyunjian@huawei.com<mailto:wangyunjian@huawei.com>>

    
    The rev_conn need will be removed, only when conn_type is CT_CONN_TYPE_UN_NAT.
    This crash will be triggered when remove conn in ct-clean thread.
    
    Reported-by: Lili Huang <huanglili.huang@huawei.com<mailto:huanglili.huang@huawei.com>>
    Signed-off-by: Yunjian Wang <wangyunjian@huawei.com<mailto:wangyunjian@huawei.com>>

    ---
     lib/conntrack.c | 7 ++++---
     1 file changed, 4 insertions(+), 3 deletions(-)
    
    diff --git a/lib/conntrack.c b/lib/conntrack.c
    index 419cb1d..c1adb56 100644
    --- a/lib/conntrack.c
    +++ b/lib/conntrack.c
    @@ -684,9 +684,10 @@ nat_clean(struct conntrack *ct, struct conn *conn,
    
         /* In the unlikely event, rev conn was recreated, then skip
          * rev_conn cleanup. */
    -    if (rev_conn && (!nat_conn_key_node ||
    -                     conn_key_cmp(&nat_conn_key_node->value,
    -                                  &rev_conn->rev_key))) {
    +    if (rev_conn &&
    +        (rev_conn->conn_type == CT_CONN_TYPE_UN_NAT) &&
    +        (!nat_conn_key_node || conn_key_cmp(&nat_conn_key_node->value,
    +                                            &rev_conn->rev_key))) {
             hmap_remove(&ct->buckets[bucket_rev_conn].connections,
                         &rev_conn->node);
             free(rev_conn);
    --
    1.8.3.1
    
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org<mailto:dev@openvswitch.org>
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=lN8LgYzgffaggReqxu_7a9QKH74RKoTbK0Faru94z7o&s=z8oNo064OLtfO_bcwjyMAq0xXydcu0oWif5YkLENYtw&e= 
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=lN8LgYzgffaggReqxu_7a9QKH74RKoTbK0Faru94z7o&s=z8oNo064OLtfO_bcwjyMAq0xXydcu0oWif5YkLENYtw&e=
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 419cb1d..c1adb56 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -684,9 +684,10 @@  nat_clean(struct conntrack *ct, struct conn *conn,
 
     /* In the unlikely event, rev conn was recreated, then skip
      * rev_conn cleanup. */
-    if (rev_conn && (!nat_conn_key_node ||
-                     conn_key_cmp(&nat_conn_key_node->value,
-                                  &rev_conn->rev_key))) {
+    if (rev_conn &&
+        (rev_conn->conn_type == CT_CONN_TYPE_UN_NAT) &&
+        (!nat_conn_key_node || conn_key_cmp(&nat_conn_key_node->value,
+                                            &rev_conn->rev_key))) {
         hmap_remove(&ct->buckets[bucket_rev_conn].connections,
                     &rev_conn->node);
         free(rev_conn);