[Qemu-devel] [RFC PATCH 0/3] filter-rewriter: introduce filter-rewriter

[Qemu-devel] [RFC PATCH 0/3] filter-rewriter: introduce filter-rewriter

[Zhang Chen]

Filter-rewriter is a part of COLO project.
It will rewrite some of secondary packet to make
secondary guest's connection established successfully.

Zhang Chen (3):
  filter-rewriter: introduce filter-rewriter initialization
  filter-rewriter: track connection and parse packet
  filter-rewriter: rewrite tcp packet to keep secondary connection

 net/Makefile.objs     |   1 +
 net/filter-rewriter.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx       |  10 ++
 trace-events          |   3 +
 vl.c                  |   3 +-
 5 files changed, 271 insertions(+), 1 deletion(-)
 create mode 100644 net/filter-rewriter.c

-- 
2.7.4

[Qemu-devel] [RFC PATCH 1/3] filter-rewriter: introduce filter-rewriter initialization

[Zhang Chen]

Filter-rewriter is a part of COLO project.
It will rewrite some of secondary packet to make
secondary guest's connection established successfully.

usage:

colo secondary:
-object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
-object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
-object filter-rewriter,id=rew0,netdev=hn0,queue=all

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 net/Makefile.objs     |   1 +
 net/filter-rewriter.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx       |  10 +++++
 vl.c                  |   3 +-
 4 files changed, 125 insertions(+), 1 deletion(-)
 create mode 100644 net/filter-rewriter.c

diff --git a/net/Makefile.objs b/net/Makefile.objs
index 119589f..645bd10 100644
--- a/net/Makefile.objs
+++ b/net/Makefile.objs
@@ -18,3 +18,4 @@ common-obj-y += filter-buffer.o
 common-obj-y += filter-mirror.o
 common-obj-y += colo-compare.o
 common-obj-y += colo-base.o
+common-obj-y += filter-rewriter.o
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
new file mode 100644
index 0000000..08b015d
--- /dev/null
+++ b/net/filter-rewriter.c
@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
+ * Copyright (c) 2016 FUJITSU LIMITED
+ * Copyright (c) 2016 Intel Corporation
+ *
+ * Author: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later.  See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "net/colo-base.h"
+#include "net/filter.h"
+#include "net/net.h"
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi-visit.h"
+#include "qom/object.h"
+#include "qemu/main-loop.h"
+#include "qemu/iov.h"
+#include "net/checksum.h"
+
+#define FILTER_COLO_REWRITER(obj) \
+    OBJECT_CHECK(RewriterState, (obj), TYPE_FILTER_REWRITER)
+
+#define TYPE_FILTER_REWRITER "filter-rewriter"
+
+enum {
+    PRIMARY = 0,
+    SECONDARY,
+};
+
+typedef struct RewriterState {
+    NetFilterState parent_obj;
+    /* connection list: the connections belonged to this NIC could be found
+     * in this list.
+     * element type: Connection
+     */
+    GQueue conn_list;
+    NetQueue *incoming_queue;
+    /* to protect conn_list */
+    QemuMutex conn_list_lock;
+    /* hashtable to save connection */
+    GHashTable *connection_track_table;
+    /* to save unprocessed_connections */
+    GQueue unprocessed_connections;
+    /* current hash size */
+    uint32_t hashtable_size;
+} RewriterState;
+
+static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
+                                         NetClientState *sender,
+                                         unsigned flags,
+                                         const struct iovec *iov,
+                                         int iovcnt,
+                                         NetPacketSent *sent_cb)
+{
+    /*
+     * if we get tcp packet
+     * we will rewrite it to make secondary guest's
+     * connection established successfully
+     */
+    return 0;
+}
+
+static void colo_rewriter_cleanup(NetFilterState *nf)
+{
+    RewriterState *s = FILTER_COLO_REWRITER(nf);
+
+    qemu_mutex_destroy(&s->conn_list_lock);
+    g_queue_free(&s->conn_list);
+}
+
+static void colo_rewriter_setup(NetFilterState *nf, Error **errp)
+{
+    RewriterState *s = FILTER_COLO_REWRITER(nf);
+
+    g_queue_init(&s->conn_list);
+    qemu_mutex_init(&s->conn_list_lock);
+    s->hashtable_size = 0;
+
+    s->connection_track_table = g_hash_table_new_full(connection_key_hash,
+                                                      connection_key_equal,
+                                                      g_free,
+                                                      connection_destroy);
+    s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
+}
+
+static void colo_rewriter_class_init(ObjectClass *oc, void *data)
+{
+    NetFilterClass *nfc = NETFILTER_CLASS(oc);
+
+    nfc->setup = colo_rewriter_setup;
+    nfc->cleanup = colo_rewriter_cleanup;
+    nfc->receive_iov = colo_rewriter_receive_iov;
+}
+
+static const TypeInfo colo_rewriter_info = {
+    .name = TYPE_FILTER_REWRITER,
+    .parent = TYPE_NETFILTER,
+    .class_init = colo_rewriter_class_init,
+    .instance_size = sizeof(RewriterState),
+};
+
+static void register_types(void)
+{
+    type_register_static(&colo_rewriter_info);
+}
+
+type_init(register_types);
diff --git a/qemu-options.hx b/qemu-options.hx
index 14bade5..d7ab165 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3859,6 +3859,16 @@ Create a filter-redirector we need to differ outdev id from indev id, id can not
 be the same. we can just use indev or outdev, but at least one of indev or outdev
 need to be specified.
 
+@item -object filter-rewriter,id=@var{id},netdev=@var{netdevid},rewriter-mode=@var{mode}[,queue=@var{all|rx|tx}]
+
+Filter-rewriter is a part of COLO project.It will rewrite some of secondary packet.
+
+usage:
+colo secondary:
+-object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
+-object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
+-object filter-rewriter,id=rew0,netdev=hn0,queue=all
+
 @item -object filter-dump,id=@var{id},netdev=@var{dev},file=@var{filename}][,maxlen=@var{len}]
 
 Dump the network traffic on netdev @var{dev} to the file specified by
diff --git a/vl.c b/vl.c
index c6b9a6f..b47be6a 100644
--- a/vl.c
+++ b/vl.c
@@ -2866,7 +2866,8 @@ static bool object_create_initial(const char *type)
         g_str_equal(type, "filter-dump") ||
         g_str_equal(type, "filter-mirror") ||
         g_str_equal(type, "filter-redirector") ||
-        g_str_equal(type, "colo-compare")) {
+        g_str_equal(type, "colo-compare") ||
+        g_str_equal(type, "filter-rewriter")) {
         return false;
     }
 
-- 
2.7.4

[Qemu-devel] [RFC PATCH 2/3] filter-rewriter: track connection and parse packet

[Zhang Chen]

We use colo-base.h to track connection and parse packet

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 net/filter-rewriter.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index 08b015d..12f88c5 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -50,6 +50,20 @@ typedef struct RewriterState {
     uint32_t hashtable_size;
 } RewriterState;
 
+/*
+ * Return 1 on success, if return 0 means the pkt
+ * is not TCP packet
+ */
+static int is_tcp_packet(Packet *pkt)
+{
+    if (!parse_packet_early(pkt) &&
+        pkt->ip->ip_p == IPPROTO_TCP) {
+        return 1;
+    } else {
+        return 0;
+    }
+}
+
 static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
                                          NetClientState *sender,
                                          unsigned flags,
@@ -57,11 +71,50 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
                                          int iovcnt,
                                          NetPacketSent *sent_cb)
 {
+    RewriterState *s = FILTER_COLO_REWRITER(nf);
+    Connection *conn;
+    ConnectionKey key = {{ 0 } };
+    Packet *pkt;
+    ssize_t size = iov_size(iov, iovcnt);
+    char *buf = g_malloc0(size);
+
+    iov_to_buf(iov, iovcnt, 0, buf, size);
+    pkt = packet_new(buf, size);
+
     /*
      * if we get tcp packet
      * we will rewrite it to make secondary guest's
      * connection established successfully
      */
+    if (is_tcp_packet(pkt)) {
+        if (sender == nf->netdev) {
+            fill_connection_key(pkt, &key, SECONDARY);
+        } else {
+            fill_connection_key(pkt, &key, PRIMARY);
+        }
+
+        conn = connection_get(s->connection_track_table,
+                              &key,
+                              &s->hashtable_size);
+        if (!conn->processing) {
+            qemu_mutex_lock(&s->conn_list_lock);
+            g_queue_push_tail(&s->conn_list, conn);
+            qemu_mutex_unlock(&s->conn_list_lock);
+            conn->processing = true;
+        }
+
+        if (sender == nf->netdev) {
+            /* This packet is sent by netdev itself */
+            /* NET_FILTER_DIRECTION_TX */
+            /* handle_primary_tcp_pkt */
+        } else {
+            /* NET_FILTER_DIRECTION_RX */
+            /* handle_secondary_tcp_pkt */
+        }
+    }
+
+    packet_destroy(pkt, NULL);
+    pkt = NULL;
     return 0;
 }
 
-- 
2.7.4

[Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Zhang Chen]

We will rewrite tcp packet secondary received and sent.

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 net/filter-rewriter.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--
 trace-events          |  3 ++
 2 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index 12f88c5..86a2f53 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -21,6 +21,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/iov.h"
 #include "net/checksum.h"
+#include "trace.h"
 
 #define FILTER_COLO_REWRITER(obj) \
     OBJECT_CHECK(RewriterState, (obj), TYPE_FILTER_REWRITER)
@@ -64,6 +65,75 @@ static int is_tcp_packet(Packet *pkt)
     }
 }
 
+static int handle_primary_tcp_pkt(NetFilterState *nf,
+                                  Connection *conn,
+                                  Packet *pkt)
+{
+    struct tcphdr *tcp_pkt;
+
+    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
+
+    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
+        char *sdebug, *ddebug;
+        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
+        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
+        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
+                "  flags=%x\n", __func__, sdebug, ddebug,
+                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
+                tcp_pkt->th_flags);
+        g_free(sdebug);
+        g_free(ddebug);
+    }
+
+    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK)) {
+        /* save primary colo tcp packet seq */
+        conn->primary_seq = ntohl(tcp_pkt->th_ack) - 1;
+
+        /* adjust tcp seq to make secondary guest handle it */
+        tcp_pkt->th_ack = htonl(conn->secondary_seq + 1);
+        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+    }
+
+    return 0;
+}
+
+static int handle_secondary_tcp_pkt(NetFilterState *nf,
+                                    Connection *conn,
+                                    Packet *pkt)
+{
+    struct tcphdr *tcp_pkt;
+
+    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
+
+    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
+        char *sdebug, *ddebug;
+        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
+        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
+        printf("handle_secondary_tcp_pkt conn->secondary_seq = %u,\n",
+               conn->secondary_seq);
+        printf("handle_secondary_tcp_pkt conn->primary_seq = %u,\n",
+               conn->primary_seq);
+        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
+                "  flags=%x\n", __func__, sdebug, ddebug,
+                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
+                tcp_pkt->th_flags);
+        g_free(sdebug);
+        g_free(ddebug);
+    }
+
+    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
+        /* save client's seq */
+        conn->secondary_seq = ntohl(tcp_pkt->th_seq);
+    }
+
+    if ((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK) {
+        tcp_pkt->th_seq = htonl(conn->primary_seq + 1);
+        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+    }
+
+    return 0;
+}
+
 static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
                                          NetClientState *sender,
                                          unsigned flags,
@@ -106,10 +176,30 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
         if (sender == nf->netdev) {
             /* This packet is sent by netdev itself */
             /* NET_FILTER_DIRECTION_TX */
-            /* handle_primary_tcp_pkt */
+            if (!handle_primary_tcp_pkt(nf, conn, pkt)) {
+                qemu_net_queue_send(s->incoming_queue, sender, 0,
+                (const uint8_t *)pkt->data, pkt->size, NULL);
+                packet_destroy(pkt, NULL);
+                pkt = NULL;
+                /*
+                 * We block the packet here,after rewrite pkt
+                 * and will send it
+                 */
+                return 1;
+            }
         } else {
             /* NET_FILTER_DIRECTION_RX */
-            /* handle_secondary_tcp_pkt */
+            if (!handle_secondary_tcp_pkt(nf, conn, pkt)) {
+                qemu_net_queue_send(s->incoming_queue, sender, 0,
+                (const uint8_t *)pkt->data, pkt->size, NULL);
+                packet_destroy(pkt, NULL);
+                pkt = NULL;
+                /*
+                 * We block the packet here,after rewrite pkt
+                 * and will send it
+                 */
+                return 1;
+            }
         }
     }
 
diff --git a/trace-events b/trace-events
index 6686cdf..5d798c6 100644
--- a/trace-events
+++ b/trace-events
@@ -1927,3 +1927,6 @@ colo_compare_icmp_miscompare_mtu(const char *sta, int size) ": %s  %d"
 colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
 colo_old_packet_check_found(int64_t old_time) "%" PRId64
 colo_compare_miscompare(void) ""
+
+# net/filter-rewriter.c
+colo_filter_rewriter_debug(void) ""
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Jason Wang]

On 2016年06月14日 19:15, Zhang Chen wrote:
> We will rewrite tcp packet secondary received and sent.

More verbose please. E.g which fields were rewrote and why.

> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>   net/filter-rewriter.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--
>   trace-events          |  3 ++
>   2 files changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> index 12f88c5..86a2f53 100644
> --- a/net/filter-rewriter.c
> +++ b/net/filter-rewriter.c
> @@ -21,6 +21,7 @@
>   #include "qemu/main-loop.h"
>   #include "qemu/iov.h"
>   #include "net/checksum.h"
> +#include "trace.h"
>   
>   #define FILTER_COLO_REWRITER(obj) \
>       OBJECT_CHECK(RewriterState, (obj), TYPE_FILTER_REWRITER)
> @@ -64,6 +65,75 @@ static int is_tcp_packet(Packet *pkt)
>       }
>   }
>   
> +static int handle_primary_tcp_pkt(NetFilterState *nf,
> +                                  Connection *conn,
> +                                  Packet *pkt)
> +{
> +    struct tcphdr *tcp_pkt;
> +
> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
> +
> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {

Why not use tracepoints directly?

> +        char *sdebug, *ddebug;
> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
> +                "  flags=%x\n", __func__, sdebug, ddebug,
> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
> +                tcp_pkt->th_flags);
> +        g_free(sdebug);
> +        g_free(ddebug);
> +    }
> +
> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK)) {
> +        /* save primary colo tcp packet seq */
> +        conn->primary_seq = ntohl(tcp_pkt->th_ack) - 1;

Looks like primary_seq will only be updated during handshake, I wonder 
how this works.

> +
> +        /* adjust tcp seq to make secondary guest handle it */
> +        tcp_pkt->th_ack = htonl(conn->secondary_seq + 1);

I'm not sure this can work for all cases. I believe we should also 
rewrite seq here. And to me, a better approach is to track the offset of 
seq between pri and sec during handshake and rewrite both ack and seq 
based on this offset.

> +        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
> +    }
> +
> +    return 0;
> +}
> +
> +static int handle_secondary_tcp_pkt(NetFilterState *nf,
> +                                    Connection *conn,
> +                                    Packet *pkt)
> +{
> +    struct tcphdr *tcp_pkt;
> +
> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
> +
> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
> +        char *sdebug, *ddebug;
> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
> +        printf("handle_secondary_tcp_pkt conn->secondary_seq = %u,\n",
> +               conn->secondary_seq);
> +        printf("handle_secondary_tcp_pkt conn->primary_seq = %u,\n",
> +               conn->primary_seq);
> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
> +                "  flags=%x\n", __func__, sdebug, ddebug,
> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
> +                tcp_pkt->th_flags);
> +        g_free(sdebug);
> +        g_free(ddebug);
> +    }
> +
> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
> +        /* save client's seq */
> +        conn->secondary_seq = ntohl(tcp_pkt->th_seq);
> +    }
> +
> +    if ((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK) {
> +        tcp_pkt->th_seq = htonl(conn->primary_seq + 1);
> +        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
> +    }
> +
> +    return 0;
> +}
> +
>   static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
>                                            NetClientState *sender,
>                                            unsigned flags,
> @@ -106,10 +176,30 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
>           if (sender == nf->netdev) {
>               /* This packet is sent by netdev itself */
>               /* NET_FILTER_DIRECTION_TX */
> -            /* handle_primary_tcp_pkt */
> +            if (!handle_primary_tcp_pkt(nf, conn, pkt)) {
> +                qemu_net_queue_send(s->incoming_queue, sender, 0,
> +                (const uint8_t *)pkt->data, pkt->size, NULL);
> +                packet_destroy(pkt, NULL);
> +                pkt = NULL;
> +                /*
> +                 * We block the packet here,after rewrite pkt
> +                 * and will send it
> +                 */
> +                return 1;
> +            }
>           } else {
>               /* NET_FILTER_DIRECTION_RX */
> -            /* handle_secondary_tcp_pkt */
> +            if (!handle_secondary_tcp_pkt(nf, conn, pkt)) {
> +                qemu_net_queue_send(s->incoming_queue, sender, 0,
> +                (const uint8_t *)pkt->data, pkt->size, NULL);
> +                packet_destroy(pkt, NULL);
> +                pkt = NULL;
> +                /*
> +                 * We block the packet here,after rewrite pkt
> +                 * and will send it
> +                 */
> +                return 1;
> +            }
>           }
>       }
>   
> diff --git a/trace-events b/trace-events
> index 6686cdf..5d798c6 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1927,3 +1927,6 @@ colo_compare_icmp_miscompare_mtu(const char *sta, int size) ": %s  %d"
>   colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
>   colo_old_packet_check_found(int64_t old_time) "%" PRId64
>   colo_compare_miscompare(void) ""
> +
> +# net/filter-rewriter.c
> +colo_filter_rewriter_debug(void) ""

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Dr. David Alan Gilbert]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> 
> On 2016年06月14日 19:15, Zhang Chen wrote:
> > We will rewrite tcp packet secondary received and sent.
> 
> More verbose please. E.g which fields were rewrote and why.
> 
> > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> > ---
> >   net/filter-rewriter.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--
> >   trace-events          |  3 ++
> >   2 files changed, 95 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> > index 12f88c5..86a2f53 100644
> > --- a/net/filter-rewriter.c
> > +++ b/net/filter-rewriter.c
> > @@ -21,6 +21,7 @@
> >   #include "qemu/main-loop.h"
> >   #include "qemu/iov.h"
> >   #include "net/checksum.h"
> > +#include "trace.h"
> >   #define FILTER_COLO_REWRITER(obj) \
> >       OBJECT_CHECK(RewriterState, (obj), TYPE_FILTER_REWRITER)
> > @@ -64,6 +65,75 @@ static int is_tcp_packet(Packet *pkt)
> >       }
> >   }
> > +static int handle_primary_tcp_pkt(NetFilterState *nf,
> > +                                  Connection *conn,
> > +                                  Packet *pkt)
> > +{
> > +    struct tcphdr *tcp_pkt;
> > +
> > +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
> > +
> > +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
> 
> Why not use tracepoints directly?

Because trace can't cope with you having to do an allocation/free.

> > +        char *sdebug, *ddebug;
> > +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
> > +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
> > +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
> > +                "  flags=%x\n", __func__, sdebug, ddebug,
> > +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
> > +                tcp_pkt->th_flags);

However, this should use the trace_ call to write the result even if it's
using trace_event_get_state to switch the whole block on/off.

> > +        g_free(sdebug);
> > +        g_free(ddebug);
> > +    }
> > +
> > +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK)) {
> > +        /* save primary colo tcp packet seq */
> > +        conn->primary_seq = ntohl(tcp_pkt->th_ack) - 1;
> 
> Looks like primary_seq will only be updated during handshake, I wonder how
> this works.

This code really needs commenting to make it see what's going on; each
of these functions should say which way the packet is going (e.g.
 'handle packets to the primary from the secondary') - there's a lot
of packet flows going on and without the comments it's very hard to follow.

I think this could be because we're fixing up the sequence numbers on the 
secondary once we've received the first response from the primary, so it's
only the first packet of each connection that the primary has to do this on -
but hmm I'm not sure without some comments.

Dave

> > +
> > +        /* adjust tcp seq to make secondary guest handle it */
> > +        tcp_pkt->th_ack = htonl(conn->secondary_seq + 1);
> 
> I'm not sure this can work for all cases. I believe we should also rewrite
> seq here. And to me, a better approach is to track the offset of seq between
> pri and sec during handshake and rewrite both ack and seq based on this
> offset.
> 
> > +        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int handle_secondary_tcp_pkt(NetFilterState *nf,
> > +                                    Connection *conn,
> > +                                    Packet *pkt)
> > +{
> > +    struct tcphdr *tcp_pkt;
> > +
> > +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
> > +
> > +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
> > +        char *sdebug, *ddebug;
> > +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
> > +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
> > +        printf("handle_secondary_tcp_pkt conn->secondary_seq = %u,\n",
> > +               conn->secondary_seq);
> > +        printf("handle_secondary_tcp_pkt conn->primary_seq = %u,\n",
> > +               conn->primary_seq);
> > +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
> > +                "  flags=%x\n", __func__, sdebug, ddebug,
> > +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
> > +                tcp_pkt->th_flags);
> > +        g_free(sdebug);
> > +        g_free(ddebug);
> > +    }
> > +
> > +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
> > +        /* save client's seq */
> > +        conn->secondary_seq = ntohl(tcp_pkt->th_seq);
> > +    }
> > +
> > +    if ((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK) {
> > +        tcp_pkt->th_seq = htonl(conn->primary_seq + 1);
> > +        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
> >                                            NetClientState *sender,
> >                                            unsigned flags,
> > @@ -106,10 +176,30 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
> >           if (sender == nf->netdev) {
> >               /* This packet is sent by netdev itself */
> >               /* NET_FILTER_DIRECTION_TX */
> > -            /* handle_primary_tcp_pkt */
> > +            if (!handle_primary_tcp_pkt(nf, conn, pkt)) {
> > +                qemu_net_queue_send(s->incoming_queue, sender, 0,
> > +                (const uint8_t *)pkt->data, pkt->size, NULL);
> > +                packet_destroy(pkt, NULL);
> > +                pkt = NULL;
> > +                /*
> > +                 * We block the packet here,after rewrite pkt
> > +                 * and will send it
> > +                 */
> > +                return 1;
> > +            }
> >           } else {
> >               /* NET_FILTER_DIRECTION_RX */
> > -            /* handle_secondary_tcp_pkt */
> > +            if (!handle_secondary_tcp_pkt(nf, conn, pkt)) {
> > +                qemu_net_queue_send(s->incoming_queue, sender, 0,
> > +                (const uint8_t *)pkt->data, pkt->size, NULL);
> > +                packet_destroy(pkt, NULL);
> > +                pkt = NULL;
> > +                /*
> > +                 * We block the packet here,after rewrite pkt
> > +                 * and will send it
> > +                 */
> > +                return 1;
> > +            }
> >           }
> >       }
> > diff --git a/trace-events b/trace-events
> > index 6686cdf..5d798c6 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -1927,3 +1927,6 @@ colo_compare_icmp_miscompare_mtu(const char *sta, int size) ": %s  %d"
> >   colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
> >   colo_old_packet_check_found(int64_t old_time) "%" PRId64
> >   colo_compare_miscompare(void) ""
> > +
> > +# net/filter-rewriter.c
> > +colo_filter_rewriter_debug(void) ""
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Zhang Chen]

On 06/20/2016 08:14 PM, Dr. David Alan Gilbert wrote:
> * Jason Wang (jasowang@redhat.com) wrote:
>>
>> On 2016年06月14日 19:15, Zhang Chen wrote:
>>> We will rewrite tcp packet secondary received and sent.
>> More verbose please. E.g which fields were rewrote and why.

OK.

>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> ---
>>>    net/filter-rewriter.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--
>>>    trace-events          |  3 ++
>>>    2 files changed, 95 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
>>> index 12f88c5..86a2f53 100644
>>> --- a/net/filter-rewriter.c
>>> +++ b/net/filter-rewriter.c
>>> @@ -21,6 +21,7 @@
>>>    #include "qemu/main-loop.h"
>>>    #include "qemu/iov.h"
>>>    #include "net/checksum.h"
>>> +#include "trace.h"
>>>    #define FILTER_COLO_REWRITER(obj) \
>>>        OBJECT_CHECK(RewriterState, (obj), TYPE_FILTER_REWRITER)
>>> @@ -64,6 +65,75 @@ static int is_tcp_packet(Packet *pkt)
>>>        }
>>>    }
>>> +static int handle_primary_tcp_pkt(NetFilterState *nf,
>>> +                                  Connection *conn,
>>> +                                  Packet *pkt)
>>> +{
>>> +    struct tcphdr *tcp_pkt;
>>> +
>>> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
>>> +
>>> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
>> Why not use tracepoints directly?
> Because trace can't cope with you having to do an allocation/free.
>
>>> +        char *sdebug, *ddebug;
>>> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
>>> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
>>> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
>>> +                "  flags=%x\n", __func__, sdebug, ddebug,
>>> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
>>> +                tcp_pkt->th_flags);
> However, this should use the trace_ call to write the result even if it's
> using trace_event_get_state to switch the whole block on/off.

I will fix it in next version.

>
>>> +        g_free(sdebug);
>>> +        g_free(ddebug);
>>> +    }
>>> +
>>> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK)) {
>>> +        /* save primary colo tcp packet seq */
>>> +        conn->primary_seq = ntohl(tcp_pkt->th_ack) - 1;
>> Looks like primary_seq will only be updated during handshake, I wonder how
>> this works.

OK.
We assume that colo guest is a tcp server.

Firstly, client start a tcp handshake. the packet's seq=client_seq,
ack=0,flag=SYN. COLO primary guest get this pkt and mirror(filter-mirror)
to secondary guest, secondary get it use filter-redirector.
Then,primary guest response 
pkt(seq=primary_seq,ack=client_seq+1,flag=ACK|SYN).
secondary guest response 
pkt(seq=secondary_seq,ack=client_seq+1,flag=ACK|SYN).
In here,we use filter-rewriter save the secondary_seq to it's tcp 
connection.
Finally handshake,client send 
pkt(seq=client_seq+1,ack=primary_seq+1,flag=ACK).
Here,filter-rewriter can get primary_seq, and rewrite ack from primary_seq+1
to secondary_seq+1, recalculate checksum. So the secondary tcp connection
kept good.

When we send/recv packet.
client send pkt(seq=client_seq+1+data_len,ack=primary_seq+1,flag=ACK|PSH).
filter-rewriter rewrite ack and send to secondary guest.
primary guest response 
pkt(seq=primary_seq+1,ack=client_seq+1+data_len,flag=ACK)
secondary guest response 
pkt(seq=secondary_seq+1,ack=client_seq+1+data_len,flag=ACK)
we rewrite secondary guest seq from secondary_seq+1 to primary_seq+1.
So tcp connection kept good.


> This code really needs commenting to make it see what's going on; each
> of these functions should say which way the packet is going (e.g.
>   'handle packets to the primary from the secondary') - there's a lot
> of packet flows going on and without the comments it's very hard to follow.

Thanks..I will add comments in next version.

>
> I think this could be because we're fixing up the sequence numbers on the
> secondary once we've received the first response from the primary, so it's
> only the first packet of each connection that the primary has to do this on -
> but hmm I'm not sure without some comments.

Yes,you are right.



>
> Dave
>
>>> +
>>> +        /* adjust tcp seq to make secondary guest handle it */
>>> +        tcp_pkt->th_ack = htonl(conn->secondary_seq + 1);
>> I'm not sure this can work for all cases. I believe we should also rewrite
>> seq here. And to me, a better approach is to track the offset of seq between
>> pri and sec during handshake and rewrite both ack and seq based on this
>> offset.

In the vast majority of cases, colo guest is a tcp server.
client kernel and guest kernel make the tcp seq work good.
we don't need rewrite seq here. we just need rewrite ack
and checksum can make secondary tcp connection work. If
colo guest is a tcp client,maybe we can wait colo-compare
do a checkpoint(secondary haven't send tcp packet in time).


Thanks
Zhang Chen


>>> +        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int handle_secondary_tcp_pkt(NetFilterState *nf,
>>> +                                    Connection *conn,
>>> +                                    Packet *pkt)
>>> +{
>>> +    struct tcphdr *tcp_pkt;
>>> +
>>> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
>>> +
>>> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
>>> +        char *sdebug, *ddebug;
>>> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
>>> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
>>> +        printf("handle_secondary_tcp_pkt conn->secondary_seq = %u,\n",
>>> +               conn->secondary_seq);
>>> +        printf("handle_secondary_tcp_pkt conn->primary_seq = %u,\n",
>>> +               conn->primary_seq);
>>> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
>>> +                "  flags=%x\n", __func__, sdebug, ddebug,
>>> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
>>> +                tcp_pkt->th_flags);
>>> +        g_free(sdebug);
>>> +        g_free(ddebug);
>>> +    }
>>> +
>>> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
>>> +        /* save client's seq */
>>> +        conn->secondary_seq = ntohl(tcp_pkt->th_seq);
>>> +    }
>>> +
>>> +    if ((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK) {
>>> +        tcp_pkt->th_seq = htonl(conn->primary_seq + 1);
>>> +        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>    static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
>>>                                             NetClientState *sender,
>>>                                             unsigned flags,
>>> @@ -106,10 +176,30 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
>>>            if (sender == nf->netdev) {
>>>                /* This packet is sent by netdev itself */
>>>                /* NET_FILTER_DIRECTION_TX */
>>> -            /* handle_primary_tcp_pkt */
>>> +            if (!handle_primary_tcp_pkt(nf, conn, pkt)) {
>>> +                qemu_net_queue_send(s->incoming_queue, sender, 0,
>>> +                (const uint8_t *)pkt->data, pkt->size, NULL);
>>> +                packet_destroy(pkt, NULL);
>>> +                pkt = NULL;
>>> +                /*
>>> +                 * We block the packet here,after rewrite pkt
>>> +                 * and will send it
>>> +                 */
>>> +                return 1;
>>> +            }
>>>            } else {
>>>                /* NET_FILTER_DIRECTION_RX */
>>> -            /* handle_secondary_tcp_pkt */
>>> +            if (!handle_secondary_tcp_pkt(nf, conn, pkt)) {
>>> +                qemu_net_queue_send(s->incoming_queue, sender, 0,
>>> +                (const uint8_t *)pkt->data, pkt->size, NULL);
>>> +                packet_destroy(pkt, NULL);
>>> +                pkt = NULL;
>>> +                /*
>>> +                 * We block the packet here,after rewrite pkt
>>> +                 * and will send it
>>> +                 */
>>> +                return 1;
>>> +            }
>>>            }
>>>        }
>>> diff --git a/trace-events b/trace-events
>>> index 6686cdf..5d798c6 100644
>>> --- a/trace-events
>>> +++ b/trace-events
>>> @@ -1927,3 +1927,6 @@ colo_compare_icmp_miscompare_mtu(const char *sta, int size) ": %s  %d"
>>>    colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
>>>    colo_old_packet_check_found(int64_t old_time) "%" PRId64
>>>    colo_compare_miscompare(void) ""
>>> +
>>> +# net/filter-rewriter.c
>>> +colo_filter_rewriter_debug(void) ""
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
> .
>

-- 
Thanks
zhangchen

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Jason Wang]

On 2016年06月22日 11:12, Zhang Chen wrote:
>
>
> On 06/20/2016 08:14 PM, Dr. David Alan Gilbert wrote:
>> * Jason Wang (jasowang@redhat.com) wrote:
>>>
>>> On 2016年06月14日 19:15, Zhang Chen wrote:
>>>> We will rewrite tcp packet secondary received and sent.
>>> More verbose please. E.g which fields were rewrote and why.
>
> OK.
>
>>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> ---
>>>>    net/filter-rewriter.c | 94 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>    trace-events          |  3 ++
>>>>    2 files changed, 95 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
>>>> index 12f88c5..86a2f53 100644
>>>> --- a/net/filter-rewriter.c
>>>> +++ b/net/filter-rewriter.c
>>>> @@ -21,6 +21,7 @@
>>>>    #include "qemu/main-loop.h"
>>>>    #include "qemu/iov.h"
>>>>    #include "net/checksum.h"
>>>> +#include "trace.h"
>>>>    #define FILTER_COLO_REWRITER(obj) \
>>>>        OBJECT_CHECK(RewriterState, (obj), TYPE_FILTER_REWRITER)
>>>> @@ -64,6 +65,75 @@ static int is_tcp_packet(Packet *pkt)
>>>>        }
>>>>    }
>>>> +static int handle_primary_tcp_pkt(NetFilterState *nf,
>>>> +                                  Connection *conn,
>>>> +                                  Packet *pkt)
>>>> +{
>>>> +    struct tcphdr *tcp_pkt;
>>>> +
>>>> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
>>>> +
>>>> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
>>> Why not use tracepoints directly?
>> Because trace can't cope with you having to do an allocation/free.
>>
>>>> +        char *sdebug, *ddebug;
>>>> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
>>>> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
>>>> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
>>>> +                "  flags=%x\n", __func__, sdebug, ddebug,
>>>> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
>>>> +                tcp_pkt->th_flags);
>> However, this should use the trace_ call to write the result even if 
>> it's
>> using trace_event_get_state to switch the whole block on/off.
>
> I will fix it in next version.
>
>>
>>>> +        g_free(sdebug);
>>>> +        g_free(ddebug);
>>>> +    }
>>>> +
>>>> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK)) {
>>>> +        /* save primary colo tcp packet seq */
>>>> +        conn->primary_seq = ntohl(tcp_pkt->th_ack) - 1;
>>> Looks like primary_seq will only be updated during handshake, I 
>>> wonder how
>>> this works.
>
> OK.
> We assume that colo guest is a tcp server.
>
> Firstly, client start a tcp handshake. the packet's seq=client_seq,
> ack=0,flag=SYN. COLO primary guest get this pkt and mirror(filter-mirror)
> to secondary guest, secondary get it use filter-redirector.
> Then,primary guest response 
> pkt(seq=primary_seq,ack=client_seq+1,flag=ACK|SYN).
> secondary guest response 
> pkt(seq=secondary_seq,ack=client_seq+1,flag=ACK|SYN).
> In here,we use filter-rewriter save the secondary_seq to it's tcp 
> connection.
> Finally handshake,client send 
> pkt(seq=client_seq+1,ack=primary_seq+1,flag=ACK).
> Here,filter-rewriter can get primary_seq, and rewrite ack from 
> primary_seq+1
> to secondary_seq+1, recalculate checksum. So the secondary tcp connection
> kept good.
>
> When we send/recv packet.
> client send 
> pkt(seq=client_seq+1+data_len,ack=primary_seq+1,flag=ACK|PSH).
> filter-rewriter rewrite ack and send to secondary guest.

If I read your code correctly, secondary_seq will only be updated during 
handshake. So the ack seq will always be same for each packet received 
by secondary?

> primary guest response 
> pkt(seq=primary_seq+1,ack=client_seq+1+data_len,flag=ACK)
> secondary guest response 
> pkt(seq=secondary_seq+1,ack=client_seq+1+data_len,flag=ACK)

Is ACK a must here?

> we rewrite secondary guest seq from secondary_seq+1 to primary_seq+1.
> So tcp connection kept good.

What if, consider we have a large window, so server(guest) want to send 
more than one TCP packets? The code can only advance primary_seq when 
we've received an ack which seems wrong.

So it will be very tricky if you don't track offset. Basically, what I 
suggest is rather simple:

1) calculate offset during handshake, e.g offset = secondary_seq_syn - 
primary_seq_syn
2) in handle_primary_tcp_pkt: tcp_pkt->th_ack += offset;
3) in handle_secondary_tcp_pkt: tcp_pkt->th_seq -= offset;

Looks like this can handle more cases and more robust than current code?

>
>
>> This code really needs commenting to make it see what's going on; each
>> of these functions should say which way the packet is going (e.g.
>>   'handle packets to the primary from the secondary') - there's a lot
>> of packet flows going on and without the comments it's very hard to 
>> follow.
>
> Thanks..I will add comments in next version.
>
>>
>> I think this could be because we're fixing up the sequence numbers on 
>> the
>> secondary once we've received the first response from the primary, so 
>> it's
>> only the first packet of each connection that the primary has to do 
>> this on -
>> but hmm I'm not sure without some comments.
>
> Yes,you are right.
>
>
>
>>
>> Dave
>>
>>>> +
>>>> +        /* adjust tcp seq to make secondary guest handle it */
>>>> +        tcp_pkt->th_ack = htonl(conn->secondary_seq + 1);
>>> I'm not sure this can work for all cases. I believe we should also 
>>> rewrite
>>> seq here. And to me, a better approach is to track the offset of seq 
>>> between
>>> pri and sec during handshake and rewrite both ack and seq based on this
>>> offset.
>
> In the vast majority of cases, colo guest is a tcp server.
> client kernel and guest kernel make the tcp seq work good.
> we don't need rewrite seq here. we just need rewrite ack
> and checksum can make secondary tcp connection work. If
> colo guest is a tcp client,maybe we can wait colo-compare
> do a checkpoint(secondary haven't send tcp packet in time).
>
>
> Thanks
> Zhang Chen
>
>
>>>> + net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int handle_secondary_tcp_pkt(NetFilterState *nf,
>>>> +                                    Connection *conn,
>>>> +                                    Packet *pkt)
>>>> +{
>>>> +    struct tcphdr *tcp_pkt;
>>>> +
>>>> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
>>>> +
>>>> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
>>>> +        char *sdebug, *ddebug;
>>>> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
>>>> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
>>>> +        printf("handle_secondary_tcp_pkt conn->secondary_seq = 
>>>> %u,\n",
>>>> +               conn->secondary_seq);
>>>> +        printf("handle_secondary_tcp_pkt conn->primary_seq = %u,\n",
>>>> +               conn->primary_seq);
>>>> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
>>>> +                "  flags=%x\n", __func__, sdebug, ddebug,
>>>> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
>>>> +                tcp_pkt->th_flags);
>>>> +        g_free(sdebug);
>>>> +        g_free(ddebug);
>>>> +    }
>>>> +
>>>> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | 
>>>> TH_SYN))) {
>>>> +        /* save client's seq */
>>>> +        conn->secondary_seq = ntohl(tcp_pkt->th_seq);
>>>> +    }
>>>> +
>>>> +    if ((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK) {
>>>> +        tcp_pkt->th_seq = htonl(conn->primary_seq + 1);
>>>> +        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>    static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
>>>>                                             NetClientState *sender,
>>>>                                             unsigned flags,
>>>> @@ -106,10 +176,30 @@ static ssize_t 
>>>> colo_rewriter_receive_iov(NetFilterState *nf,
>>>>            if (sender == nf->netdev) {
>>>>                /* This packet is sent by netdev itself */
>>>>                /* NET_FILTER_DIRECTION_TX */
>>>> -            /* handle_primary_tcp_pkt */
>>>> +            if (!handle_primary_tcp_pkt(nf, conn, pkt)) {
>>>> +                qemu_net_queue_send(s->incoming_queue, sender, 0,
>>>> +                (const uint8_t *)pkt->data, pkt->size, NULL);
>>>> +                packet_destroy(pkt, NULL);
>>>> +                pkt = NULL;
>>>> +                /*
>>>> +                 * We block the packet here,after rewrite pkt
>>>> +                 * and will send it
>>>> +                 */
>>>> +                return 1;
>>>> +            }
>>>>            } else {
>>>>                /* NET_FILTER_DIRECTION_RX */
>>>> -            /* handle_secondary_tcp_pkt */
>>>> +            if (!handle_secondary_tcp_pkt(nf, conn, pkt)) {
>>>> +                qemu_net_queue_send(s->incoming_queue, sender, 0,
>>>> +                (const uint8_t *)pkt->data, pkt->size, NULL);
>>>> +                packet_destroy(pkt, NULL);
>>>> +                pkt = NULL;
>>>> +                /*
>>>> +                 * We block the packet here,after rewrite pkt
>>>> +                 * and will send it
>>>> +                 */
>>>> +                return 1;
>>>> +            }
>>>>            }
>>>>        }
>>>> diff --git a/trace-events b/trace-events
>>>> index 6686cdf..5d798c6 100644
>>>> --- a/trace-events
>>>> +++ b/trace-events
>>>> @@ -1927,3 +1927,6 @@ colo_compare_icmp_miscompare_mtu(const char 
>>>> *sta, int size) ": %s  %d"
>>>>    colo_compare_ip_info(int psize, const char *sta, const char 
>>>> *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, 
>>>> ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
>>>>    colo_old_packet_check_found(int64_t old_time) "%" PRId64
>>>>    colo_compare_miscompare(void) ""
>>>> +
>>>> +# net/filter-rewriter.c
>>>> +colo_filter_rewriter_debug(void) ""
>> -- 
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
>>
>> .
>>
>

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Zhang Chen]

On 06/22/2016 02:34 PM, Jason Wang wrote:
>
>
> On 2016年06月22日 11:12, Zhang Chen wrote:
>>
>>
>> On 06/20/2016 08:14 PM, Dr. David Alan Gilbert wrote:
>>> * Jason Wang (jasowang@redhat.com) wrote:
>>>>
>>>> On 2016年06月14日 19:15, Zhang Chen wrote:
>>>>> We will rewrite tcp packet secondary received and sent.
>>>> More verbose please. E.g which fields were rewrote and why.
>>
>> OK.
>>
>>>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>> ---
>>>>>    net/filter-rewriter.c | 94 
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>    trace-events          |  3 ++
>>>>>    2 files changed, 95 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
>>>>> index 12f88c5..86a2f53 100644
>>>>> --- a/net/filter-rewriter.c
>>>>> +++ b/net/filter-rewriter.c
>>>>> @@ -21,6 +21,7 @@
>>>>>    #include "qemu/main-loop.h"
>>>>>    #include "qemu/iov.h"
>>>>>    #include "net/checksum.h"
>>>>> +#include "trace.h"
>>>>>    #define FILTER_COLO_REWRITER(obj) \
>>>>>        OBJECT_CHECK(RewriterState, (obj), TYPE_FILTER_REWRITER)
>>>>> @@ -64,6 +65,75 @@ static int is_tcp_packet(Packet *pkt)
>>>>>        }
>>>>>    }
>>>>> +static int handle_primary_tcp_pkt(NetFilterState *nf,
>>>>> +                                  Connection *conn,
>>>>> +                                  Packet *pkt)
>>>>> +{
>>>>> +    struct tcphdr *tcp_pkt;
>>>>> +
>>>>> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
>>>>> +
>>>>> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
>>>> Why not use tracepoints directly?
>>> Because trace can't cope with you having to do an allocation/free.
>>>
>>>>> +        char *sdebug, *ddebug;
>>>>> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
>>>>> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
>>>>> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
>>>>> +                "  flags=%x\n", __func__, sdebug, ddebug,
>>>>> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
>>>>> +                tcp_pkt->th_flags);
>>> However, this should use the trace_ call to write the result even if 
>>> it's
>>> using trace_event_get_state to switch the whole block on/off.
>>
>> I will fix it in next version.
>>
>>>
>>>>> +        g_free(sdebug);
>>>>> +        g_free(ddebug);
>>>>> +    }
>>>>> +
>>>>> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK)) {
>>>>> +        /* save primary colo tcp packet seq */
>>>>> +        conn->primary_seq = ntohl(tcp_pkt->th_ack) - 1;
>>>> Looks like primary_seq will only be updated during handshake, I 
>>>> wonder how
>>>> this works.
>>
>> OK.
>> We assume that colo guest is a tcp server.
>>
>> Firstly, client start a tcp handshake. the packet's seq=client_seq,
>> ack=0,flag=SYN. COLO primary guest get this pkt and 
>> mirror(filter-mirror)
>> to secondary guest, secondary get it use filter-redirector.
>> Then,primary guest response 
>> pkt(seq=primary_seq,ack=client_seq+1,flag=ACK|SYN).
>> secondary guest response 
>> pkt(seq=secondary_seq,ack=client_seq+1,flag=ACK|SYN).
>> In here,we use filter-rewriter save the secondary_seq to it's tcp 
>> connection.
>> Finally handshake,client send 
>> pkt(seq=client_seq+1,ack=primary_seq+1,flag=ACK).
>> Here,filter-rewriter can get primary_seq, and rewrite ack from 
>> primary_seq+1
>> to secondary_seq+1, recalculate checksum. So the secondary tcp 
>> connection
>> kept good.
>>
>> When we send/recv packet.
>> client send 
>> pkt(seq=client_seq+1+data_len,ack=primary_seq+1,flag=ACK|PSH).
>> filter-rewriter rewrite ack and send to secondary guest.
>
> If I read your code correctly, secondary_seq will only be updated 
> during handshake. So the ack seq will always be same for each packet 
> received by secondary?

Yes. I don't know why kernel do this. But　I dump the packet hex found that,
the ack packet flag=ACK means only ack enabled.and the seq will affect 
tcp checksum
make connection failed.


>
>> primary guest response 
>> pkt(seq=primary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>> secondary guest response 
>> pkt(seq=secondary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>
> Is ACK a must here?

Yes.

>
>> we rewrite secondary guest seq from secondary_seq+1 to primary_seq+1.
>> So tcp connection kept good.
>
> What if, consider we have a large window, so server(guest) want to 
> send more than one TCP packets? The code can only advance primary_seq 
> when we've received an ack which seems wrong.
>
> So it will be very tricky if you don't track offset. Basically, what I 
> suggest is rather simple:
>
> 1) calculate offset during handshake, e.g offset = secondary_seq_syn - 
> primary_seq_syn
> 2) in handle_primary_tcp_pkt: tcp_pkt->th_ack += offset;
> 3) in handle_secondary_tcp_pkt: tcp_pkt->th_seq -= offset;
>
> Looks like this can handle more cases and more robust than current code?

Make sense, I will change it in next version.

Thanks
Zhang Chen

>
>>
>>
>>> This code really needs commenting to make it see what's going on; each
>>> of these functions should say which way the packet is going (e.g.
>>>   'handle packets to the primary from the secondary') - there's a lot
>>> of packet flows going on and without the comments it's very hard to 
>>> follow.
>>
>> Thanks..I will add comments in next version.
>>
>>>
>>> I think this could be because we're fixing up the sequence numbers 
>>> on the
>>> secondary once we've received the first response from the primary, 
>>> so it's
>>> only the first packet of each connection that the primary has to do 
>>> this on -
>>> but hmm I'm not sure without some comments.
>>
>> Yes,you are right.
>>
>>
>>
>>>
>>> Dave
>>>
>>>>> +
>>>>> +        /* adjust tcp seq to make secondary guest handle it */
>>>>> +        tcp_pkt->th_ack = htonl(conn->secondary_seq + 1);
>>>> I'm not sure this can work for all cases. I believe we should also 
>>>> rewrite
>>>> seq here. And to me, a better approach is to track the offset of 
>>>> seq between
>>>> pri and sec during handshake and rewrite both ack and seq based on 
>>>> this
>>>> offset.
>>
>> In the vast majority of cases, colo guest is a tcp server.
>> client kernel and guest kernel make the tcp seq work good.
>> we don't need rewrite seq here. we just need rewrite ack
>> and checksum can make secondary tcp connection work. If
>> colo guest is a tcp client,maybe we can wait colo-compare
>> do a checkpoint(secondary haven't send tcp packet in time).
>>
>>
>> Thanks
>> Zhang Chen
>>
>>
>>>>> + net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int handle_secondary_tcp_pkt(NetFilterState *nf,
>>>>> +                                    Connection *conn,
>>>>> +                                    Packet *pkt)
>>>>> +{
>>>>> +    struct tcphdr *tcp_pkt;
>>>>> +
>>>>> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
>>>>> +
>>>>> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
>>>>> +        char *sdebug, *ddebug;
>>>>> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
>>>>> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
>>>>> +        printf("handle_secondary_tcp_pkt conn->secondary_seq = 
>>>>> %u,\n",
>>>>> +               conn->secondary_seq);
>>>>> +        printf("handle_secondary_tcp_pkt conn->primary_seq = %u,\n",
>>>>> +               conn->primary_seq);
>>>>> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
>>>>> +                "  flags=%x\n", __func__, sdebug, ddebug,
>>>>> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
>>>>> +                tcp_pkt->th_flags);
>>>>> +        g_free(sdebug);
>>>>> +        g_free(ddebug);
>>>>> +    }
>>>>> +
>>>>> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | 
>>>>> TH_SYN))) {
>>>>> +        /* save client's seq */
>>>>> +        conn->secondary_seq = ntohl(tcp_pkt->th_seq);
>>>>> +    }
>>>>> +
>>>>> +    if ((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK) {
>>>>> +        tcp_pkt->th_seq = htonl(conn->primary_seq + 1);
>>>>> +        net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>    static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
>>>>>                                             NetClientState *sender,
>>>>>                                             unsigned flags,
>>>>> @@ -106,10 +176,30 @@ static ssize_t 
>>>>> colo_rewriter_receive_iov(NetFilterState *nf,
>>>>>            if (sender == nf->netdev) {
>>>>>                /* This packet is sent by netdev itself */
>>>>>                /* NET_FILTER_DIRECTION_TX */
>>>>> -            /* handle_primary_tcp_pkt */
>>>>> +            if (!handle_primary_tcp_pkt(nf, conn, pkt)) {
>>>>> +                qemu_net_queue_send(s->incoming_queue, sender, 0,
>>>>> +                (const uint8_t *)pkt->data, pkt->size, NULL);
>>>>> +                packet_destroy(pkt, NULL);
>>>>> +                pkt = NULL;
>>>>> +                /*
>>>>> +                 * We block the packet here,after rewrite pkt
>>>>> +                 * and will send it
>>>>> +                 */
>>>>> +                return 1;
>>>>> +            }
>>>>>            } else {
>>>>>                /* NET_FILTER_DIRECTION_RX */
>>>>> -            /* handle_secondary_tcp_pkt */
>>>>> +            if (!handle_secondary_tcp_pkt(nf, conn, pkt)) {
>>>>> +                qemu_net_queue_send(s->incoming_queue, sender, 0,
>>>>> +                (const uint8_t *)pkt->data, pkt->size, NULL);
>>>>> +                packet_destroy(pkt, NULL);
>>>>> +                pkt = NULL;
>>>>> +                /*
>>>>> +                 * We block the packet here,after rewrite pkt
>>>>> +                 * and will send it
>>>>> +                 */
>>>>> +                return 1;
>>>>> +            }
>>>>>            }
>>>>>        }
>>>>> diff --git a/trace-events b/trace-events
>>>>> index 6686cdf..5d798c6 100644
>>>>> --- a/trace-events
>>>>> +++ b/trace-events
>>>>> @@ -1927,3 +1927,6 @@ colo_compare_icmp_miscompare_mtu(const char 
>>>>> *sta, int size) ": %s  %d"
>>>>>    colo_compare_ip_info(int psize, const char *sta, const char 
>>>>> *stb, int ssize, const char *stc, const char *std) "ppkt size = 
>>>>> %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst 
>>>>> = %s"
>>>>>    colo_old_packet_check_found(int64_t old_time) "%" PRId64
>>>>>    colo_compare_miscompare(void) ""
>>>>> +
>>>>> +# net/filter-rewriter.c
>>>>> +colo_filter_rewriter_debug(void) ""
>>> -- 
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>>>
>>> .
>>>
>>
>
>
>
> .
>

-- 
Thanks
zhangchen

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Jason Wang]

On 2016年06月23日 18:48, Zhang Chen wrote:
>
>
> On 06/22/2016 02:34 PM, Jason Wang wrote:
>>
>>
>> On 2016年06月22日 11:12, Zhang Chen wrote:
>>>
>>>
>>> On 06/20/2016 08:14 PM, Dr. David Alan Gilbert wrote:
>>>> * Jason Wang (jasowang@redhat.com) wrote:
>>>>>
>>>>> On 2016年06月14日 19:15, Zhang Chen wrote:
>>>>>> We will rewrite tcp packet secondary received and sent.
>>>>> More verbose please. E.g which fields were rewrote and why.
>>>
>>> OK.
>>>
>>>>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>>>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>>> ---
>>>>>>    net/filter-rewriter.c | 94 
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>>    trace-events          |  3 ++
>>>>>>    2 files changed, 95 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
>>>>>> index 12f88c5..86a2f53 100644
>>>>>> --- a/net/filter-rewriter.c
>>>>>> +++ b/net/filter-rewriter.c
>>>>>> @@ -21,6 +21,7 @@
>>>>>>    #include "qemu/main-loop.h"
>>>>>>    #include "qemu/iov.h"
>>>>>>    #include "net/checksum.h"
>>>>>> +#include "trace.h"
>>>>>>    #define FILTER_COLO_REWRITER(obj) \
>>>>>>        OBJECT_CHECK(RewriterState, (obj), TYPE_FILTER_REWRITER)
>>>>>> @@ -64,6 +65,75 @@ static int is_tcp_packet(Packet *pkt)
>>>>>>        }
>>>>>>    }
>>>>>> +static int handle_primary_tcp_pkt(NetFilterState *nf,
>>>>>> +                                  Connection *conn,
>>>>>> +                                  Packet *pkt)
>>>>>> +{
>>>>>> +    struct tcphdr *tcp_pkt;
>>>>>> +
>>>>>> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
>>>>>> +
>>>>>> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
>>>>> Why not use tracepoints directly?
>>>> Because trace can't cope with you having to do an allocation/free.
>>>>
>>>>>> + char *sdebug, *ddebug;
>>>>>> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
>>>>>> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
>>>>>> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
>>>>>> +                "  flags=%x\n", __func__, sdebug, ddebug,
>>>>>> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
>>>>>> +                tcp_pkt->th_flags);
>>>> However, this should use the trace_ call to write the result even 
>>>> if it's
>>>> using trace_event_get_state to switch the whole block on/off.
>>>
>>> I will fix it in next version.
>>>
>>>>
>>>>>> + g_free(sdebug);
>>>>>> +        g_free(ddebug);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK)) {
>>>>>> +        /* save primary colo tcp packet seq */
>>>>>> +        conn->primary_seq = ntohl(tcp_pkt->th_ack) - 1;
>>>>> Looks like primary_seq will only be updated during handshake, I 
>>>>> wonder how
>>>>> this works.
>>>
>>> OK.
>>> We assume that colo guest is a tcp server.
>>>
>>> Firstly, client start a tcp handshake. the packet's seq=client_seq,
>>> ack=0,flag=SYN. COLO primary guest get this pkt and 
>>> mirror(filter-mirror)
>>> to secondary guest, secondary get it use filter-redirector.
>>> Then,primary guest response 
>>> pkt(seq=primary_seq,ack=client_seq+1,flag=ACK|SYN).
>>> secondary guest response 
>>> pkt(seq=secondary_seq,ack=client_seq+1,flag=ACK|SYN).
>>> In here,we use filter-rewriter save the secondary_seq to it's tcp 
>>> connection.
>>> Finally handshake,client send 
>>> pkt(seq=client_seq+1,ack=primary_seq+1,flag=ACK).
>>> Here,filter-rewriter can get primary_seq, and rewrite ack from 
>>> primary_seq+1
>>> to secondary_seq+1, recalculate checksum. So the secondary tcp 
>>> connection
>>> kept good.
>>>
>>> When we send/recv packet.
>>> client send 
>>> pkt(seq=client_seq+1+data_len,ack=primary_seq+1,flag=ACK|PSH).
>>> filter-rewriter rewrite ack and send to secondary guest.
>>
>> If I read your code correctly, secondary_seq will only be updated 
>> during handshake. So the ack seq will always be same for each packet 
>> received by secondary?
>
> Yes. I don't know why kernel do this. But　I dump the packet hex found 
> that,
> the ack packet flag=ACK means only ack enabled.and the seq will affect 
> tcp checksum
> make connection failed.
>

Not sure I get your meaning, but basically the code here should not have 
any assumptions on guest behaviors.

>
>>
>>> primary guest response 
>>> pkt(seq=primary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>> secondary guest response 
>>> pkt(seq=secondary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>
>> Is ACK a must here?
>
> Yes.
>

Looks not, e.g what happens if guest does not use piggybacking acks?

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Zhang Chen]

On 06/24/2016 02:08 PM, Jason Wang wrote:
>
>
> On 2016年06月23日 18:48, Zhang Chen wrote:
>>
>>
>> On 06/22/2016 02:34 PM, Jason Wang wrote:
>>>
>>>
>>> On 2016年06月22日 11:12, Zhang Chen wrote:
>>>>
>>>>
>>>> On 06/20/2016 08:14 PM, Dr. David Alan Gilbert wrote:
>>>>> * Jason Wang (jasowang@redhat.com) wrote:
>>>>>>
>>>>>> On 2016年06月14日 19:15, Zhang Chen wrote:
>>>>>>> We will rewrite tcp packet secondary received and sent.
>>>>>> More verbose please. E.g which fields were rewrote and why.
>>>>
>>>> OK.
>>>>
>>>>>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>>>>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>>>> ---
>>>>>>>    net/filter-rewriter.c | 94 
>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>>>    trace-events          |  3 ++
>>>>>>>    2 files changed, 95 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
>>>>>>> index 12f88c5..86a2f53 100644
>>>>>>> --- a/net/filter-rewriter.c
>>>>>>> +++ b/net/filter-rewriter.c
>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>    #include "qemu/main-loop.h"
>>>>>>>    #include "qemu/iov.h"
>>>>>>>    #include "net/checksum.h"
>>>>>>> +#include "trace.h"
>>>>>>>    #define FILTER_COLO_REWRITER(obj) \
>>>>>>>        OBJECT_CHECK(RewriterState, (obj), TYPE_FILTER_REWRITER)
>>>>>>> @@ -64,6 +65,75 @@ static int is_tcp_packet(Packet *pkt)
>>>>>>>        }
>>>>>>>    }
>>>>>>> +static int handle_primary_tcp_pkt(NetFilterState *nf,
>>>>>>> +                                  Connection *conn,
>>>>>>> +                                  Packet *pkt)
>>>>>>> +{
>>>>>>> +    struct tcphdr *tcp_pkt;
>>>>>>> +
>>>>>>> +    tcp_pkt = (struct tcphdr *)pkt->transport_layer;
>>>>>>> +
>>>>>>> +    if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
>>>>>> Why not use tracepoints directly?
>>>>> Because trace can't cope with you having to do an allocation/free.
>>>>>
>>>>>>> + char *sdebug, *ddebug;
>>>>>>> +        sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
>>>>>>> +        ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
>>>>>>> +        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
>>>>>>> +                "  flags=%x\n", __func__, sdebug, ddebug,
>>>>>>> +                ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
>>>>>>> +                tcp_pkt->th_flags);
>>>>> However, this should use the trace_ call to write the result even 
>>>>> if it's
>>>>> using trace_event_get_state to switch the whole block on/off.
>>>>
>>>> I will fix it in next version.
>>>>
>>>>>
>>>>>>> + g_free(sdebug);
>>>>>>> +        g_free(ddebug);
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK)) {
>>>>>>> +        /* save primary colo tcp packet seq */
>>>>>>> +        conn->primary_seq = ntohl(tcp_pkt->th_ack) - 1;
>>>>>> Looks like primary_seq will only be updated during handshake, I 
>>>>>> wonder how
>>>>>> this works.
>>>>
>>>> OK.
>>>> We assume that colo guest is a tcp server.
>>>>
>>>> Firstly, client start a tcp handshake. the packet's seq=client_seq,
>>>> ack=0,flag=SYN. COLO primary guest get this pkt and 
>>>> mirror(filter-mirror)
>>>> to secondary guest, secondary get it use filter-redirector.
>>>> Then,primary guest response 
>>>> pkt(seq=primary_seq,ack=client_seq+1,flag=ACK|SYN).
>>>> secondary guest response 
>>>> pkt(seq=secondary_seq,ack=client_seq+1,flag=ACK|SYN).
>>>> In here,we use filter-rewriter save the secondary_seq to it's tcp 
>>>> connection.
>>>> Finally handshake,client send 
>>>> pkt(seq=client_seq+1,ack=primary_seq+1,flag=ACK).
>>>> Here,filter-rewriter can get primary_seq, and rewrite ack from 
>>>> primary_seq+1
>>>> to secondary_seq+1, recalculate checksum. So the secondary tcp 
>>>> connection
>>>> kept good.
>>>>
>>>> When we send/recv packet.
>>>> client send 
>>>> pkt(seq=client_seq+1+data_len,ack=primary_seq+1,flag=ACK|PSH).
>>>> filter-rewriter rewrite ack and send to secondary guest.
>>>
>>> If I read your code correctly, secondary_seq will only be updated 
>>> during handshake. So the ack seq will always be same for each packet 
>>> received by secondary?
>>
>> Yes. I don't know why kernel do this. But　I dump the packet hex 
>> found that,
>> the ack packet flag=ACK means only ack enabled.and the seq will 
>> affect tcp checksum
>> make connection failed.
>>
>
> Not sure I get your meaning, but basically the code here should not 
> have any assumptions on guest behaviors.

Yes. I get your point.

>
>>
>>>
>>>> primary guest response 
>>>> pkt(seq=primary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>>> secondary guest response 
>>>> pkt(seq=secondary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>>
>>> Is ACK a must here?
>>
>> Yes.
>>
>
> Looks not, e.g what happens if guest does not use piggybacking acks?
>
>

If guest does not use piggybacking acks, it will send a independent 
packet for ack.
we will get this packet.
like:
pkt(seq=xxxx,ack=xxx,flag=ACK).


Thanks
Zhang Chen

> .
>

-- 
Thanks
zhangchen

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Jason Wang]

On 2016年06月28日 14:33, Zhang Chen wrote:
>>>
>>>>
>>>>> primary guest response 
>>>>> pkt(seq=primary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>>>> secondary guest response 
>>>>> pkt(seq=secondary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>>>
>>>> Is ACK a must here?
>>>
>>> Yes.
>>>
>>
>> Looks not, e.g what happens if guest does not use piggybacking acks?
>>
>>
>
> If guest does not use piggybacking acks, it will send a independent 
> packet for ack.
> we will get this packet.
> like:
> pkt(seq=xxxx,ack=xxx,flag=ACK). 

Right, so looks like if guest want to send some data too, it can send 
tcp packet without ACK set?

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Zhang Chen]

On 06/29/2016 09:55 AM, Jason Wang wrote:
>
>
> On 2016年06月28日 14:33, Zhang Chen wrote:
>>>>
>>>>>
>>>>>> primary guest response 
>>>>>> pkt(seq=primary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>>>>> secondary guest response 
>>>>>> pkt(seq=secondary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>>>>
>>>>> Is ACK a must here?
>>>>
>>>> Yes.
>>>>
>>>
>>> Looks not, e.g what happens if guest does not use piggybacking acks?
>>>
>>>
>>
>> If guest does not use piggybacking acks, it will send a independent 
>> packet for ack.
>> we will get this packet.
>> like:
>> pkt(seq=xxxx,ack=xxx,flag=ACK). 
>
> Right, so looks like if guest want to send some data too, it can send 
> tcp packet without ACK set?

NO, I tried it. the tcp packet always has ACK set except the SYN packet 
and FIN packet.
you can dump the packet to see it.

Thanks
Zhang Chen

>
>
>

-- 
Thanks
zhangchen

Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection

[Jason Wang]

On 2016年06月29日 14:13, Zhang Chen wrote:
>
>
> On 06/29/2016 09:55 AM, Jason Wang wrote:
>>
>>
>> On 2016年06月28日 14:33, Zhang Chen wrote:
>>>>>
>>>>>>
>>>>>>> primary guest response 
>>>>>>> pkt(seq=primary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>>>>>> secondary guest response 
>>>>>>> pkt(seq=secondary_seq+1,ack=client_seq+1+data_len,flag=ACK)
>>>>>>
>>>>>> Is ACK a must here?
>>>>>
>>>>> Yes.
>>>>>
>>>>
>>>> Looks not, e.g what happens if guest does not use piggybacking acks?
>>>>
>>>>
>>>
>>> If guest does not use piggybacking acks, it will send a independent 
>>> packet for ack.
>>> we will get this packet.
>>> like:
>>> pkt(seq=xxxx,ack=xxx,flag=ACK). 
>>
>> Right, so looks like if guest want to send some data too, it can send 
>> tcp packet without ACK set?
>
> NO, I tried it. the tcp packet always has ACK set except the SYN 
> packet and FIN packet.
> you can dump the packet to see it.
>
> Thanks
> Zhang Chen

Right, RFC said:

"   If the ACK control bit is set this field contains the value of the
     next sequence number the sender of the segment is expecting to
     receive.  Once a connection is established this is always sent.
"