Re: [PATCH 2/4] net/colo: Fix a "double free" crash to clear the conn_list

[address@hidden]

On 31/03/2022 10:25, Zhang, Chen wrote:
>
>> -----Original Message-----
>> From: lizhijian@fujitsu.com <lizhijian@fujitsu.com>
>> Sent: Thursday, March 31, 2022 9:15 AM
>> To: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
>> <jasowang@redhat.com>
>> Cc: qemu-dev <qemu-devel@nongnu.org>; Like Xu <like.xu@linux.intel.com>
>> Subject: Re: [PATCH 2/4] net/colo: Fix a "double free" crash to clear the
>> conn_list
>>
>>
>> connection_track_table
>> -----+----------
>> key1 | conn    |-----------------------------------------------------------+
>> -----+----------                                                           |
>> key2 | conn    |----------------------------------+                        |
>> -----+----------                                  |                        |
>> key3 | conn    |---------+                        |                        |
>> -----+----------         |                        |                        |
>>                            |                        |                        
>> |
>>                            |                        |                        
>> |
>>       + CompareState ++    |                        |                        
>> |
>>       |               |    V                        V                        
>> V
>>       +---------------+   +---------------+         +---------------+
>>       |conn_list      +--->conn           +--------->conn           |     
>> connx
>>       +---------------+   +---------------+         +---------------+
>>       |               |     |           |             |          |
>>       +---------------+ +---v----+  +---v----+    +---v----+ +---v----+
>>                         |primary |  |secondary    |primary | |secondary
>>                         |packet  |  |packet  +    |packet  | |packet  +
>>                         +--------+  +--------+    +--------+ +--------+
>>                             |           |             |          |
>>                         +---v----+  +---v----+    +---v----+ +---v----+
>>                         |primary |  |secondary    |primary | |secondary
>>                         |packet  |  |packet  +    |packet  | |packet  +
>>                         +--------+  +--------+    +--------+ +--------+
>>                             |           |             |          |
>>                         +---v----+  +---v----+    +---v----+ +---v----+
>>                         |primary |  |secondary    |primary | |secondary
>>                         |packet  |  |packet  +    |packet  | |packet  +
>>                         +--------+  +--------+    +--------+ +--------+
>>
>> I recalled that we should above relationships between
>> connection_track_table conn_list and conn.
>> That means both connection_track_table and conn_list reference to the
>> same conn instance.
>>
>> So before this patch, connection_get() is possible to use-after-free/double
>> free conn. where 1st was in
>> connection_hashtable_reset() and 2nd was
>> 221             while (!g_queue_is_empty(conn_list)) {
>> 222                 connection_destroy(g_queue_pop_head(conn_list));
>> 223             }
>>
>> I also doubt that your current abort was just due to above use-after-
>> free/double free.
>> If so, looks it's enough we just update to g_queue_clear(conn_list) in the 
>> 2nd
>> place.
> Make sense, but It also means the original patch works here, skip free conn 
> in connection_hashtable_reset() and do it in:
> 221             while (!g_queue_is_empty(conn_list)) {
>   222                 connection_destroy(g_queue_pop_head(conn_list));
>   223             }.
> It also avoid use-after-free/double free conn.
Although you will not use-after-free here, you have to consider other 
situations carefully that
g_hash_table_remove_all() g_hash_table_destroy() were called where the 
conn_list should also be freed
with you approach.




> Maybe we can keep the original version to fix it?
And your commit log should be more clear.

Thanks
Zhijian

>
> Thanks
> Chen
>
>> Thanks
>> Zhijian
>>
>>
>> On 28/03/2022 17:13, Zhang, Chen wrote:
>>>> -----Original Message-----
>>>> From: lizhijian@fujitsu.com <lizhijian@fujitsu.com>
>>>> Sent: Monday, March 21, 2022 11:06 AM
>>>> To: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
>>>> <jasowang@redhat.com>; lizhijian@fujitsu.com
>>>> Cc: qemu-dev <qemu-devel@nongnu.org>; Like Xu
>>>> <like.xu@linux.intel.com>
>>>> Subject: Re: [PATCH 2/4] net/colo: Fix a "double free" crash to clear
>>>> the conn_list
>>>>
>>>>
>>>>
>>>> On 09/03/2022 16:38, Zhang Chen wrote:
>>>>> We notice the QEMU may crash when the guest has too many incoming
>>>>> network connections with the following log:
>>>>>
>>>>> 15197@1593578622.668573:colo_proxy_main : colo proxy connection
>>>>> hashtable full, clear it
>>>>> free(): invalid pointer
>>>>> [1]    15195 abort (core dumped)  qemu-system-x86_64 ....
>>>>>
>>>>> This is because we create the s->connection_track_table with
>>>>> g_hash_table_new_full() which is defined as:
>>>>>
>>>>> GHashTable * g_hash_table_new_full (GHashFunc hash_func,
>>>>>                           GEqualFunc key_equal_func,
>>>>>                           GDestroyNotify key_destroy_func,
>>>>>                           GDestroyNotify value_destroy_func);
>>>>>
>>>>> The fourth parameter connection_destroy() will be called to free the
>>>>> memory allocated for all 'Connection' values in the hashtable when
>>>>> we call g_hash_table_remove_all() in the connection_hashtable_reset().
>>>>>
>>>>> It's unnecessary because we clear the conn_list explicitly later,
>>>>> and it's buggy when other agents try to call connection_get() with
>>>>> the same connection_track_table.
>>>>>
>>>>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>>>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>>>> ---
>>>>>     net/colo-compare.c    | 2 +-
>>>>>     net/filter-rewriter.c | 2 +-
>>>>>     2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/colo-compare.c b/net/colo-compare.c index
>>>>> 62554b5b3c..ab054cfd21 100644
>>>>> --- a/net/colo-compare.c
>>>>> +++ b/net/colo-compare.c
>>>>> @@ -1324,7 +1324,7 @@ static void
>>>> colo_compare_complete(UserCreatable *uc, Error **errp)
>>>>>         s->connection_track_table =
>>>> g_hash_table_new_full(connection_key_hash,
>>>>>                                                           
>>>>> connection_key_equal,
>>>>>                                                           g_free,
>>>>> -                                                      
>>>>> connection_destroy);
>>>>> +                                                      NULL);
>>>> 202 /* if not found, create a new connection and add to hash table */
>>>> 203 Connection *connection_get(GHashTable *connection_track_table,
>>>> 204                            ConnectionKey *key,
>>>> 205                            GQueue *conn_list)
>>>> 206 {
>>>> 207     Connection *conn =
>>>> g_hash_table_lookup(connection_track_table,
>>>> key);
>>>> 208
>>>> 209     if (conn == NULL) {
>>>> 210         ConnectionKey *new_key = g_memdup(key, sizeof(*key));
>>>> 211
>>>> 212         conn = connection_new(key);
>>>> 213
>>>> 214         if (g_hash_table_size(connection_track_table) >
>>>> HASHTABLE_MAX_SIZE) {
>>>> 215             trace_colo_proxy_main("colo proxy connection hashtable 
>>>> full,"
>>>> 216                                   " clear it");
>>>> 217 connection_hashtable_reset(connection_track_table);
>>>>
>>>> 197 void connection_hashtable_reset(GHashTable
>>>> *connection_track_table)
>>>> 198 {
>>>> 199 g_hash_table_remove_all(connection_track_table);
>>>> 200 }
>>>>
>>>> IIUC,  above subroutine will do some cleanup explicitly. And before
>>>> your patch, connection_hashtable_reset() will release all keys and
>>>> their values in this hashtable. But now, you remove all keys and just
>>>> one value(conn_list) instead. Does it means other values will be leaked ?
>>> Thanks Zhijian. Re-think about it. Yes, I think you are right.
>>> It looks need a lock to avoid into connection_get() when triggered the clear
>> hashtable operation.
>>> What do you think?
>>>
>>> Thanks
>>> Chen
>>>
>>>
>>>> 218 /*
>>>> 219              * clear the conn_list
>>>> 220 */
>>>> 221             while (!g_queue_is_empty(conn_list)) {
>>>> 222 connection_destroy(g_queue_pop_head(conn_list));
>>>> 223 }
>>>> 224 }
>>>> 225
>>>> 226         g_hash_table_insert(connection_track_table, new_key,
>>>> conn);
>>>> 227 }
>>>> 228
>>>> 229     return conn;
>>>> 230 }
>>>>
>>>>
>>>> Thanks
>>>> Zhijian
>>>>
>>>>>         colo_compare_iothread(s);
>>>>>
>>>>> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c index
>>>>> bf05023dc3..c18c4c2019 100644
>>>>> --- a/net/filter-rewriter.c
>>>>> +++ b/net/filter-rewriter.c
>>>>> @@ -383,7 +383,7 @@ static void colo_rewriter_setup(NetFilterState
>>>>> *nf,
>>>> Error **errp)
>>>>>         s->connection_track_table =
>>>> g_hash_table_new_full(connection_key_hash,
>>>>>                                                           
>>>>> connection_key_equal,
>>>>>                                                           g_free,
>>>>> -                                                      
>>>>> connection_destroy);
>>>>> +                                                      NULL);
>>>>>         s->incoming_queue =
>>>> qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
>>>>>     }
>>>>>