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 <zhengch...@huawei.com>
>> Signed-off-by: Yan Jin <jinya...@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.
