Re: [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison

[Dr. David Alan Gilbert]

* Zhang Chen (address@hidden) wrote:
> Signed-off-by: Zhang Chen <address@hidden>
> Signed-off-by: Li Zhijian <address@hidden>
> Signed-off-by: Wen Congyang <address@hidden>
> ---
>  net/colo-compare.c | 158 
> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 154 insertions(+), 4 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 4b5a2d4..3dad461 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -385,9 +385,148 @@ static int colo_packet_compare(Packet *ppkt, Packet 
> *spkt)
>      }
>  }
>  
> -static int colo_packet_compare_all(Packet *spkt, Packet *ppkt)
> +/*
> + * called from the compare thread on the primary
> + * for compare tcp packet
> + * compare_tcp copied from Dr. David Alan Gilbert's branch
> + */
> +static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
> +{
> +    struct tcphdr *ptcp, *stcp;
> +    int res;
> +    char *sdebug, *ddebug;
> +    ptrdiff_t offset;
> +
> +    trace_colo_compare_main("compare tcp");
> +    ptcp = (struct tcphdr *)ppkt->transport_layer;
> +    stcp = (struct tcphdr *)spkt->transport_layer;
> +
> +    /* Initial is compare the whole packet */
> +    offset = 12; /* Hack! Skip virtio header */

So, when I post a set of patches and mark it saying that I know they've
got a lot of hacks in them, it's good for those reusing those patches
to check they need the hacks!

In my world I found I needed to skip over that header and I didn't understand
why; but hadn't figured out the details yet, and I'd added the 12 everywhere -
I think this is the only place you've got it, so it's almost certainly wrong.

> +    if (ptcp->th_flags == stcp->th_flags &&
> +        ((ptcp->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
> +        /* This is the syn/ack response from the guest to an incoming
> +         * connection; the secondary won't have matched the sequence number
> +         * Note: We should probably compare the IP level?
> +         * Note hack: This already has the virtio offset
> +         */
> +        offset = sizeof(ptcp->th_ack) + (void *)&ptcp->th_ack - ppkt->data;
> +    }
> +    /* Note - we want to compare everything as long as it's not the syn/ack? 
> */
> +    assert(offset > 0);
> +    assert(spkt->size > offset);
> +
> +    /* The 'identification' field in the IP header is *very* random
> +     * it almost never matches.  Fudge this by ignoring differences in
> +     * unfragmented packets; they'll normally sort themselves out if 
> different
> +     * anyway, and it should recover at the TCP level.
> +     * An alternative would be to get both the primary and secondary to 
> rewrite
> +     * somehow; but that would need some sync traffic to sync the state
> +     */
> +    if (ntohs(ppkt->ip->ip_off) & IP_DF) {
> +        spkt->ip->ip_id = ppkt->ip->ip_id;
> +        /* and the sum will be different if the IDs were different */
> +        spkt->ip->ip_sum = ppkt->ip->ip_sum;
> +    }
> +
> +    res = memcmp(ppkt->data + offset, spkt->data + offset,
> +                 (spkt->size - offset));
> +
> +    if (res && DEBUG_TCP_COMPARE) {
> +        sdebug = strdup(inet_ntoa(ppkt->ip->ip_src));
> +        ddebug = strdup(inet_ntoa(ppkt->ip->ip_dst));
> +        fprintf(stderr, "%s: src/dst: %s/%s offset=%zd p: seq/ack=%u/%u"
> +        " s: seq/ack=%u/%u res=%d flags=%x/%x\n", __func__,
> +                   sdebug, ddebug, offset,
> +                   ntohl(ptcp->th_seq), ntohl(ptcp->th_ack),
> +                   ntohl(stcp->th_seq), ntohl(stcp->th_ack),
> +                   res, ptcp->th_flags, stcp->th_flags);
> +        if (res && (ptcp->th_seq == stcp->th_seq)) {
> +            trace_colo_compare_with_int("Primary len", ppkt->size);
> +            colo_dump_packet(ppkt);
> +            trace_colo_compare_with_int("Secondary len", spkt->size);
> +            colo_dump_packet(spkt);
> +        }

Try and use meaningful traceing for this - don't use a 'compare_with_int'
trace; but use a name that says what you're doing - for example
trace_colo_tcp_miscompare ; that way if you're running COLO and just
want to see why you're getting so many miscompares, you can look
at this without turning on all the rest of the debug.

Also, in my version instead of using a DEBUG_TCP macro, I again used
the trace system, so, my code here was:

    if (trace_event_get_state(TRACE_COLO_PROXY_MISCOMPARE) && res) {

    that means you can switch it on and off at runtime using the
trace system.  Then just as it's running I can get to the (qemu) prompt
and do:
       trace-event colo_proxy_miscompare on

   and see what's happening without recompiling.

> +        g_free(sdebug);
> +        g_free(ddebug);
> +    }
> +
> +    return res;
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare udp packet
> + */
> +static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
> +{
> +    int ret = 1;
> +
> +    trace_colo_compare_main("compare udp");
> +    ret = colo_packet_compare(ppkt, spkt);
> +
> +    if (ret) {
> +        trace_colo_compare_main("primary udp");
> +        colo_dump_packet(ppkt);
> +        trace_colo_compare_main("secondary udp");
> +        colo_dump_packet(spkt);
> +    }
> +
> +    return ret;
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare icmp packet
> + */
> +static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
> +{
> +    int network_length;
> +    struct icmp *icmp_ppkt, *icmp_spkt;
> +
> +    trace_colo_compare_main("compare icmp");
> +    network_length = (ppkt->ip->ip_hl & 0x0F) * 4;

Do you need that & 0x0f - the definition in ip.h is ip_hl:4 ?

> +    icmp_ppkt = (struct icmp *)(ppkt->data + network_length + ETH_HLEN);
> +    icmp_spkt = (struct icmp *)(spkt->data + network_length + ETH_HLEN);
> +
> +    if ((icmp_ppkt->icmp_type == icmp_spkt->icmp_type) &&
> +        (icmp_ppkt->icmp_code == icmp_spkt->icmp_code)) {
> +        if (icmp_ppkt->icmp_type == ICMP_REDIRECT) {

Do you need to check the lengths again before reading any of these fields?

> +            if (icmp_ppkt->icmp_gwaddr.s_addr !=
> +                icmp_spkt->icmp_gwaddr.s_addr) {
> +                trace_colo_compare_main("icmp_gwaddr.s_addr not same");
> +                trace_colo_compare_with_char("ppkt s_addr",
> +                        inet_ntoa(icmp_ppkt->icmp_gwaddr));
> +                trace_colo_compare_with_char("spkt s_addr",
> +                        inet_ntoa(icmp_spkt->icmp_gwaddr));
> +                return -1;
> +            }
> +        } else if ((icmp_ppkt->icmp_type == ICMP_UNREACH) &&
> +                   (icmp_ppkt->icmp_type == ICMP_UNREACH_NEEDFRAG)) {
> +            if (icmp_ppkt->icmp_nextmtu != icmp_spkt->icmp_nextmtu) {
> +                trace_colo_compare_main("icmp_nextmtu not same");
> +                trace_colo_compare_with_int("ppkt s_addr",
> +                                             icmp_ppkt->icmp_nextmtu);
> +                trace_colo_compare_with_int("spkt s_addr",
> +                                             icmp_spkt->icmp_nextmtu);
> +                return -1;
> +            }
> +        }
> +    } else {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare other packet
> + */
> +static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
>  {
> -    trace_colo_compare_main("compare all");
> +    trace_colo_compare_main("compare other");

Try and make the traces give you all the information you're likely to need - for
example, adding ip_proto here would be really useful for when you find you've
suddenly got a lot of miscompare compare others and want to figure out why.

>      return colo_packet_compare(ppkt, spkt);
>  }
>  
> @@ -406,8 +545,19 @@ static void colo_compare_connection(void *opaque, void 
> *user_data)
>      while (!g_queue_is_empty(&conn->primary_list) &&
>             !g_queue_is_empty(&conn->secondary_list)) {
>          pkt = g_queue_pop_head(&conn->primary_list);
> -        result = g_queue_find_custom(&conn->secondary_list,
> -                              pkt, (GCompareFunc)colo_packet_compare_all);
> +        if (conn->ip_proto == IPPROTO_TCP) {
> +            result = g_queue_find_custom(&conn->secondary_list,
> +                        pkt, (GCompareFunc)colo_packet_compare_tcp);
> +        } else if (conn->ip_proto == IPPROTO_UDP) {
> +            result = g_queue_find_custom(&conn->secondary_list,
> +                        pkt, (GCompareFunc)colo_packet_compare_udp);
> +        } else if (conn->ip_proto == IPPROTO_ICMP) {
> +            result = g_queue_find_custom(&conn->secondary_list,
> +                        pkt, (GCompareFunc)colo_packet_compare_icmp);
> +        } else {
> +            result = g_queue_find_custom(&conn->secondary_list,
> +                        pkt, (GCompareFunc)colo_packet_compare_other);
> +        }
>  
>          if (result) {
>              ret = compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size);
> -- 
> 1.9.1

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK