[ovs-dev] [patch v2 1/2] conntrack: Fix fragmentation checks.

Justin Pettit jpettit at gmail.com
Wed Jun 27 21:38:10 UTC 2018


Thanks for the patch.  It doesn't seem to apply cleanly against master.  Can you please respin it?

Also, the note about backporting it to 2.9 is helpful, but can you move it to a place where it won't be part of the commit message?

--Justin


> On Jun 25, 2018, at 11:33 AM, Darrell Ball <dlu998 at gmail.com> wrote:
> 
> The ipv4 fragmentation check is broken and allows fragments through.
> There were fragile and poorly maintainable checks in extract_l3_ipv*
> designed to save a few cycles.  The checks make assumptions about what
> sanity checks may have been done and could be skipped based on inferring
> from the value of another paramater that should be unrelated (l4
> pointer needing assignment).  Since the benefit is minimal, remove
> the special checks and always do sanity checks.  Some basic
> optimization is added for the sanity checks and some parameter
> ordering is changed for the functions being touched.
> 
> Four tests are added to better maintain fragmentation support.
> 
> This needs backporting to 2.9.
> 
> Fixes: c8b1ad49da68("conntrack: Reorder sanity checks in extract_l3_ipvx().")
> Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
> Signed-off-by: Darrell Ball <dlu998 at gmail.com>
> ---
> lib/conntrack.c         | 95 +++++++++++++++++++++----------------------------
> tests/system-traffic.at | 92 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 132 insertions(+), 55 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 82229fa..825f1ad 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -123,13 +123,14 @@ static uint8_t
> reverse_icmp_type(uint8_t type);
> static uint8_t
> reverse_icmp6_type(uint8_t type);
> -static inline bool
> -extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
> -                const char **new_data, bool validate_checksum);
> -static inline bool
> -extract_l3_ipv6(struct conn_key *key, const void *data, size_t size,
> -                const char **new_data);
> -
> +static inline bool extract_l3_ipv4(const void *data, size_t size,
> +                                   bool validate_checksum,
> +                                   struct conn_key *key,
> +                                   const char **new_data);
> +
> +static inline bool extract_l3_ipv6(const void *data, size_t size,
> +                                   struct conn_key *key,
> +                                   const char **new_data);
> static struct alg_exp_node *
> expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key,
>                    uint32_t basis, bool src_ip_wc);
> @@ -648,8 +649,8 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
>         struct ip_header *nh = dp_packet_l3(pkt);
>         struct icmp_header *icmp = dp_packet_l4(pkt);
>         struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1);
> -        extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3) - pad,
> -                        &inner_l4, false);
> +        extract_l3_ipv4(inner_l3, tail - (char *)inner_l3 - pad, false,
> +                        &inner_key, &inner_l4);
>         pkt->l3_ofs += (char *) inner_l3 - (char *) nh;
>         pkt->l4_ofs += inner_l4 - (char *) icmp;
> 
> @@ -669,9 +670,8 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
>         struct icmp6_error_header *icmp6 = dp_packet_l4(pkt);
>         struct ovs_16aligned_ip6_hdr *inner_l3_6 =
>             (struct ovs_16aligned_ip6_hdr *) (icmp6 + 1);
> -        extract_l3_ipv6(&inner_key, inner_l3_6,
> -                        tail - ((char *)inner_l3_6) - pad,
> -                        &inner_l4);
> +        extract_l3_ipv6(inner_l3_6, tail - ((char *)inner_l3_6) - pad,
> +                        &inner_key, &inner_l4);
>         pkt->l3_ofs += (char *) inner_l3_6 - (char *) nh6;
>         pkt->l4_ofs += inner_l4 - (char *) icmp6;
> 
> @@ -1497,46 +1497,37 @@ clean_thread_main(void *f_)
>     return NULL;
> }
> 
> -/* Key extraction */
> -
> -/* The function stores a pointer to the first byte after the header in
> - * '*new_data', if 'new_data' is not NULL.  If it is NULL, the caller is
> - * not interested in the header's tail,  meaning that the header has
> - * already been parsed (e.g. by flow_extract): we take this as a hint to
> - * save a few checks.  If 'validate_checksum' is true, the function returns
> - * false if the IPv4 checksum is invalid. */
> static inline bool
> -extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
> -                const char **new_data, bool validate_checksum)
> +extract_l3_ipv4(const void *data, size_t size, bool validate_checksum,
> +                struct conn_key *key, const char **new_data)
> {
> -    if (new_data) {
> -        if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> -            return false;
> -        }
> +    if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> +        return false;
>     }
> 
>     const struct ip_header *ip = data;
>     size_t ip_len = IP_IHL(ip->ip_ihl_ver) * 4;
> 
> -    if (new_data) {
> -        if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
> -            return false;
> -        }
> -        if (OVS_UNLIKELY(size < ip_len)) {
> -            return false;
> -        }
> +    if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
> +        return false;
> +    }
> 
> -        if (IP_IS_FRAGMENT(ip->ip_frag_off)) {
> -            return false;
> -        }
> +    if (OVS_UNLIKELY(size < ip_len)) {
> +        return false;
> +    }
> 
> -        *new_data = (char *) data + ip_len;
> +    if (OVS_UNLIKELY(validate_checksum && csum(data, ip_len) != 0)) {
> +        return false;
>     }
> 
> -    if (validate_checksum && csum(data, ip_len) != 0) {
> +    if (OVS_UNLIKELY(IP_IS_FRAGMENT(ip->ip_frag_off))) {
>         return false;
>     }
> 
> +    if (new_data) {
> +        *new_data = (char *) data + ip_len;
> +    }
> +
>     key->src.addr.ipv4 = ip->ip_src;
>     key->dst.addr.ipv4 = ip->ip_dst;
>     key->nw_proto = ip->ip_proto;
> @@ -1544,21 +1535,14 @@ extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
>     return true;
> }
> 
> -/* The function stores a pointer to the first byte after the header in
> - * '*new_data', if 'new_data' is not NULL.  If it is NULL, the caller is
> - * not interested in the header's tail,  meaning that the header has
> - * already been parsed (e.g. by flow_extract): we take this as a hint to
> - * save a few checks. */
> static inline bool
> -extract_l3_ipv6(struct conn_key *key, const void *data, size_t size,
> +extract_l3_ipv6(const void *data, size_t size, struct conn_key *key,
>                 const char **new_data)
> {
>     const struct ovs_16aligned_ip6_hdr *ip6 = data;
> 
> -    if (new_data) {
> -        if (OVS_UNLIKELY(size < sizeof *ip6)) {
> -            return false;
> -        }
> +    if (OVS_UNLIKELY(size < sizeof *ip6)) {
> +        return false;
>     }
> 
>     data = ip6 + 1;
> @@ -1566,11 +1550,12 @@ extract_l3_ipv6(struct conn_key *key, const void *data, size_t size,
>     uint8_t nw_proto = ip6->ip6_nxt;
>     uint8_t nw_frag = 0;
> 
> -    if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag)) {
> +    if (OVS_UNLIKELY(!parse_ipv6_ext_hdrs(&data, &size, &nw_proto,
> +                                          &nw_frag))) {
>         return false;
>     }
> 
> -    if (nw_frag) {
> +    if (OVS_UNLIKELY(nw_frag)) {
>         return false;
>     }
> 
> @@ -1758,7 +1743,7 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
> 
>         memset(&inner_key, 0, sizeof inner_key);
>         inner_key.dl_type = htons(ETH_TYPE_IP);
> -        bool ok = extract_l3_ipv4(&inner_key, l3, tail - l3, &l4, false);
> +        bool ok = extract_l3_ipv4(l3, tail - l3, false, &inner_key, &l4);
>         if (!ok) {
>             return false;
>         }
> @@ -1843,7 +1828,7 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
> 
>         memset(&inner_key, 0, sizeof inner_key);
>         inner_key.dl_type = htons(ETH_TYPE_IPV6);
> -        bool ok = extract_l3_ipv6(&inner_key, l3, tail - l3, &l4);
> +        bool ok = extract_l3_ipv6(l3, tail - l3, &inner_key, &l4);
>         if (!ok) {
>             return false;
>         }
> @@ -1964,11 +1949,11 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
>         } else {
>             bool hwol_good_l3_csum = dp_packet_ip_checksum_valid(pkt);
>             /* Validate the checksum only when hwol is not supported. */
> -            ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL,
> -                                 !hwol_good_l3_csum);
> +            ok = extract_l3_ipv4(l3, tail - (char *) l3, !hwol_good_l3_csum,
> +                                 &ctx->key,  NULL);
>         }
>     } else if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> -        ok = extract_l3_ipv6(&ctx->key, l3, tail - (char *) l3, NULL);
> +        ok = extract_l3_ipv6(l3, tail - (char *) l3, &ctx->key, NULL);
>     } else {
>         ok = false;
>     }
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 7080efe..61d8bc0 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2057,6 +2057,52 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.255.2.2 | FORMAT_PI
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
> 
> +AT_SETUP([conntrack - IPv4 fragmentation incomplete reassembled packet])
> +CHECK_CONNTRACK()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +AT_DATA([bundle.txt], [dnl
> +packet-out in_port=1, packet=50540000000a5054000000090800450001a400012000001183440a0101010a01010200010002000800000304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809,  actions=ct(commit)
> +])
> +
> +AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +dnl Uses same first fragment as above 'incomplete reassembled packet' test.
> +AT_SETUP([conntrack - IPv4 fragmentation with fragments specified])
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_FRAG()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +AT_DATA([bundle.txt], [dnl
> +packet-out in_port=1, packet=50540000000a5054000000090800450001a400012000001183440a0101010a01010200010002000800000304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809,  actions=ct(commit)
> +packet-out in_port=1, packet=50540000000a505400000009080045000030000100320011a4860a0101010a01010200010002000800000010203040506070809000010203040506070809, actions=ct(commit)
> +])
> +
> +AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>)
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([conntrack - IPv6 fragmentation])
> CHECK_CONNTRACK()
> CHECK_CONNTRACK_FRAG()
> @@ -2235,6 +2281,52 @@ NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -w 2 fc00:ffff::4 | FORMAT
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
> 
> +AT_SETUP([conntrack - IPv6 fragmentation incomplete reassembled packet])
> +CHECK_CONNTRACK()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "fc00::1/96")
> +ADD_VETH(p1, at_ns1, br0, "fc00::2/96")
> +
> +AT_DATA([bundle.txt], [dnl
> +packet-out in_port=1, packet=50540000000a50540000000986dd6000000005102cfffc000000000000000000000000000001fc0000000000000000000000000000021100000100000001000100020008f6290001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809
> 00010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030
> 4050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809,  actions=ct(commit)
> +])
> +
> +AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fc00::2)], [0], [dnl
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +dnl Uses same first fragment as above 'incomplete reassembled packet' test.
> +AT_SETUP([conntrack - IPv6 fragmentation with fragments specified])
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_FRAG()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "fc00::1/96")
> +ADD_VETH(p1, at_ns1, br0, "fc00::2/96")
> +
> +AT_DATA([bundle.txt], [dnl
> +packet-out in_port=1, packet=50540000000a50540000000986dd6000000005102cfffc000000000000000000000000000001fc0000000000000000000000000000021100000100000001000100020008cdf30001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809
> 00010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030
> 4050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809,  actions=ct(commit)
> +packet-out in_port=1, packet=50540000000a50540000000986dd6000000000242cfffc000000000000000000000000000001fc000000000000000000000000000002110005080000000100010002000800000001020304050607080900010203040506070809, actions=ct(commit)
> +])
> +
> +AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fc00::2)], [0], [dnl
> +udp,orig=(src=fc00::1,dst=fc00::2,sport=<cleared>,dport=<cleared>),reply=(src=fc00::2,dst=fc00::1,sport=<cleared>,dport=<cleared>)
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([conntrack - Fragmentation over vxlan])
> OVS_CHECK_VXLAN()
> CHECK_CONNTRACK()
> -- 
> 1.9.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list