Re: [Qemu-devel] [PATCH] COLO-compare: Add compare_lock aviod comparison conflict

[Jason Wang <jasowang@redhat.com>]

On 2017年04月20日 14:36, Zhang Chen wrote:
>
>
> On 04/20/2017 02:20 PM, Hailiang Zhang wrote:
>> On 2017/4/20 12:32, Zhang Chen wrote:
>>> When network traffic heavy, compare_pri_rs_finalize() and
>>> compare_sec_rs_finalize() have a chance to confilct.
>>> Both of them call colo_compare_connection() to compare packet,
>>> But during compare_pri_rs_finalize() comparison, have secondary
>>> packet come and call compare_sec_rs_finalize(), that packet will be
>>> handle twice. If packet same, the pkt will be double free.
>>
>> Interesting, if I'm right, this should not happen, because, all the 
>> comparing works
>> are done in colo compare thread, so there is no chance to access the 
>> connect_list
>> concurrently.  Besides, even both of the packets from primary and 
>> secondary arrive
>> at the same time, it should only be handle once, we will handle it 
>> with the later arrived one,
>> No ?
>
> No, In my test often trigger this bug, you can use udp server and 
> client test it.
>
> 13517@1492648526.850246:colo_compare_main : packet same and release 
> packet
> 13517@1492648526.850304:colo_compare_main : packet same and release 
> packet
> *** glibc detected *** 
> /home/zhangchen/qemu-colo-apr14/x86_64-softmmu/qemu-system-x86_64: 
> double free or corruption (out): 0x0000555556a75210 ***
> ======= Backtrace: =========
> /lib64/libc.so.6(+0x76628)[0x7ffff53d6628]
> /lib64/libc.so.6(cfree+0x6c)[0x7ffff53db5cc]
>
>
> Thanks
> Zhang Chen

I agree that you should check whether or not they are running in the 
same thread.

Thanks

>
>>
>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> ---
>>>   net/colo-compare.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>>> index 54e6d40..686c1b4 100644
>>> --- a/net/colo-compare.c
>>> +++ b/net/colo-compare.c
>>> @@ -79,6 +79,8 @@ typedef struct CompareState {
>>>        * element type: Connection
>>>        */
>>>       GQueue conn_list;
>>> +    /* compare lock */
>>> +    QemuMutex compare_lock;
>>>       /* hashtable to save connection */
>>>       GHashTable *connection_track_table;
>>>       /* compare thread, a thread for each NIC */
>>> @@ -619,7 +621,9 @@ static void 
>>> compare_pri_rs_finalize(SocketReadState *pri_rs)
>>>           compare_chr_send(&s->chr_out, pri_rs->buf, 
>>> pri_rs->packet_len);
>>>       } else {
>>>           /* compare connection */
>>> +        qemu_mutex_lock(&s->compare_lock);
>>>           g_queue_foreach(&s->conn_list, colo_compare_connection, s);
>>> +        qemu_mutex_unlock(&s->compare_lock);
>>>       }
>>>   }
>>>   @@ -631,7 +635,9 @@ static void 
>>> compare_sec_rs_finalize(SocketReadState *sec_rs)
>>>           trace_colo_compare_main("secondary: unsupported packet in");
>>>       } else {
>>>           /* compare connection */
>>> +        qemu_mutex_lock(&s->compare_lock);
>>>           g_queue_foreach(&s->conn_list, colo_compare_connection, s);
>>> +        qemu_mutex_unlock(&s->compare_lock);
>>>       }
>>>   }
>>>   @@ -702,6 +708,7 @@ static void 
>>> colo_compare_complete(UserCreatable *uc, Error **errp)
>>>       net_socket_rs_init(&s->sec_rs, compare_sec_rs_finalize);
>>>         g_queue_init(&s->conn_list);
>>> +    qemu_mutex_init(&s->compare_lock);
>>>         s->connection_track_table = 
>>> g_hash_table_new_full(connection_key_hash,
>>> connection_key_equal,
>>> @@ -771,6 +778,7 @@ static void colo_compare_finalize(Object *obj)
>>>       g_queue_foreach(&s->conn_list, colo_flush_packets, s);
>>>         g_queue_clear(&s->conn_list);
>>> +    qemu_mutex_destroy(&s->compare_lock);
>>>         g_hash_table_destroy(s->connection_track_table);
>>>       g_free(s->pri_indev);
>>
>>
>>
>>
>> .
>>
>