qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Jason Wang
Subject: Re: [Qemu-devel] [RFC PATCH 3/3] filter-rewriter: rewrite tcp packet to keep secondary connection
Date: Wed, 22 Jun 2016 14:34:28 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0



On 2016年06月22日 11:12, Zhang Chen wrote:


On 06/20/2016 08:14 PM, Dr. David Alan Gilbert wrote:
* Jason Wang (address@hidden) 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 <address@hidden>
Signed-off-by: Li Zhijian <address@hidden>
Signed-off-by: Wen Congyang <address@hidden>
---
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 / address@hidden / Manchester, UK


.






reply via email to

[Prev in Thread] Current Thread [Next in Thread]