On 2017年02月22日 11:46, zhanghailiang wrote:
After a net connection is closed, we didn't clear its releated resources
in connection_track_table, which will lead to memory leak.
Not a real leak but would lead reset of hash table if too many closed
connections.
Let't track the state of net connection, if it is closed, its related
resources will be cleared up.
The issue is the state were tracked partially, do we need a full state
machine here?
Signed-off-by: zhanghailiang <zhang.zhanghaili...@huawei.com>
---
net/colo.h | 4 +++
net/filter-rewriter.c | 70 +++++++++++++++++++++++++++++++++++++++++++++------
2 files changed, 67 insertions(+), 7 deletions(-)
diff --git a/net/colo.h b/net/colo.h
index 7c524f3..cd9027f 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
@@ -69,6 +70,9 @@ typedef struct Connection {
* 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 c4ab91c..7e7ec35 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -60,9 +60,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;
@@ -97,15 +97,45 @@ static int handle_primary_tcp_pkt(NetFilterState *nf,
tcp_pkt->th_ack = htonl(ntohl(tcp_pkt->th_ack) + conn->offset);
net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ /*
+ * Case 1:
+ * The *server* side of this connect is VM, *client* tries to close
+ * the connection.
+ *
+ * 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, except that, some errors
+ * happen between the path of 'filter object' and vNIC, if this rare
+ * case really happen, we can still create a new connection,
+ * So it is safe to remove the connection from connection_track_table.
+ *
+ */
+ if ((conn->tcp_state == TCPS_LAST_ACK) &&
+ (ntohl(tcp_pkt->th_ack) == (conn->fin_ack_seq + 1))) {
+ fprintf(stderr, "Remove conn "
Can this even compile?
+ g_hash_table_remove(rf->connection_track_table, key);
+ }
+ }
+ /*
+ * Case 2:
+ * The *server* side of this connect is VM, *server* tries to close
+ * the connection.
+ *
+ * We got 'fin=1, ack=1' packet from client side, we need to
+ * record the seq of 'fin=1, ack=1' packet.
+ */
+ if ((tcp_pkt->th_flags & (TH_ACK | TH_FIN)) == (TH_ACK | TH_FIN)) {
+ conn->fin_ack_seq = htonl(tcp_pkt->th_seq);
+ conn->tcp_state = TCPS_LAST_ACK;
}
return 0;
}
/* handle tcp packet from secondary guest */
-static int handle_secondary_tcp_pkt(NetFilterState *nf,
+static int handle_secondary_tcp_pkt(RewriterState *rf,
Connection *conn,
- Packet *pkt)
+ Packet *pkt, ConnectionKey *key)
{
struct tcphdr *tcp_pkt;
@@ -133,8 +163,34 @@ static int handle_secondary_tcp_pkt(NetFilterState *nf,
tcp_pkt->th_seq = htonl(ntohl(tcp_pkt->th_seq) - conn->offset);
net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ /*
+ * Case 2:
+ * The *server* side of this connect is VM, *server* tries to close
+ * the connection.
+ *
+ * We got 'ack=1' packets from server side, it acks 'fin=1, ack=1'
+ * packet from client side. Like Case 1, there should be no packets
+ * in the connection from now know, But the difference here is
+ * if the packet is lost, We will get the resent 'fin=1,ack=1' packet.
+ * TODO: Fix above case.
+ */
+ if ((conn->tcp_state == TCPS_LAST_ACK) &&
+ (ntohl(tcp_pkt->th_ack) == (conn->fin_ack_seq + 1))) {
+ g_hash_table_remove(rf->connection_track_table, key);
+ }
+ }
+ /*
+ * Case 1:
+ * The *server* side of this connect is VM, *client* tries to close
+ * the connection.
+ *
+ * We got 'fin=1, ack=1' packet from server side, we need to
+ * record the seq of 'fin=1, ack=1' packet.
+ */
+ if ((tcp_pkt->th_flags & (TH_ACK | TH_FIN)) == (TH_ACK | TH_FIN)) {
+ conn->fin_ack_seq = ntohl(tcp_pkt->th_seq);
+ conn->tcp_state = TCPS_LAST_ACK;
I thought the tcp_state should store the state of TCP from the view of
secondary VM? So TCPS_LAST_ACK is wrong and bring lots of confusion. And
the handle of active close needs more states here. E.g if connection is
in FIN_WAIT_2, the connection is only half closed, remote peer can still
send packet to us unless we receive a FIN.
Thanks
}
-
return 0;
}
@@ -178,7 +234,7 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
if (sender == nf->netdev) {
/* NET_FILTER_DIRECTION_TX */
- if (!handle_primary_tcp_pkt(nf, conn, pkt)) {
+ if (!handle_primary_tcp_pkt(s, conn, pkt, &key)) {
qemu_net_queue_send(s->incoming_queue, sender, 0,
(const uint8_t *)pkt->data, pkt->size, NULL);
packet_destroy(pkt, NULL);
@@ -191,7 +247,7 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
}
} else {
/* NET_FILTER_DIRECTION_RX */
- if (!handle_secondary_tcp_pkt(nf, conn, pkt)) {
+ if (!handle_secondary_tcp_pkt(s, conn, pkt, &key)) {
qemu_net_queue_send(s->incoming_queue, sender, 0,
(const uint8_t *)pkt->data, pkt->size, NULL);
packet_destroy(pkt, NULL);
