Re: [Qemu-devel] [PULL V2 01/26] filter-rewriter: Add TCP state machine and fix memory leak in connection_track_table
On Tue, Oct 30, 2018 at 10:02 AM Jason Wang wrote:
>
> On 2018/10/29 下午7:01, Peter Maydell wrote:
> > On 19 October 2018 at 04:22, Jason Wang wrote:
> >> From: Zhang Chen
> >>
> >> We add almost full TCP state machine in filter-rewriter, except
> >> TCPS_LISTEN and some simplify in VM active close FIN states.
> >> The reason for this simplify job is because guest kernel will track
> >> the TCP status and wait 2MSL time too, if client resend the FIN packet,
> >> guest will resend the last ACK, so we needn't wait 2MSL time in
> filter-rewriter.
> >>
> >> After a net connection is closed, we didn't clear its related resources
> >> in connection_track_table, which will lead to memory leak.
> >>
> >> Let's track the state of net connection, if it is closed, its related
> >> resources will be cleared up.
> > Hi. Coverity (CID 1396477) points out that here:
> >
> >> +/*
> >> + * Active close step 2.
> >> + */
> >> +if (conn->tcp_state == TCPS_FIN_WAIT_1) {
> >> +conn->tcp_state = TCPS_TIME_WAIT;
> > ...this assignment to conn->tcp_state has no effect, because...
> >
> >> +/*
> >> + * For simplify implementation, we needn't wait 2MSL time
> >> + * in filter rewriter. Because guest kernel will track the
> >> + * TCP status and wait 2MSL time, if client resend the FIN
> >> + * packet, guest will apply the last ACK too.
> >> + */
> >> +conn->tcp_state = TCPS_CLOSED;
> > ...we immediately overwrite it with a different value.
> >
> >> +g_hash_table_remove(rf->connection_track_table, key);
> >> +}
> >> }
> > What was the intention of the code here?
> >
> > thanks
> > -- PMM
>
>
> Looks not.
>
> Chen, can you please send a patch to fix this?
>
>
Sure, I will send a patch for this issue.
Thanks
Zhang Chen
> Thanks
>
>
Re: [Qemu-devel] [PULL V2 01/26] filter-rewriter: Add TCP state machine and fix memory leak in connection_track_table
On 2018/10/29 下午7:01, Peter Maydell wrote:
On 19 October 2018 at 04:22, Jason Wang wrote:
From: Zhang Chen
We add almost full TCP state machine in filter-rewriter, except
TCPS_LISTEN and some simplify in VM active close FIN states.
The reason for this simplify job is because guest kernel will track
the TCP status and wait 2MSL time too, if client resend the FIN packet,
guest will resend the last ACK, so we needn't wait 2MSL time in filter-rewriter.
After a net connection is closed, we didn't clear its related resources
in connection_track_table, which will lead to memory leak.
Let's track the state of net connection, if it is closed, its related
resources will be cleared up.
Hi. Coverity (CID 1396477) points out that here:
+/*
+ * Active close step 2.
+ */
+if (conn->tcp_state == TCPS_FIN_WAIT_1) {
+conn->tcp_state = TCPS_TIME_WAIT;
...this assignment to conn->tcp_state has no effect, because...
+/*
+ * For simplify implementation, we needn't wait 2MSL time
+ * in filter rewriter. Because guest kernel will track the
+ * TCP status and wait 2MSL time, if client resend the FIN
+ * packet, guest will apply the last ACK too.
+ */
+conn->tcp_state = TCPS_CLOSED;
...we immediately overwrite it with a different value.
+g_hash_table_remove(rf->connection_track_table, key);
+}
}
What was the intention of the code here?
thanks
-- PMM
Looks not.
Chen, can you please send a patch to fix this?
Thanks
Re: [Qemu-devel] [PULL V2 01/26] filter-rewriter: Add TCP state machine and fix memory leak in connection_track_table
On 19 October 2018 at 04:22, Jason Wang wrote:
> From: Zhang Chen
>
> We add almost full TCP state machine in filter-rewriter, except
> TCPS_LISTEN and some simplify in VM active close FIN states.
> The reason for this simplify job is because guest kernel will track
> the TCP status and wait 2MSL time too, if client resend the FIN packet,
> guest will resend the last ACK, so we needn't wait 2MSL time in
> filter-rewriter.
>
> After a net connection is closed, we didn't clear its related resources
> in connection_track_table, which will lead to memory leak.
>
> Let's track the state of net connection, if it is closed, its related
> resources will be cleared up.
Hi. Coverity (CID 1396477) points out that here:
> +/*
> + * Active close step 2.
> + */
> +if (conn->tcp_state == TCPS_FIN_WAIT_1) {
> +conn->tcp_state = TCPS_TIME_WAIT;
...this assignment to conn->tcp_state has no effect, because...
> +/*
> + * For simplify implementation, we needn't wait 2MSL time
> + * in filter rewriter. Because guest kernel will track the
> + * TCP status and wait 2MSL time, if client resend the FIN
> + * packet, guest will apply the last ACK too.
> + */
> +conn->tcp_state = TCPS_CLOSED;
...we immediately overwrite it with a different value.
> +g_hash_table_remove(rf->connection_track_table, key);
> +}
> }
What was the intention of the code here?
thanks
-- PMM
[Qemu-devel] [PULL V2 01/26] filter-rewriter: Add TCP state machine and fix memory leak in connection_track_table
From: Zhang Chen
We add almost full TCP state machine in filter-rewriter, except
TCPS_LISTEN and some simplify in VM active close FIN states.
The reason for this simplify job is because guest kernel will track
the TCP status and wait 2MSL time too, if client resend the FIN packet,
guest will resend the last ACK, so we needn't wait 2MSL time in filter-rewriter.
After a net connection is closed, we didn't clear its related resources
in connection_track_table, which will lead to memory leak.
Let's track the state of net connection, if it is closed, its related
resources will be cleared up.
Signed-off-by: zhanghailiang
Signed-off-by: Zhang Chen
Signed-off-by: Zhang Chen
Signed-off-by: Jason Wang
---
net/colo.c| 2 +-
net/colo.h| 9 ++---
net/filter-rewriter.c | 109 +-
3 files changed, 104 insertions(+), 16 deletions(-)
diff --git a/net/colo.c b/net/colo.c
index 6dda4ed..97c8fc9 100644
--- a/net/colo.c
+++ b/net/colo.c
@@ -137,7 +137,7 @@ Connection *connection_new(ConnectionKey *key)
conn->ip_proto = key->ip_proto;
conn->processing = false;
conn->offset = 0;
-conn->syn_flag = 0;
+conn->tcp_state = TCPS_CLOSED;
conn->pack = 0;
conn->sack = 0;
g_queue_init(>primary_list);
diff --git a/net/colo.h b/net/colo.h
index da6c36d..0277e0e 100644
--- a/net/colo.h
+++ b/net/colo.h
@@ -18,6 +18,7 @@
#include "slirp/slirp.h"
#include "qemu/jhash.h"
#include "qemu/timer.h"
+#include "slirp/tcp.h"
#define HASHTABLE_MAX_SIZE 16384
@@ -81,11 +82,9 @@ typedef struct Connection {
uint32_t sack;
/* offset = secondary_seq - primary_seq */
tcp_seq offset;
-/*
- * we use this flag update offset func
- * run once in independent tcp connection
- */
-int syn_flag;
+
+int tcp_state; /* TCP FSM state */
+tcp_seq fin_ack_seq; /* the seq of 'fin=1,ack=1' */
} Connection;
uint32_t connection_key_hash(const void *opaque);
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index f584e4e..dd323fa 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -59,9 +59,9 @@ static int is_tcp_packet(Packet *pkt)
}
/* handle tcp packet from primary guest */
-static int handle_primary_tcp_pkt(NetFilterState *nf,
+static int handle_primary_tcp_pkt(RewriterState *rf,
Connection *conn,
- Packet *pkt)
+ Packet *pkt, ConnectionKey *key)
{
struct tcphdr *tcp_pkt;
@@ -74,23 +74,28 @@ static int handle_primary_tcp_pkt(NetFilterState *nf,
trace_colo_filter_rewriter_conn_offset(conn->offset);
}
+if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN)) &&
+conn->tcp_state == TCPS_SYN_SENT) {
+conn->tcp_state = TCPS_ESTABLISHED;
+}
+
if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_SYN)) {
/*
* we use this flag update offset func
* run once in independent tcp connection
*/
-conn->syn_flag = 1;
+conn->tcp_state = TCPS_SYN_RECEIVED;
}
if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK)) {
-if (conn->syn_flag) {
+if (conn->tcp_state == TCPS_SYN_RECEIVED) {
/*
* offset = secondary_seq - primary seq
* ack packet sent by guest from primary node,
* so we use th_ack - 1 get primary_seq
*/
conn->offset -= (ntohl(tcp_pkt->th_ack) - 1);
-conn->syn_flag = 0;
+conn->tcp_state = TCPS_ESTABLISHED;
}
if (conn->offset) {
/* handle packets to the secondary from the primary */
@@ -99,15 +104,66 @@ static int handle_primary_tcp_pkt(NetFilterState *nf,
net_checksum_calculate((uint8_t *)pkt->data + pkt->vnet_hdr_len,
pkt->size - pkt->vnet_hdr_len);
}
+
+/*
+ * Passive close step 3
+ */
+if ((conn->tcp_state == TCPS_LAST_ACK) &&
+(ntohl(tcp_pkt->th_ack) == (conn->fin_ack_seq + 1))) {
+conn->tcp_state = TCPS_CLOSED;
+g_hash_table_remove(rf->connection_track_table, key);
+}
+}
+
+if ((tcp_pkt->th_flags & TH_FIN) == TH_FIN) {
+/*
+ * Passive close.
+ * Step 1:
+ * The *server* side of this connect is VM, *client* tries to close
+ * the connection. We will into CLOSE_WAIT status.
+ *
+ * Step 2:
+ * In this step we will into LAST_ACK status.
+ *
+ * We got 'fin=1, ack=1' packet from server side, we need to
+ * record the seq of 'fin=1, ack=1' packet.
+ *
+ * Step 3:
+ * We got 'ack=1' packets from client side, it acks 'fin=1, ack=1'
+ * packet from server side. From this point, we can ensure that there
+ * will be no packets in the connection,
