[ovs-dev] lib/conntrack: remove unnecessary addr check for ICMP.

Message ID 1512242752-73246-1-git-send-email-wangzhike@jd.com
State New
Headers show
Series
  • [ovs-dev] lib/conntrack: remove unnecessary addr check for ICMP.
Related show

Commit Message

王志克 Dec. 2, 2017, 7:25 p.m.
From: wangzhike <wangzhike@jd.com>

ICMP response (Unreachable/fragmentationRequired/...) may be created
at devices in the middle, and such packets are tagged as invalid in
user space conntrack. In fact it does not make sense to validate the
src and dest address.

Signed-off-by: wang zhike <wangzhike@jd.com>
---
 lib/conntrack.c | 13 -------------
 1 file changed, 13 deletions(-)

Comments

Darrell Ball Dec. 6, 2017, 7:22 p.m. | #1
Thanks for looking at this.

In the commit message, can you delineate.

1/ The forward direction packet in terms of src ip, dest ip, L4 attributes
2/ The reverse direction error packet in terms of src ip, dest ip, icmp error payload

Darrell

On 12/4/17, 10:22 PM, "ovs-dev-bounces@openvswitch.org on behalf of wang zhike" <ovs-dev-bounces@openvswitch.org on behalf of wangzhike@jd.com> wrote:

    From: wangzhike <wangzhike@jd.com>
    
    ICMP response (Unreachable/fragmentationRequired/...) may be created
    at devices in the middle, and such packets are tagged as invalid in
    user space conntrack. In fact it does not make sense to validate the
    src and dest address.
    
    Signed-off-by: wang zhike <wangzhike@jd.com>
    ---
     lib/conntrack.c | 13 -------------
     1 file changed, 13 deletions(-)
    
    diff --git a/lib/conntrack.c b/lib/conntrack.c
    index f5a3aa9..c44ad0f 100644
    --- a/lib/conntrack.c
    +++ b/lib/conntrack.c
    @@ -1702,11 +1702,6 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
                 return false;
             }
     
    -        if (inner_key.src.addr.ipv4_aligned != key->dst.addr.ipv4_aligned
    -            || inner_key.dst.addr.ipv4_aligned != key->src.addr.ipv4_aligned) {
    -            return false;
    -        }
    -
             key->src = inner_key.src;
             key->dst = inner_key.dst;
             key->nw_proto = inner_key.nw_proto;
    @@ -1789,14 +1784,6 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
                 return false;
             }
     
    -        /* pf doesn't do this, but it seems a good idea */
    -        if (!ipv6_addr_equals(&inner_key.src.addr.ipv6_aligned,
    -                              &key->dst.addr.ipv6_aligned)
    -            || !ipv6_addr_equals(&inner_key.dst.addr.ipv6_aligned,
    -                                 &key->src.addr.ipv6_aligned)) {
    -            return false;
    -        }
    -
             key->src = inner_key.src;
             key->dst = inner_key.dst;
             key->nw_proto = inner_key.nw_proto;
    -- 
    1.8.3.1
    
    _______________________________________________
    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=IqOQ4UcLnOpQUkvP7FHVH6IWeAK_4DDBRBqX2w3wl94&s=Xmvgnl8plChpjcr77sLIOxE0krKVuNVqvS3_eOoNeMg&e=
Darrell Ball Dec. 7, 2017, 2:13 a.m. | #2
Hi Wang

To speed up the process, I sent an alternative patch here:

https://patchwork.ozlabs.org/patch/845407/

I agree the address sanity check is not correct but I think it should be partially retained
rather than removed. I also think a test was needed.

Pls let me know if it makes sense.
Also. if you prefer, I can make you the author.

Thanks Darrell




On 12/6/17, 11:22 AM, "Darrell Ball" <dball@vmware.com> wrote:

    Thanks for looking at this.
    
    In the commit message, can you delineate.
    
    1/ The forward direction packet in terms of src ip, dest ip, L4 attributes
    2/ The reverse direction error packet in terms of src ip, dest ip, icmp error payload
    
    Darrell
    
    On 12/4/17, 10:22 PM, "ovs-dev-bounces@openvswitch.org on behalf of wang zhike" <ovs-dev-bounces@openvswitch.org on behalf of wangzhike@jd.com> wrote:
    
        From: wangzhike <wangzhike@jd.com>
        
        ICMP response (Unreachable/fragmentationRequired/...) may be created
        at devices in the middle, and such packets are tagged as invalid in
        user space conntrack. In fact it does not make sense to validate the
        src and dest address.
        
        Signed-off-by: wang zhike <wangzhike@jd.com>
        ---
         lib/conntrack.c | 13 -------------
         1 file changed, 13 deletions(-)
        
        diff --git a/lib/conntrack.c b/lib/conntrack.c
        index f5a3aa9..c44ad0f 100644
        --- a/lib/conntrack.c
        +++ b/lib/conntrack.c
        @@ -1702,11 +1702,6 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
                     return false;
                 }
         
        -        if (inner_key.src.addr.ipv4_aligned != key->dst.addr.ipv4_aligned
        -            || inner_key.dst.addr.ipv4_aligned != key->src.addr.ipv4_aligned) {
        -            return false;
        -        }
        -
                 key->src = inner_key.src;
                 key->dst = inner_key.dst;
                 key->nw_proto = inner_key.nw_proto;
        @@ -1789,14 +1784,6 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
                     return false;
                 }
         
        -        /* pf doesn't do this, but it seems a good idea */
        -        if (!ipv6_addr_equals(&inner_key.src.addr.ipv6_aligned,
        -                              &key->dst.addr.ipv6_aligned)
        -            || !ipv6_addr_equals(&inner_key.dst.addr.ipv6_aligned,
        -                                 &key->src.addr.ipv6_aligned)) {
        -            return false;
        -        }
        -
                 key->src = inner_key.src;
                 key->dst = inner_key.dst;
                 key->nw_proto = inner_key.nw_proto;
        -- 
        1.8.3.1
        
        _______________________________________________
        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=IqOQ4UcLnOpQUkvP7FHVH6IWeAK_4DDBRBqX2w3wl94&s=Xmvgnl8plChpjcr77sLIOxE0krKVuNVqvS3_eOoNeMg&e=
王志克 Dec. 7, 2017, 2:48 a.m. | #3
Hi Darrell,

In fact I indeed considerred whether to keep partial check as your patch. My idea is that it is better to do as little work as possible. So in my opnion such validation (and even checksum validation and so on) is not necessary at all (OVS is middle device, and such error validation can be done at end host stack).

I am open to the decision. So if you think your patch is more suitable, I can be the co-author.

Br,
Wang Zhike


-----Original Message-----
From: Darrell Ball [mailto:dball@vmware.com] 

Sent: Thursday, December 07, 2017 10:14 AM
To: 王志克; dev@openvswitch.org; Daniele Di Proietto
Subject: Re: [ovs-dev] [PATCH] lib/conntrack: remove unnecessary addr check for ICMP.

Hi Wang

To speed up the process, I sent an alternative patch here:

https://patchwork.ozlabs.org/patch/845407/

I agree the address sanity check is not correct but I think it should be partially retained
rather than removed. I also think a test was needed.

Pls let me know if it makes sense.
Also. if you prefer, I can make you the author.

Thanks Darrell




On 12/6/17, 11:22 AM, "Darrell Ball" <dball@vmware.com> wrote:

    Thanks for looking at this.
    
    In the commit message, can you delineate.
    
    1/ The forward direction packet in terms of src ip, dest ip, L4 attributes
    2/ The reverse direction error packet in terms of src ip, dest ip, icmp error payload
    
    Darrell
    
    On 12/4/17, 10:22 PM, "ovs-dev-bounces@openvswitch.org on behalf of wang zhike" <ovs-dev-bounces@openvswitch.org on behalf of wangzhike@jd.com> wrote:
    
        From: wangzhike <wangzhike@jd.com>

        
        ICMP response (Unreachable/fragmentationRequired/...) may be created
        at devices in the middle, and such packets are tagged as invalid in
        user space conntrack. In fact it does not make sense to validate the
        src and dest address.
        
        Signed-off-by: wang zhike <wangzhike@jd.com>

        ---
         lib/conntrack.c | 13 -------------
         1 file changed, 13 deletions(-)
        
        diff --git a/lib/conntrack.c b/lib/conntrack.c
        index f5a3aa9..c44ad0f 100644
        --- a/lib/conntrack.c
        +++ b/lib/conntrack.c
        @@ -1702,11 +1702,6 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
                     return false;
                 }
         
        -        if (inner_key.src.addr.ipv4_aligned != key->dst.addr.ipv4_aligned
        -            || inner_key.dst.addr.ipv4_aligned != key->src.addr.ipv4_aligned) {
        -            return false;
        -        }
        -
                 key->src = inner_key.src;
                 key->dst = inner_key.dst;
                 key->nw_proto = inner_key.nw_proto;
        @@ -1789,14 +1784,6 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
                     return false;
                 }
         
        -        /* pf doesn't do this, but it seems a good idea */
        -        if (!ipv6_addr_equals(&inner_key.src.addr.ipv6_aligned,
        -                              &key->dst.addr.ipv6_aligned)
        -            || !ipv6_addr_equals(&inner_key.dst.addr.ipv6_aligned,
        -                                 &key->src.addr.ipv6_aligned)) {
        -            return false;
        -        }
        -
                 key->src = inner_key.src;
                 key->dst = inner_key.dst;
                 key->nw_proto = inner_key.nw_proto;
        -- 
        1.8.3.1
        
        _______________________________________________
        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=IqOQ4UcLnOpQUkvP7FHVH6IWeAK_4DDBRBqX2w3wl94&s=Xmvgnl8plChpjcr77sLIOxE0krKVuNVqvS3_eOoNeMg&e=
Darrell Ball Dec. 7, 2017, 3:14 a.m. | #4
On 12/6/17, 6:49 PM, "王志克" <wangzhike@jd.com> wrote:

    Hi Darrell,
    
    In fact I indeed considerred whether to keep partial check as your patch. My idea is that it is better to do as little work as possible. So in my opnion such validation (and even checksum validation and so on) is not necessary at all (OVS is middle device, and such error validation can be done at end host stack).


[Darrell] The validations are necessary; under DOS, it is better to stop the attack as early as possible.
                Also, making assumptions about what the ‘target’ host stack will or can do is “making assumptions”. 
    
    I am open to the decision. So if you think your patch is more suitable, I can be the co-author.

[Darrell] I will keep you as co-author then.
    
    Br,
    Wang Zhike
    
    
    -----Original Message-----
    From: Darrell Ball [mailto:dball@vmware.com] 

    Sent: Thursday, December 07, 2017 10:14 AM
    To: 王志克; dev@openvswitch.org; Daniele Di Proietto
    Subject: Re: [ovs-dev] [PATCH] lib/conntrack: remove unnecessary addr check for ICMP.
    
    Hi Wang
    
    To speed up the process, I sent an alternative patch here:
    
    https://patchwork.ozlabs.org/patch/845407/
    
    I agree the address sanity check is not correct but I think it should be partially retained
    rather than removed. I also think a test was needed.
    
    Pls let me know if it makes sense.
    Also. if you prefer, I can make you the author.
    
    Thanks Darrell
    
    
    
    
    On 12/6/17, 11:22 AM, "Darrell Ball" <dball@vmware.com> wrote:
    
        Thanks for looking at this.
        
        In the commit message, can you delineate.
        
        1/ The forward direction packet in terms of src ip, dest ip, L4 attributes
        2/ The reverse direction error packet in terms of src ip, dest ip, icmp error payload
        
        Darrell
        
        On 12/4/17, 10:22 PM, "ovs-dev-bounces@openvswitch.org on behalf of wang zhike" <ovs-dev-bounces@openvswitch.org on behalf of wangzhike@jd.com> wrote:
        
            From: wangzhike <wangzhike@jd.com>

            
            ICMP response (Unreachable/fragmentationRequired/...) may be created
            at devices in the middle, and such packets are tagged as invalid in
            user space conntrack. In fact it does not make sense to validate the
            src and dest address.
            
            Signed-off-by: wang zhike <wangzhike@jd.com>

            ---
             lib/conntrack.c | 13 -------------
             1 file changed, 13 deletions(-)
            
            diff --git a/lib/conntrack.c b/lib/conntrack.c
            index f5a3aa9..c44ad0f 100644
            --- a/lib/conntrack.c
            +++ b/lib/conntrack.c
            @@ -1702,11 +1702,6 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
                         return false;
                     }
             
            -        if (inner_key.src.addr.ipv4_aligned != key->dst.addr.ipv4_aligned
            -            || inner_key.dst.addr.ipv4_aligned != key->src.addr.ipv4_aligned) {
            -            return false;
            -        }
            -
                     key->src = inner_key.src;
                     key->dst = inner_key.dst;
                     key->nw_proto = inner_key.nw_proto;
            @@ -1789,14 +1784,6 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
                         return false;
                     }
             
            -        /* pf doesn't do this, but it seems a good idea */
            -        if (!ipv6_addr_equals(&inner_key.src.addr.ipv6_aligned,
            -                              &key->dst.addr.ipv6_aligned)
            -            || !ipv6_addr_equals(&inner_key.dst.addr.ipv6_aligned,
            -                                 &key->src.addr.ipv6_aligned)) {
            -            return false;
            -        }
            -
                     key->src = inner_key.src;
                     key->dst = inner_key.dst;
                     key->nw_proto = inner_key.nw_proto;
            -- 
            1.8.3.1
            
            _______________________________________________
            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=IqOQ4UcLnOpQUkvP7FHVH6IWeAK_4DDBRBqX2w3wl94&s=Xmvgnl8plChpjcr77sLIOxE0krKVuNVqvS3_eOoNeMg&e=

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index f5a3aa9..c44ad0f 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1702,11 +1702,6 @@  extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
             return false;
         }
 
-        if (inner_key.src.addr.ipv4_aligned != key->dst.addr.ipv4_aligned
-            || inner_key.dst.addr.ipv4_aligned != key->src.addr.ipv4_aligned) {
-            return false;
-        }
-
         key->src = inner_key.src;
         key->dst = inner_key.dst;
         key->nw_proto = inner_key.nw_proto;
@@ -1789,14 +1784,6 @@  extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
             return false;
         }
 
-        /* pf doesn't do this, but it seems a good idea */
-        if (!ipv6_addr_equals(&inner_key.src.addr.ipv6_aligned,
-                              &key->dst.addr.ipv6_aligned)
-            || !ipv6_addr_equals(&inner_key.dst.addr.ipv6_aligned,
-                                 &key->src.addr.ipv6_aligned)) {
-            return false;
-        }
-
         key->src = inner_key.src;
         key->dst = inner_key.dst;
         key->nw_proto = inner_key.nw_proto;