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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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