* [Qemu-devel] [PATCH 0/3] filter-rewriter: fix two bugs and one optimization
@ 2017-02-20 8:01 zhanghailiang
2017-02-20 8:01 ` [Qemu-devel] [PATCH 1/3] net/colo: fix memory double free error zhanghailiang
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: zhanghailiang @ 2017-02-20 8:01 UTC (permalink / raw)
To: jasowang, zhangchen.fnst, lizhijian
Cc: qemu-devel, xuquan8, pss.wulizhen, zhanghailiang
Hi,
Patch 1 fixes a double free bug, and patch 2 fixes a memory leak bug.
Patch 3 is an optimization for filter-rewriter.
Please review, thanks.
zhanghailiang (3):
net/colo: fix memory double free error
filter-rewriter: fix memory leak for connection in
connection_track_table
filter-rewriter: skip net_checksum_calculate() while offset = 0
net/colo.c | 2 --
net/colo.h | 4 +++
net/filter-rewriter.c | 86 +++++++++++++++++++++++++++++++++++++++++++--------
3 files changed, 77 insertions(+), 15 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/3] net/colo: fix memory double free error
2017-02-20 8:01 [Qemu-devel] [PATCH 0/3] filter-rewriter: fix two bugs and one optimization zhanghailiang
@ 2017-02-20 8:01 ` zhanghailiang
2017-02-21 2:25 ` Zhang Chen
2017-02-20 8:02 ` [Qemu-devel] [PATCH 2/3] filter-rewriter: fix memory leak for connection in connection_track_table zhanghailiang
2017-02-20 8:02 ` [Qemu-devel] [PATCH 3/3] filter-rewriter: skip net_checksum_calculate() while offset = 0 zhanghailiang
2 siblings, 1 reply; 6+ messages in thread
From: zhanghailiang @ 2017-02-20 8:01 UTC (permalink / raw)
To: jasowang, zhangchen.fnst, lizhijian
Cc: qemu-devel, xuquan8, pss.wulizhen, zhanghailiang
The 'primary_list' and 'secondary_list' members of struct Connection
is not allocated through dynamically g_queue_new(), but we free it by using
g_queue_free(), which will lead to a double-free bug.
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
net/colo.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/colo.c b/net/colo.c
index 6a6eacd..7d5c423 100644
--- a/net/colo.c
+++ b/net/colo.c
@@ -147,9 +147,7 @@ void connection_destroy(void *opaque)
Connection *conn = opaque;
g_queue_foreach(&conn->primary_list, packet_destroy, NULL);
- g_queue_free(&conn->primary_list);
g_queue_foreach(&conn->secondary_list, packet_destroy, NULL);
- g_queue_free(&conn->secondary_list);
g_slice_free(Connection, conn);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/3] filter-rewriter: fix memory leak for connection in connection_track_table
2017-02-20 8:01 [Qemu-devel] [PATCH 0/3] filter-rewriter: fix two bugs and one optimization zhanghailiang
2017-02-20 8:01 ` [Qemu-devel] [PATCH 1/3] net/colo: fix memory double free error zhanghailiang
@ 2017-02-20 8:02 ` zhanghailiang
2017-02-20 8:02 ` [Qemu-devel] [PATCH 3/3] filter-rewriter: skip net_checksum_calculate() while offset = 0 zhanghailiang
2 siblings, 0 replies; 6+ messages in thread
From: zhanghailiang @ 2017-02-20 8:02 UTC (permalink / raw)
To: jasowang, zhangchen.fnst, lizhijian
Cc: qemu-devel, xuquan8, pss.wulizhen, zhanghailiang
After a net connection is closed, we didn't clear its releated resources
in connection_track_table, which will lead to memory leak.
Let't track the state of net connection, if it is closed, its related
resources will be cleared up.
Signed-off-by: zhanghailiang <zhang.zhanghailiang@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 "
+ 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;
}
-
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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 3/3] filter-rewriter: skip net_checksum_calculate() while offset = 0
2017-02-20 8:01 [Qemu-devel] [PATCH 0/3] filter-rewriter: fix two bugs and one optimization zhanghailiang
2017-02-20 8:01 ` [Qemu-devel] [PATCH 1/3] net/colo: fix memory double free error zhanghailiang
2017-02-20 8:02 ` [Qemu-devel] [PATCH 2/3] filter-rewriter: fix memory leak for connection in connection_track_table zhanghailiang
@ 2017-02-20 8:02 ` zhanghailiang
2 siblings, 0 replies; 6+ messages in thread
From: zhanghailiang @ 2017-02-20 8:02 UTC (permalink / raw)
To: jasowang, zhangchen.fnst, lizhijian
Cc: qemu-devel, xuquan8, pss.wulizhen, zhanghailiang
While the offset of packets's sequence for primary side and
secondary side is zero, it is unnecessary to call net_checksum_calculate()
to recalculate the checksume value of packets.
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
net/filter-rewriter.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index 7e7ec35..c9a6d43 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -93,10 +93,12 @@ static int handle_primary_tcp_pkt(RewriterState *rf,
conn->offset -= (ntohl(tcp_pkt->th_ack) - 1);
conn->syn_flag = 0;
}
- /* handle packets to the secondary from the primary */
- tcp_pkt->th_ack = htonl(ntohl(tcp_pkt->th_ack) + conn->offset);
+ if (conn->offset) {
+ /* handle packets to the secondary from the primary */
+ tcp_pkt->th_ack = htonl(ntohl(tcp_pkt->th_ack) + conn->offset);
- net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ }
/*
* Case 1:
* The *server* side of this connect is VM, *client* tries to close
@@ -112,7 +114,6 @@ static int handle_primary_tcp_pkt(RewriterState *rf,
*/
if ((conn->tcp_state == TCPS_LAST_ACK) &&
(ntohl(tcp_pkt->th_ack) == (conn->fin_ack_seq + 1))) {
- fprintf(stderr, "Remove conn "
g_hash_table_remove(rf->connection_track_table, key);
}
}
@@ -159,10 +160,13 @@ static int handle_secondary_tcp_pkt(RewriterState *rf,
}
if ((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK) {
- /* handle packets to the primary from the secondary*/
- tcp_pkt->th_seq = htonl(ntohl(tcp_pkt->th_seq) - conn->offset);
+ /* Only need to adjust seq while offset is Non-zero */
+ if (conn->offset) {
+ /* handle packets to the primary from the secondary*/
+ tcp_pkt->th_seq = htonl(ntohl(tcp_pkt->th_seq) - conn->offset);
- net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ }
/*
* Case 2:
* The *server* side of this connect is VM, *server* tries to close
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] net/colo: fix memory double free error
2017-02-20 8:01 ` [Qemu-devel] [PATCH 1/3] net/colo: fix memory double free error zhanghailiang
@ 2017-02-21 2:25 ` Zhang Chen
2017-02-21 3:06 ` Hailiang Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Zhang Chen @ 2017-02-21 2:25 UTC (permalink / raw)
To: zhanghailiang, jasowang, lizhijian; +Cc: qemu-devel, xuquan8, pss.wulizhen
On 02/20/2017 04:01 PM, zhanghailiang wrote:
> The 'primary_list' and 'secondary_list' members of struct Connection
> is not allocated through dynamically g_queue_new(), but we free it by using
> g_queue_free(), which will lead to a double-free bug.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
> net/colo.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/colo.c b/net/colo.c
> index 6a6eacd..7d5c423 100644
> --- a/net/colo.c
> +++ b/net/colo.c
> @@ -147,9 +147,7 @@ void connection_destroy(void *opaque)
> Connection *conn = opaque;
>
> g_queue_foreach(&conn->primary_list, packet_destroy, NULL);
> - g_queue_free(&conn->primary_list);
> g_queue_foreach(&conn->secondary_list, packet_destroy, NULL);
> - g_queue_free(&conn->secondary_list);
I think we need use g_queue_clear () here.
void
g_queue_clear (GQueue *queue);
Removes all the elements in queue . If queue elements contain
dynamically-allocated memory, they should be freed first.
Thanks
Zhang Chen
> g_slice_free(Connection, conn);
> }
>
--
Thanks
Zhang Chen
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] net/colo: fix memory double free error
2017-02-21 2:25 ` Zhang Chen
@ 2017-02-21 3:06 ` Hailiang Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Hailiang Zhang @ 2017-02-21 3:06 UTC (permalink / raw)
To: Zhang Chen, jasowang, lizhijian; +Cc: xuquan8, qemu-devel, pss.wulizhen
On 2017/2/21 10:25, Zhang Chen wrote:
>
>
> On 02/20/2017 04:01 PM, zhanghailiang wrote:
>> The 'primary_list' and 'secondary_list' members of struct Connection
>> is not allocated through dynamically g_queue_new(), but we free it by using
>> g_queue_free(), which will lead to a double-free bug.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>> net/colo.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/net/colo.c b/net/colo.c
>> index 6a6eacd..7d5c423 100644
>> --- a/net/colo.c
>> +++ b/net/colo.c
>> @@ -147,9 +147,7 @@ void connection_destroy(void *opaque)
>> Connection *conn = opaque;
>>
>> g_queue_foreach(&conn->primary_list, packet_destroy, NULL);
>> - g_queue_free(&conn->primary_list);
>> g_queue_foreach(&conn->secondary_list, packet_destroy, NULL);
>> - g_queue_free(&conn->secondary_list);
>
> I think we need use g_queue_clear () here.
>
Ha, you are right, my original modification will introduce memory leak.
Will fix in next version.
> void
> g_queue_clear (GQueue *queue);
> Removes all the elements in queue . If queue elements contain
> dynamically-allocated memory, they should be freed first.
>
> Thanks
> Zhang Chen
>
>> g_slice_free(Connection, conn);
>> }
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-21 3:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-20 8:01 [Qemu-devel] [PATCH 0/3] filter-rewriter: fix two bugs and one optimization zhanghailiang
2017-02-20 8:01 ` [Qemu-devel] [PATCH 1/3] net/colo: fix memory double free error zhanghailiang
2017-02-21 2:25 ` Zhang Chen
2017-02-21 3:06 ` Hailiang Zhang
2017-02-20 8:02 ` [Qemu-devel] [PATCH 2/3] filter-rewriter: fix memory leak for connection in connection_track_table zhanghailiang
2017-02-20 8:02 ` [Qemu-devel] [PATCH 3/3] filter-rewriter: skip net_checksum_calculate() while offset = 0 zhanghailiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.