[ovs-dev] [PATCH v2 2/3] conntrack: Return NEW for IPv6 ND packets without tracking.

Darrell Ball dball at vmware.com
Fri Dec 23 05:20:50 UTC 2016


Some comments inline

On 12/22/16, 6:36 PM, "Daniele Di Proietto" <diproiettod at vmware.com> wrote:

    The userspace connection tracker treats Neighbor Discovery packets
    as invalid, because they're not checked against any connection.
    
    This in inconsistent with the kernel connection tracker which always
    returns 'CS_NEW'.
    
    Therefore, this commit makes the userspace connection tracker conforming
    with the kernel.  ND packets still do not create or read any state, but
    they're treated as NEW.
    
    To support this, the key extraction functions can now return
    KEY_NO_TRACK, meaning that the packet is ok, but it should be treated
    statelessly.


s/    To support this, the key extraction functions can now return
    KEY_NO_TRACK, meaning that the packet is ok, but it should be treated
    statelessly.
/
    To support this, the key extraction functions can now return
    KEY_NO_TRACK, meaning the packet should be treated statelessly
    and not be sent to the connection tracker.
/


    We also have to remove a test that explicitly checked that neighbor
    discovery was treated as invalid.
    
    Reported-by: Sridhar Gaddam <sgaddam at redhat.com>
    Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
    ---
    v2: Update comment to reflect that we do not do special validation with
        the packet.
    ---
     lib/conntrack.c         | 134 ++++++++++++++++++++++++++++++++----------------
     tests/ofproto-dpif.at   |  32 ------------
     tests/system-traffic.at |  35 +++++++++++++
     3 files changed, 125 insertions(+), 76 deletions(-)
    
    diff --git a/lib/conntrack.c b/lib/conntrack.c
    index 9bea3d9..86228d6 100644
    --- a/lib/conntrack.c
    +++ b/lib/conntrack.c
    @@ -52,9 +52,17 @@ struct conn_lookup_ctx {
         bool related;
     };
     
    -static bool conn_key_extract(struct conntrack *, struct dp_packet *,
    -                             ovs_be16 dl_type, struct conn_lookup_ctx *,
    -                             uint16_t zone);
    +enum key_status {
    +    KEY_INVALID,   /* Could not extract the connection key: invalid. */
    +    KEY_OK,        /* Connection key is ok. */
    +    KEY_NO_TRACK,  /* Connection key is ok, but it should not be tracked. */


KEY_NO_TRACK,  /* Connection key should not be tracked. */


    +};
    +
    +static enum key_status conn_key_extract(struct conntrack *,
    +                                        struct dp_packet *,
    +                                        ovs_be16 dl_type,
    +                                        struct conn_lookup_ctx *,
    +                                        uint16_t zone);
     static uint32_t conn_key_hash(const struct conn_key *, uint32_t basis);
     static void conn_key_reverse(struct conn_key *);
     static void conn_key_lookup(struct conntrack_bucket *ctb,
    @@ -157,6 +165,20 @@ static unsigned hash_to_bucket(uint32_t hash)
         return (hash >> (32 - CONNTRACK_BUCKETS_SHIFT)) % CONNTRACK_BUCKETS;
     }
     
    +static uint16_t
    +key_status_to_cs(enum key_status s)
    +{
    +    switch (s) {
    +    case KEY_INVALID:
    +        return CS_INVALID;
    +    case KEY_OK:
    +    case KEY_NO_TRACK:
    +        return CS_NEW;
    +    default:
    +        OVS_NOT_REACHED();
    +    }
    +}
    +
     static void
     write_ct_md(struct dp_packet *pkt, uint16_t state, uint16_t zone,
                 uint32_t mark, ovs_u128 label)
    @@ -303,10 +325,13 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
     
         memset(bucket_list, INT8_C(-1), sizeof bucket_list);
         for (i = 0; i < cnt; i++) {
    +        enum key_status extract_res;
             unsigned bucket;
     
    -        if (!conn_key_extract(ct, pkts[i], dl_type, &ctxs[i], zone)) {
    -            write_ct_md(pkts[i], CS_INVALID, zone, 0, OVS_U128_ZERO);
    +        extract_res = conn_key_extract(ct, pkts[i], dl_type, &ctxs[i], zone);
    +        if (extract_res != KEY_OK) {
    +            write_ct_md(pkts[i], key_status_to_cs(extract_res), zone, 0,
    +                        OVS_U128_ZERO);
                 continue;
             }
     
    @@ -693,8 +718,11 @@ extract_l4_udp(struct conn_key *key, const void *data, size_t size)
         return key->src.port && key->dst.port;
     }
     
    -static inline bool extract_l4(struct conn_key *key, const void *data,
    -                              size_t size, bool *related, const void *l3);
    +static inline enum key_status extract_l4(struct conn_key *key,
    +                                         const void *data,
    +                                         size_t size,
    +                                         bool *related,
    +                                         const void *l3);
     
     static uint8_t
     reverse_icmp_type(uint8_t type)
    @@ -724,14 +752,14 @@ reverse_icmp_type(uint8_t type)
      * instead and set *related to true.  If 'related' is NULL we're
      * already processing a nested header and no such recursion is
      * possible */
    -static inline int
    +static inline enum key_status
     extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
                     bool *related)
     {
         const struct icmp_header *icmp = data;
     
         if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) {
    -        return false;
    +        return KEY_INVALID;
         }
     
         switch (icmp->icmp_type) {
    @@ -742,13 +770,15 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
         case ICMP4_INFOREQUEST:
         case ICMP4_INFOREPLY:
             if (icmp->icmp_code != 0) {
    -            return false;
    +            return KEY_INVALID;
             }
             /* Separate ICMP connection: identified using id */
             key->src.icmp_id = key->dst.icmp_id = icmp->icmp_fields.echo.id;
             key->src.icmp_type = icmp->icmp_type;
             key->dst.icmp_type = reverse_icmp_type(icmp->icmp_type);
    -        break;
    +
    +        return KEY_OK;
    +
         case ICMP4_DST_UNREACH:
         case ICMP4_TIME_EXCEEDED:
         case ICMP4_PARAM_PROB:
    @@ -760,41 +790,40 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
             const char *l3 = (const char *) (icmp + 1);
             const char *tail = (const char *) data + size;
             const char *l4;
    +        enum key_status res;
             bool ok;
     
             if (!related) {
    -            return false;
    +            return KEY_INVALID;
             }
     
             memset(&inner_key, 0, sizeof inner_key);
             inner_key.dl_type = htons(ETH_TYPE_IP);
             ok = extract_l3_ipv4(&inner_key, l3, tail - l3, &l4, false);
             if (!ok) {
    -            return false;
    +            return KEY_INVALID;
             }
     
             /* pf doesn't do this, but it seems a good idea */
             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;
    +            return KEY_INVALID;
             }
     
             key->src = inner_key.src;
             key->dst = inner_key.dst;
             key->nw_proto = inner_key.nw_proto;
     
    -        ok = extract_l4(key, l4, tail - l4, NULL, l3);
    -        if (ok) {
    +        res = extract_l4(key, l4, tail - l4, NULL, l3);
    +        if (res != KEY_INVALID) {
                 conn_key_reverse(key);
                 *related = true;
             }
    -        return ok;
    +        return res;
         }
         default:
    -        return false;
    +        return KEY_INVALID;
         }
    -
    -    return true;
     }
     
     static uint8_t
    @@ -815,7 +844,7 @@ reverse_icmp6_type(uint8_t type)
      * instead and set *related to true.  If 'related' is NULL we're
      * already processing a nested header and no such recursion is
      * possible */
    -static inline bool
    +static inline enum key_status
     extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
                      bool *related)
     {
    @@ -824,20 +853,22 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
         /* All the messages that we support need at least 4 bytes after
          * the header */
         if (size < sizeof *icmp6 + 4) {
    -        return false;
    +        return KEY_INVALID;
         }
     
         switch (icmp6->icmp6_type) {
         case ICMP6_ECHO_REQUEST:
         case ICMP6_ECHO_REPLY:
             if (icmp6->icmp6_code != 0) {
    -            return false;
    +            return KEY_INVALID;
             }
             /* Separate ICMP connection: identified using id */
             key->src.icmp_id = key->dst.icmp_id = *(ovs_be16 *) (icmp6 + 1);
             key->src.icmp_type = icmp6->icmp6_type;
             key->dst.icmp_type = reverse_icmp6_type(icmp6->icmp6_type);
    -        break;
    +
    +        return KEY_OK;
    +
         case ICMP6_DST_UNREACH:
         case ICMP6_PACKET_TOO_BIG:
         case ICMP6_TIME_EXCEEDED:
    @@ -848,17 +879,18 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
             const char *l3 = (const char *) icmp6 + 8;
             const char *tail = (const char *) data + size;
             const char *l4 = NULL;
    +        enum key_status res;
             bool ok;
     
             if (!related) {
    -            return false;
    +            return KEY_INVALID;
             }
     
             memset(&inner_key, 0, sizeof inner_key);
             inner_key.dl_type = htons(ETH_TYPE_IPV6);
             ok = extract_l3_ipv6(&inner_key, l3, tail - l3, &l4);
             if (!ok) {
    -            return false;
    +            return KEY_INVALID;
             }
     
             /* pf doesn't do this, but it seems a good idea */
    @@ -866,25 +898,32 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
                                   &key->dst.addr.ipv6_aligned)
                 || !ipv6_addr_equals(&inner_key.dst.addr.ipv6_aligned,
                                      &key->src.addr.ipv6_aligned)) {
    -            return false;
    +            return KEY_INVALID;
             }
     
             key->src = inner_key.src;
             key->dst = inner_key.dst;
             key->nw_proto = inner_key.nw_proto;
     
    -        ok = extract_l4(key, l4, tail - l4, NULL, l3);
    -        if (ok) {
    +        res = extract_l4(key, l4, tail - l4, NULL, l3);
    +        if (res != KEY_INVALID) {
                 conn_key_reverse(key);
                 *related = true;
             }
    -        return ok;
    +        return res;
         }
    +    case ND_NEIGHBOR_SOLICIT:
    +    case ND_NEIGHBOR_ADVERT:
    +    case ND_ROUTER_SOLICIT:
    +    case ND_ROUTER_ADVERT:

case ND_REDIRECT:

    +        if (icmp6->icmp6_code != 0) {
    +            return KEY_INVALID;
    +        }
    +        /* This packet is not going to be tracked. */
    +        return KEY_NO_TRACK;
         default:
    -        return false;
    +        return KEY_INVALID;
         }
    -
    -    return true;
     }
     
     /* Extract l4 fields into 'key', which must already contain valid l3
    @@ -896,30 +935,34 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
      *
      * If 'related' is NULL, it means that we're already parsing a header nested
      * in an ICMP error.  In this case, we skip checksum and length validation. */
    -static inline bool
    +static inline enum key_status
     extract_l4(struct conn_key *key, const void *data, size_t size, bool *related,
                const void *l3)
     {
         if (key->nw_proto == IPPROTO_TCP) {
             return (!related || check_l4_tcp(key, data, size, l3))
    -               && extract_l4_tcp(key, data, size);
    +               ? extract_l4_tcp(key, data, size)
    +               : KEY_INVALID;
         } else if (key->nw_proto == IPPROTO_UDP) {
             return (!related || check_l4_udp(key, data, size, l3))
    -               && extract_l4_udp(key, data, size);
    +               ? extract_l4_udp(key, data, size)
    +               : KEY_INVALID;
         } else if (key->dl_type == htons(ETH_TYPE_IP)
                    && key->nw_proto == IPPROTO_ICMP) {
             return (!related || check_l4_icmp(data, size))
    -               && extract_l4_icmp(key, data, size, related);
    +               ? extract_l4_icmp(key, data, size, related)
    +               : KEY_INVALID;
         } else if (key->dl_type == htons(ETH_TYPE_IPV6)
                    && key->nw_proto == IPPROTO_ICMPV6) {
             return (!related || check_l4_icmp6(key, data, size, l3))
    -               && extract_l4_icmp6(key, data, size, related);
    +               ? extract_l4_icmp6(key, data, size, related)
    +               : KEY_INVALID;
         } else {
    -        return false;
    +        return KEY_INVALID;
         }
     }
     
    -static bool
    +static enum key_status
     conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
                      struct conn_lookup_ctx *ctx, uint16_t zone)
     {
    @@ -932,7 +975,7 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
         memset(ctx, 0, sizeof *ctx);
     
         if (!l2 || !l3 || !l4) {
    -        return false;
    +        return KEY_INVALID;
         }
     
         ctx->key.zone = zone;
    @@ -979,13 +1022,16 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
         }
     
         if (ok) {
    -        if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3)) {
    +        enum key_status res;
    +
    +        res = extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3);
    +        if (res == KEY_OK) {
                 ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
    -            return true;
             }
    +        return res;
         }
     
    -    return false;
    +    return KEY_INVALID;
     }
     ?
     /* Symmetric */
    diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
    index 5f594be..5b70032 100644
    --- a/tests/ofproto-dpif.at
    +++ b/tests/ofproto-dpif.at
    @@ -8626,38 +8626,6 @@ OFPT_ECHO_REQUEST (xid=0x0): 0 bytes of payload
     OVS_VSWITCHD_STOP
     AT_CLEANUP
     
    -AT_SETUP([ofproto-dpif - conntrack - untrackable traffic])
    -OVS_VSWITCHD_START
    -
    -add_of_ports br0 1 2
    -
    -AT_CHECK([ovs-appctl vlog/set dpif_netdev:dbg vconn:info ofproto_dpif:info])
    -
    -AT_DATA([flows.txt], [dnl
    -ipv6,ct_state=-trk,action=ct(table=0,zone=0)
    -ct_state=+trk,action=controller
    -])
    -
    -AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
    -
    -AT_CAPTURE_FILE([ofctl_monitor.log])
    -AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log])
    -
    -AT_CHECK([ovs-appctl time/stop])
    -
    -AT_CHECK([ovs-appctl netdev-dummy/receive p2 '0060970769ea0000860580da86dd6000000000203afffe80000000000000020086fffe0580dafe80000000000000026097fffe0769ea870068bd00000000fe80000000000000026097fffe0769ea01010000860580da'])
    -
    -OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 1])
    -OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
    -
    -AT_CHECK([cat ofctl_monitor.log], [0], [dnl
    -NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=86 ct_state=inv|trk,in_port=2 (via action) data_len=86 (unbuffered)
    -icmp6,vlan_tci=0x0000,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src=fe80::200:86ff:fe05:80da,ipv6_dst=fe80::260:97ff:fe07:69ea,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=255,icmp_type=135,icmp_code=0,nd_target=fe80::260:97ff:fe07:69ea,nd_sll=00:00:86:05:80:da,nd_tll=00:00:00:00:00:00 icmp6_csum:68bd
    -])
    -
    -OVS_VSWITCHD_STOP
    -AT_CLEANUP
    -
     AT_SETUP([ofproto-dpif - conntrack - zones])
     OVS_VSWITCHD_START
     
    diff --git a/tests/system-traffic.at b/tests/system-traffic.at
    index a5023d3..7d86c81 100644
    --- a/tests/system-traffic.at
    +++ b/tests/system-traffic.at
    @@ -740,6 +740,41 @@ icmpv6,orig=(src=fc00::1,dst=fc00::2,id=<cleared>,type=128,code=0),reply=(src=fc
     OVS_TRAFFIC_VSWITCHD_STOP
     AT_CLEANUP
     
    +AT_SETUP([conntrack - neighbor discovery])
    +CHECK_CONNTRACK()
    +OVS_TRAFFIC_VSWITCHD_START()

Maybe create 2 ns and veths and then send packets b/w them
Maybe dump-flows to check the stats as well


    +
    +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
    +AT_DATA([flows.txt], [dnl
    +table=0,priority=1,action=drop
    +table=0,priority=100,ip6,action=ct(table=1)
    +dnl
    +table=1,priority=100,ip6,action=controller
    +])
    +
    +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
    +
    +AT_CAPTURE_FILE([ofctl_monitor.log])
    +AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir --pidfile 2> ofctl_monitor.log])
    +
    +dnl e6:6f:38:26:34:c4 > 33:33:ff:00:00:02, ethertype IPv6 (0x86dd), length 86: (hlim 255, next-header ICMPv6 (58) payload length: 32) fc00::1 > ff02::1:ff00:2: [icmp6 sum ok] ICMP6, neighbor solicitation, length 32, who has fc00::2
    +dnl     source link-address option (1), length 8 (1): e6:6f:38:26:34:c4
    +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 LOCAL 'resubmit(,0)' '3333ff000002e66f382634c486dd6000000000203afffc000000000000000000000000000001ff0200000000000000000001ff00000287002e3e00000000fc0000000000000000000000000000020101e66f382634c4'])
    +
    +dnl 56:5b:b3:68:8f:09 > e6:6f:38:26:34:c4, ethertype IPv6 (0x86dd), length 86: (hlim 255, next-header ICMPv6 (58) payload length: 32) fc00::2 > fc00::1: [icmp6 sum ok] ICMP6, neighbor advertisement, length 32, tgt is fc00::2, Flags [solicited, override]
    +dnl     destination link-address option (2), length 8 (1): 56:5b:b3:68:8f:09
    +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 LOCAL 'resubmit(,0)' 'e66f382634c4565bb3688f0986dd6000000000203afffc000000000000000000000000000002fc000000000000000000000000000001880088ce60000000fc0000000000000000000000000000020201565bb3688f09'])
    +
    +AT_CHECK([cat ofctl_monitor.log], [0], [dnl
    +NXT_PACKET_IN2 (xid=0x0): table_id=1 cookie=0x0 total_len=86 ct_state=new|trk,in_port=LOCAL (via action)
     data_len=86 (unbuffered)

+icmp6,vlan_tci=0x0000,dl_src=e6:6f:38:26:34:c4,dl_dst=33:33:ff:00:00:02,ipv6_src=fc00::1,ipv6_dst=ff02::1:ff00:2,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=255,icmp_type=135,icmp_code=0,nd_target=fc00::2,nd_sll=e6:6f:38:26:34:c4,nd_tll=00:00:00:00:00:00 icmp6_csum:2e3e
    +NXT_PACKET_IN2 (xid=0x0): table_id=1 cookie=0x0 total_len=86 ct_state=new|trk,in_port=LOCAL (via action) data_len=86 (unbuffered)
+icmp6,vlan_tci=0x0000,dl_src=56:5b:b3:68:8f:09,dl_dst=e6:6f:38:26:34:c4,ipv6_src=fc00::2,ipv6_dst=fc00::1,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=255,icmp_type=136,icmp_code=0,nd_target=fc00::2,nd_sll=00:00:00:00:00:00,nd_tll=56:5b:b3:68:8f:09 icmp6_csum:88ce
    +])
    +
    +OVS_TRAFFIC_VSWITCHD_STOP
    +AT_CLEANUP
    +
     AT_SETUP([conntrack - preserve registers])
     CHECK_CONNTRACK()
     OVS_TRAFFIC_VSWITCHD_START()
    -- 
    2.10.2
    
    



More information about the dev mailing list