Re: [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams

[Zheng Chuan]

On 2020/9/14 17:26, Daniel P. Berrangé wrote:
> On Mon, Sep 14, 2020 at 05:20:14PM +0800, Zheng Chuan wrote:
>>
>>
>> On 2020/9/14 17:02, Daniel P. Berrangé wrote:
>>> On Sun, Sep 13, 2020 at 10:47:33AM +0800, Chuan Zheng wrote:
>>>> MigrationState is need for tls session build and tls hostname is need
>>>> for tls handshake, add both MigrationState and tls_hostname
>>>> into MultiFDSendParams.
>>>>
>>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>>> Signed-off-by: Yan Jin <jinyan12@huawei.com>
>>>> ---
>>>>  migration/multifd.c | 5 +++++
>>>>  migration/multifd.h | 4 ++++
>>>>  2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/migration/multifd.c b/migration/multifd.c
>>>> index d044120..3e41d9e 100644
>>>> --- a/migration/multifd.c
>>>> +++ b/migration/multifd.c
>>>> @@ -543,11 +543,14 @@ void multifd_save_cleanup(void)
>>>>  
>>>>          socket_send_channel_destroy(p->c);
>>>>          p->c = NULL;
>>>> +        p->s = NULL;
>>>>          qemu_mutex_destroy(&p->mutex);
>>>>          qemu_sem_destroy(&p->sem);
>>>>          qemu_sem_destroy(&p->sem_sync);
>>>>          g_free(p->name);
>>>>          p->name = NULL;
>>>> +        g_free(p->tls_hostname);
>>>> +        p->tls_hostname = NULL;
>>>>          multifd_pages_clear(p->pages);
>>>>          p->pages = NULL;
>>>>          p->packet_len = 0;
>>>> @@ -779,6 +782,8 @@ int multifd_save_setup(Error **errp)
>>>>          p->packet->magic = cpu_to_be32(MULTIFD_MAGIC);
>>>>          p->packet->version = cpu_to_be32(MULTIFD_VERSION);
>>>>          p->name = g_strdup_printf("multifdsend_%d", i);
>>>> +        p->s = migrate_get_current();
>>>> +        p->tls_hostname = g_strdup(p->s->hostname);
>>>>          socket_send_channel_create(multifd_new_send_channel_async, p);
>>>>      }
>>>>  
>>>> diff --git a/migration/multifd.h b/migration/multifd.h
>>>> index 448a03d..2b400e7 100644
>>>> --- a/migration/multifd.h
>>>> +++ b/migration/multifd.h
>>>> @@ -66,11 +66,15 @@ typedef struct {
>>>>  } MultiFDPages_t;
>>>>  
>>>>  typedef struct {
>>>> +    /* Migration State */
>>>> +    MigrationState *s;
>>>>      /* this fields are not changed once the thread is created */
>>>>      /* channel number */
>>>>      uint8_t id;
>>>>      /* channel thread name */
>>>>      char *name;
>>>> +    /* tls hostname */
>>>> +    char *tls_hostname;
>>>
>>> Why do we need this, when it is already accessible from the
>>> MigrationState field you're adding
>>>
>>>
>>> Regards,
>>> Daniel
>>>
>> Hi，Daniel. Thank you for your review.
>>
>> This is because i have free hostname in MigrationState field after 
>> migrate_fd_connect(s, error).
>> Since multifd thread creation is async by socket_send_channel_create(), we 
>> must record it in MultiFDSendParams
>> in case of concurrency issues.
>>
>> migration_channel_connect
>>        migrate_fd_connect
>>            multifd_save_setup
>>               socket_send_channel_create(multifd_new_send_channel_async, p); 
>> / async, do not wait for multifd creation
>>                 g_free(s->hostname);
>>                                                           
>> multifd_new_send_channel_async
>>                                                                   
>> multifd_channel_connect
>>                                                                          
>> multifd_tls_channel_connect
>>                                                                              
>>    migration_tls_client_create  /* UAF happen */
>>
>> As you mentioned in Patch001, i am not sure if it will cause the same 
>> concurrency issues if i put hostname in MigrationState field
>> freed in migrate_fd_cancel.
> 
> If MigrationState isn't safe to access from the multifd threads, then
> don't addd it to the struct, as I think that will mislead people into
> thinking it is ok to use. Only add the hostname.
> 
> 
> Regards,
> Daniel
> 
Sure, I'll fix that in V4.

In addition, is that OK I pass hostname to MultiFDSendParams through 
MigrationState field as Patch001 do?