* [PATCH v3 1/6] migration/tls: save hostname into MigrationState
2020-09-13 2:47 [PATCH v3 0/6] *** Add Multifd support for TLS migration *** Chuan Zheng
@ 2020-09-13 2:47 ` Chuan Zheng
2020-09-14 9:00 ` Daniel P. Berrangé
2020-09-13 2:47 ` [PATCH v3 2/6] migration/tls: extract migration_tls_client_create for common-use Chuan Zheng
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Chuan Zheng @ 2020-09-13 2:47 UTC (permalink / raw)
To: quintela, eblake, dgilbert, berrange
Cc: zhengchuan, zhang.zhanghailiang, yuxiating, qemu-devel,
xiexiangyou, alex.chen, jinyan12
hostname is need in multifd-tls, save hostname into MigrationState.
Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: Yan Jin <jinyan12@huawei.com>
---
migration/channel.c | 6 ++++++
migration/migration.c | 1 +
migration/migration.h | 5 +++++
3 files changed, 12 insertions(+)
diff --git a/migration/channel.c b/migration/channel.c
index 20e4c8e..0e4104a 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -66,6 +66,11 @@ void migration_channel_connect(MigrationState *s,
trace_migration_set_outgoing_channel(
ioc, object_get_typename(OBJECT(ioc)), hostname, error);
+ /* Save hostname into MigrationState for handshake */
+ if (hostname) {
+ s->hostname = g_strdup(hostname);
+ }
+
if (!error) {
if (s->parameters.tls_creds &&
*s->parameters.tls_creds &&
@@ -90,5 +95,6 @@ void migration_channel_connect(MigrationState *s,
}
}
migrate_fd_connect(s, error);
+ g_free(s->hostname);
error_free(error);
}
diff --git a/migration/migration.c b/migration/migration.c
index 58a5452..e20b778 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1883,6 +1883,7 @@ void migrate_init(MigrationState *s)
s->migration_thread_running = false;
error_free(s->error);
s->error = NULL;
+ s->hostname = NULL;
migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
diff --git a/migration/migration.h b/migration/migration.h
index bdc7450..bc96322 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -259,6 +259,11 @@ struct MigrationState
* (which is in 4M chunk).
*/
uint8_t clear_bitmap_shift;
+
+ /*
+ * This save hostname when out-going migration starts
+ */
+ char *hostname;
};
void migrate_set_state(int *state, int old_state, int new_state);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/6] migration/tls: save hostname into MigrationState
2020-09-13 2:47 ` [PATCH v3 1/6] migration/tls: save hostname into MigrationState Chuan Zheng
@ 2020-09-14 9:00 ` Daniel P. Berrangé
2020-09-14 11:22 ` Zheng Chuan
0 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Berrangé @ 2020-09-14 9:00 UTC (permalink / raw)
To: Chuan Zheng
Cc: zhang.zhanghailiang, quintela, yuxiating, dgilbert, xiexiangyou,
qemu-devel, alex.chen, jinyan12
On Sun, Sep 13, 2020 at 10:47:31AM +0800, Chuan Zheng wrote:
> hostname is need in multifd-tls, save hostname into MigrationState.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: Yan Jin <jinyan12@huawei.com>
> ---
> migration/channel.c | 6 ++++++
> migration/migration.c | 1 +
> migration/migration.h | 5 +++++
> 3 files changed, 12 insertions(+)
>
> diff --git a/migration/channel.c b/migration/channel.c
> index 20e4c8e..0e4104a 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -66,6 +66,11 @@ void migration_channel_connect(MigrationState *s,
> trace_migration_set_outgoing_channel(
> ioc, object_get_typename(OBJECT(ioc)), hostname, error);
>
> + /* Save hostname into MigrationState for handshake */
> + if (hostname) {
> + s->hostname = g_strdup(hostname);
> + }
> +
> if (!error) {
> if (s->parameters.tls_creds &&
> *s->parameters.tls_creds &&
> @@ -90,5 +95,6 @@ void migration_channel_connect(MigrationState *s,
> }
> }
> migrate_fd_connect(s, error);
> + g_free(s->hostname);
> error_free(error);
> }
IIUC, this means hostname is free'd once the initial connection is
established. Don't we have to wait until all the multifd connections
exist too ?
IOW, should we be doing this somewhere in a cleanup path. Perhaps
migrate_fd_cancel() is the rigt place ?
> diff --git a/migration/migration.c b/migration/migration.c
> index 58a5452..e20b778 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1883,6 +1883,7 @@ void migrate_init(MigrationState *s)
> s->migration_thread_running = false;
> error_free(s->error);
> s->error = NULL;
> + s->hostname = NULL;
>
> migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
>
> diff --git a/migration/migration.h b/migration/migration.h
> index bdc7450..bc96322 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -259,6 +259,11 @@ struct MigrationState
> * (which is in 4M chunk).
> */
> uint8_t clear_bitmap_shift;
> +
> + /*
> + * This save hostname when out-going migration starts
> + */
> + char *hostname;
> };
>
> void migrate_set_state(int *state, int old_state, int new_state);
> --
> 1.8.3.1
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/6] migration/tls: save hostname into MigrationState
2020-09-14 9:00 ` Daniel P. Berrangé
@ 2020-09-14 11:22 ` Zheng Chuan
0 siblings, 0 replies; 14+ messages in thread
From: Zheng Chuan @ 2020-09-14 11:22 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: zhang.zhanghailiang, quintela, yuxiating, dgilbert, xiexiangyou,
qemu-devel, alex.chen, jinyan12
On 2020/9/14 17:00, Daniel P. Berrangé wrote:
> On Sun, Sep 13, 2020 at 10:47:31AM +0800, Chuan Zheng wrote:
>> hostname is need in multifd-tls, save hostname into MigrationState.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: Yan Jin <jinyan12@huawei.com>
>> ---
>> migration/channel.c | 6 ++++++
>> migration/migration.c | 1 +
>> migration/migration.h | 5 +++++
>> 3 files changed, 12 insertions(+)
>>
>> diff --git a/migration/channel.c b/migration/channel.c
>> index 20e4c8e..0e4104a 100644
>> --- a/migration/channel.c
>> +++ b/migration/channel.c
>> @@ -66,6 +66,11 @@ void migration_channel_connect(MigrationState *s,
>> trace_migration_set_outgoing_channel(
>> ioc, object_get_typename(OBJECT(ioc)), hostname, error);
>>
>> + /* Save hostname into MigrationState for handshake */
>> + if (hostname) {
>> + s->hostname = g_strdup(hostname);
>> + }
>> +
>> if (!error) {
>> if (s->parameters.tls_creds &&
>> *s->parameters.tls_creds &&
>> @@ -90,5 +95,6 @@ void migration_channel_connect(MigrationState *s,
>> }
>> }
>> migrate_fd_connect(s, error);
>> + g_free(s->hostname);
>> error_free(error);
>> }
>
> IIUC, this means hostname is free'd once the initial connection is
> established. Don't we have to wait until all the multifd connections
> exist too ?
>
> IOW, should we be doing this somewhere in a cleanup path. Perhaps
> migrate_fd_cancel() is the rigt place ?
>
Well, Maybe another alternate way is define series functions to save/destroy tls_hostname in 'tls.c'
other than add hostname into MigrationState.
such as:
+static char *migration_tls_hostname = NULL;
+
+void migration_destroy_tls_hostname(void)
+{
+ if (migration_tls_hostname) {
+ g_free(migration_tls_hostname);
+ migration_tls_hostname = NULL;
+ }
+}
+
+static void migration_save_tls_hostname(const char *hostname)
+{
+ migration_destroy_tls_hostname();
+ migration_tls_hostname = g_strdup(hostname);
+}
+
+char *migration_get_tls_hostname(void)
+{
+ return migration_tls_hostname;
+}
How do you think, is that better?
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 58a5452..e20b778 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1883,6 +1883,7 @@ void migrate_init(MigrationState *s)
>> s->migration_thread_running = false;
>> error_free(s->error);
>> s->error = NULL;
>> + s->hostname = NULL;
>>
>> migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
>>
>> diff --git a/migration/migration.h b/migration/migration.h
>> index bdc7450..bc96322 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -259,6 +259,11 @@ struct MigrationState
>> * (which is in 4M chunk).
>> */
>> uint8_t clear_bitmap_shift;
>> +
>> + /*
>> + * This save hostname when out-going migration starts
>> + */
>> + char *hostname;
>> };
>>
>> void migrate_set_state(int *state, int old_state, int new_state);
>> --
>> 1.8.3.1
>>
>
> Regards,
> Daniel
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 2/6] migration/tls: extract migration_tls_client_create for common-use
2020-09-13 2:47 [PATCH v3 0/6] *** Add Multifd support for TLS migration *** Chuan Zheng
2020-09-13 2:47 ` [PATCH v3 1/6] migration/tls: save hostname into MigrationState Chuan Zheng
@ 2020-09-13 2:47 ` Chuan Zheng
2020-09-13 2:47 ` [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams Chuan Zheng
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Chuan Zheng @ 2020-09-13 2:47 UTC (permalink / raw)
To: quintela, eblake, dgilbert, berrange
Cc: zhengchuan, zhang.zhanghailiang, yuxiating, qemu-devel,
xiexiangyou, alex.chen, jinyan12
migration_tls_client_create will be used in multifd-tls, let's
extract it.
Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: Yan Jin <jinyan12@huawei.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
migration/tls.c | 26 ++++++++++++++++++--------
migration/tls.h | 6 ++++++
2 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/migration/tls.c b/migration/tls.c
index 7a02ec8..186be8a 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -22,7 +22,6 @@
#include "channel.h"
#include "migration.h"
#include "tls.h"
-#include "io/channel-tls.h"
#include "crypto/tlscreds.h"
#include "qemu/error-report.h"
#include "qapi/error.h"
@@ -125,11 +124,10 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
object_unref(OBJECT(ioc));
}
-
-void migration_tls_channel_connect(MigrationState *s,
- QIOChannel *ioc,
- const char *hostname,
- Error **errp)
+QIOChannelTLS *migration_tls_client_create(MigrationState *s,
+ QIOChannel *ioc,
+ const char *hostname,
+ Error **errp)
{
QCryptoTLSCreds *creds;
QIOChannelTLS *tioc;
@@ -137,7 +135,7 @@ void migration_tls_channel_connect(MigrationState *s,
creds = migration_tls_get_creds(
s, QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, errp);
if (!creds) {
- return;
+ return NULL;
}
if (s->parameters.tls_hostname && *s->parameters.tls_hostname) {
@@ -145,11 +143,23 @@ void migration_tls_channel_connect(MigrationState *s,
}
if (!hostname) {
error_setg(errp, "No hostname available for TLS");
- return;
+ return NULL;
}
tioc = qio_channel_tls_new_client(
ioc, creds, hostname, errp);
+
+ return tioc;
+}
+
+void migration_tls_channel_connect(MigrationState *s,
+ QIOChannel *ioc,
+ const char *hostname,
+ Error **errp)
+{
+ QIOChannelTLS *tioc;
+
+ tioc = migration_tls_client_create(s, ioc, hostname, errp);
if (!tioc) {
return;
}
diff --git a/migration/tls.h b/migration/tls.h
index cdd7000..0cfbe36 100644
--- a/migration/tls.h
+++ b/migration/tls.h
@@ -22,11 +22,17 @@
#define QEMU_MIGRATION_TLS_H
#include "io/channel.h"
+#include "io/channel-tls.h"
void migration_tls_channel_process_incoming(MigrationState *s,
QIOChannel *ioc,
Error **errp);
+QIOChannelTLS *migration_tls_client_create(MigrationState *s,
+ QIOChannel *ioc,
+ const char *hostname,
+ Error **errp);
+
void migration_tls_channel_connect(MigrationState *s,
QIOChannel *ioc,
const char *hostname,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams
2020-09-13 2:47 [PATCH v3 0/6] *** Add Multifd support for TLS migration *** Chuan Zheng
2020-09-13 2:47 ` [PATCH v3 1/6] migration/tls: save hostname into MigrationState Chuan Zheng
2020-09-13 2:47 ` [PATCH v3 2/6] migration/tls: extract migration_tls_client_create for common-use Chuan Zheng
@ 2020-09-13 2:47 ` Chuan Zheng
2020-09-14 9:02 ` Daniel P. Berrangé
2020-09-13 2:47 ` [PATCH v3 4/6] migration/tls: extract cleanup function for common-use Chuan Zheng
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Chuan Zheng @ 2020-09-13 2:47 UTC (permalink / raw)
To: quintela, eblake, dgilbert, berrange
Cc: zhengchuan, zhang.zhanghailiang, yuxiating, qemu-devel,
xiexiangyou, alex.chen, jinyan12
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;
/* channel thread id */
QemuThread thread;
/* communication channel */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams
2020-09-13 2:47 ` [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams Chuan Zheng
@ 2020-09-14 9:02 ` Daniel P. Berrangé
2020-09-14 9:20 ` Zheng Chuan
0 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Berrangé @ 2020-09-14 9:02 UTC (permalink / raw)
To: Chuan Zheng
Cc: zhang.zhanghailiang, quintela, yuxiating, qemu-devel,
xiexiangyou, dgilbert, alex.chen, jinyan12
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams
2020-09-14 9:02 ` Daniel P. Berrangé
@ 2020-09-14 9:20 ` Zheng Chuan
2020-09-14 9:26 ` Daniel P. Berrangé
0 siblings, 1 reply; 14+ messages in thread
From: Zheng Chuan @ 2020-09-14 9:20 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: zhang.zhanghailiang, quintela, yuxiating, qemu-devel,
xiexiangyou, dgilbert, alex.chen, jinyan12
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.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams
2020-09-14 9:20 ` Zheng Chuan
@ 2020-09-14 9:26 ` Daniel P. Berrangé
2020-09-14 9:36 ` Zheng Chuan
0 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Berrangé @ 2020-09-14 9:26 UTC (permalink / raw)
To: Zheng Chuan
Cc: zhang.zhanghailiang, quintela, yuxiating, qemu-devel,
xiexiangyou, dgilbert, alex.chen, jinyan12
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams
2020-09-14 9:26 ` Daniel P. Berrangé
@ 2020-09-14 9:36 ` Zheng Chuan
0 siblings, 0 replies; 14+ messages in thread
From: Zheng Chuan @ 2020-09-14 9:36 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: zhang.zhanghailiang, quintela, yuxiating, qemu-devel,
xiexiangyou, dgilbert, alex.chen, jinyan12
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?
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 4/6] migration/tls: extract cleanup function for common-use
2020-09-13 2:47 [PATCH v3 0/6] *** Add Multifd support for TLS migration *** Chuan Zheng
` (2 preceding siblings ...)
2020-09-13 2:47 ` [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams Chuan Zheng
@ 2020-09-13 2:47 ` Chuan Zheng
2020-09-13 2:47 ` [PATCH v3 5/6] migration/tls: add support for multifd tls-handshake Chuan Zheng
2020-09-13 2:47 ` [PATCH v3 6/6] migration/tls: add trace points for multifd-tls Chuan Zheng
5 siblings, 0 replies; 14+ messages in thread
From: Chuan Zheng @ 2020-09-13 2:47 UTC (permalink / raw)
To: quintela, eblake, dgilbert, berrange
Cc: zhengchuan, zhang.zhanghailiang, yuxiating, qemu-devel,
xiexiangyou, alex.chen, jinyan12
multifd channel cleanup is need if multifd handshake failed,
let's extract it.
Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: Yan Jin <jinyan12@huawei.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
migration/multifd.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index 3e41d9e..fe08911 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -720,6 +720,23 @@ out:
return NULL;
}
+static void multifd_new_send_channel_cleanup(MultiFDSendParams *p,
+ QIOChannel *ioc, Error *err)
+{
+ migrate_set_error(migrate_get_current(), err);
+ /* Error happen, we need to tell who pay attention to me */
+ qemu_sem_post(&multifd_send_state->channels_ready);
+ qemu_sem_post(&p->sem_sync);
+ /*
+ * Although multifd_send_thread is not created, but main migration
+ * thread neet to judge whether it is running, so we need to mark
+ * its status.
+ */
+ p->quit = true;
+ object_unref(OBJECT(ioc));
+ error_free(err);
+}
+
static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
{
MultiFDSendParams *p = opaque;
@@ -728,25 +745,18 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
trace_multifd_new_send_channel_async(p->id);
if (qio_task_propagate_error(task, &local_err)) {
- migrate_set_error(migrate_get_current(), local_err);
- /* Error happen, we need to tell who pay attention to me */
- qemu_sem_post(&multifd_send_state->channels_ready);
- qemu_sem_post(&p->sem_sync);
- /*
- * Although multifd_send_thread is not created, but main migration
- * thread neet to judge whether it is running, so we need to mark
- * its status.
- */
- p->quit = true;
- object_unref(OBJECT(sioc));
- error_free(local_err);
+ goto cleanup;
} else {
p->c = QIO_CHANNEL(sioc);
qio_channel_set_delay(p->c, false);
p->running = true;
qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
QEMU_THREAD_JOINABLE);
+ return;
}
+
+cleanup:
+ multifd_new_send_channel_cleanup(p, sioc, local_err);
}
int multifd_save_setup(Error **errp)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 5/6] migration/tls: add support for multifd tls-handshake
2020-09-13 2:47 [PATCH v3 0/6] *** Add Multifd support for TLS migration *** Chuan Zheng
` (3 preceding siblings ...)
2020-09-13 2:47 ` [PATCH v3 4/6] migration/tls: extract cleanup function for common-use Chuan Zheng
@ 2020-09-13 2:47 ` Chuan Zheng
2020-09-13 2:47 ` [PATCH v3 6/6] migration/tls: add trace points for multifd-tls Chuan Zheng
5 siblings, 0 replies; 14+ messages in thread
From: Chuan Zheng @ 2020-09-13 2:47 UTC (permalink / raw)
To: quintela, eblake, dgilbert, berrange
Cc: zhengchuan, zhang.zhanghailiang, yuxiating, qemu-devel,
xiexiangyou, alex.chen, jinyan12
Similar like migration main thread, we need to do handshake
for each multifd thread.
Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: Yan Jin <jinyan12@huawei.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
migration/multifd.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 75 insertions(+), 2 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index fe08911..8aea4e0 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -20,6 +20,7 @@
#include "ram.h"
#include "migration.h"
#include "socket.h"
+#include "tls.h"
#include "qemu-file.h"
#include "trace.h"
#include "multifd.h"
@@ -720,6 +721,77 @@ out:
return NULL;
}
+static bool multifd_channel_connect(MultiFDSendParams *p,
+ QIOChannel *ioc,
+ Error *error);
+
+static void multifd_tls_outgoing_handshake(QIOTask *task,
+ gpointer opaque)
+{
+ MultiFDSendParams *p = opaque;
+ QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
+ Error *err = NULL;
+
+ qio_task_propagate_error(task, &err);
+ multifd_channel_connect(p, ioc, err);
+}
+
+static void multifd_tls_channel_connect(MultiFDSendParams *p,
+ QIOChannel *ioc,
+ Error **errp)
+{
+ MigrationState *s = p->s;
+ const char *hostname = p->tls_hostname;
+ QIOChannelTLS *tioc;
+
+ tioc = migration_tls_client_create(s, ioc, hostname, errp);
+ if (!tioc) {
+ return;
+ }
+
+ qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
+ qio_channel_tls_handshake(tioc,
+ multifd_tls_outgoing_handshake,
+ p,
+ NULL,
+ NULL);
+
+}
+
+static bool multifd_channel_connect(MultiFDSendParams *p,
+ QIOChannel *ioc,
+ Error *error)
+{
+ MigrationState *s = p->s;
+
+ if (!error) {
+ if (s->parameters.tls_creds &&
+ *s->parameters.tls_creds &&
+ !object_dynamic_cast(OBJECT(ioc),
+ TYPE_QIO_CHANNEL_TLS)) {
+ multifd_tls_channel_connect(p, ioc, &error);
+ if (!error) {
+ /*
+ * tls_channel_connect will call back to this
+ * function after the TLS handshake,
+ * so we mustn't call multifd_send_thread until then
+ */
+ return false;
+ } else {
+ return true;
+ }
+ } else {
+ /* update for tls qio channel */
+ p->c = ioc;
+ qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
+ QEMU_THREAD_JOINABLE);
+ }
+ return false;
+ }
+
+ return true;
+}
+
static void multifd_new_send_channel_cleanup(MultiFDSendParams *p,
QIOChannel *ioc, Error *err)
{
@@ -750,8 +822,9 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
p->c = QIO_CHANNEL(sioc);
qio_channel_set_delay(p->c, false);
p->running = true;
- qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
- QEMU_THREAD_JOINABLE);
+ if (multifd_channel_connect(p, sioc, local_err)) {
+ goto cleanup;
+ }
return;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 6/6] migration/tls: add trace points for multifd-tls
2020-09-13 2:47 [PATCH v3 0/6] *** Add Multifd support for TLS migration *** Chuan Zheng
` (4 preceding siblings ...)
2020-09-13 2:47 ` [PATCH v3 5/6] migration/tls: add support for multifd tls-handshake Chuan Zheng
@ 2020-09-13 2:47 ` Chuan Zheng
2020-09-14 8:55 ` Daniel P. Berrangé
5 siblings, 1 reply; 14+ messages in thread
From: Chuan Zheng @ 2020-09-13 2:47 UTC (permalink / raw)
To: quintela, eblake, dgilbert, berrange
Cc: zhengchuan, zhang.zhanghailiang, yuxiating, qemu-devel,
xiexiangyou, alex.chen, jinyan12
add trace points for multifd-tls for debug.
Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: Yan Jin <jinyan12@huawei.com>
---
migration/multifd.c | 10 +++++++++-
migration/trace-events | 4 ++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index 8aea4e0..0760502 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -732,7 +732,11 @@ static void multifd_tls_outgoing_handshake(QIOTask *task,
QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
Error *err = NULL;
- qio_task_propagate_error(task, &err);
+ if (qio_task_propagate_error(task, &err)) {
+ trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
+ } else {
+ trace_multifd_tls_outgoing_handshake_complete(ioc);
+ }
multifd_channel_connect(p, ioc, err);
}
@@ -749,6 +753,7 @@ static void multifd_tls_channel_connect(MultiFDSendParams *p,
return;
}
+ trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
qio_channel_tls_handshake(tioc,
multifd_tls_outgoing_handshake,
@@ -764,6 +769,9 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
{
MigrationState *s = p->s;
+ trace_multifd_set_outgoing_channel(
+ ioc, object_get_typename(OBJECT(ioc)), s->hostname, error);
+
if (!error) {
if (s->parameters.tls_creds &&
*s->parameters.tls_creds &&
diff --git a/migration/trace-events b/migration/trace-events
index 7ba2fa6..faf52a8 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -129,6 +129,10 @@ multifd_send_sync_main_wait(uint8_t id) "channel %d"
multifd_send_terminate_threads(bool error) "error %d"
multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t pages) "channel %d packets %" PRIu64 " pages %" PRIu64
multifd_send_thread_start(uint8_t id) "%d"
+multifd_tls_outgoing_handshake_start(void *ioc, void *tioc, const char *hostname) "ioc=%p tioc=%p hostname=%s"
+multifd_tls_outgoing_handshake_error(void *ioc, const char *err) "ioc=%p err=%s"
+multifd_tls_outgoing_handshake_complete(void *ioc, void) "ioc=%p"
+multifd_set_outgoing_channel(void *ioc, const char *ioctype, const char *hostname, void *err) "ioc=%p ioctype=%s hostname=%s err=%p"
# migration.c
await_return_path_close_on_source_close(void) ""
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 6/6] migration/tls: add trace points for multifd-tls
2020-09-13 2:47 ` [PATCH v3 6/6] migration/tls: add trace points for multifd-tls Chuan Zheng
@ 2020-09-14 8:55 ` Daniel P. Berrangé
0 siblings, 0 replies; 14+ messages in thread
From: Daniel P. Berrangé @ 2020-09-14 8:55 UTC (permalink / raw)
To: Chuan Zheng
Cc: zhang.zhanghailiang, quintela, yuxiating, dgilbert, xiexiangyou,
qemu-devel, alex.chen, jinyan12
On Sun, Sep 13, 2020 at 10:47:36AM +0800, Chuan Zheng wrote:
> add trace points for multifd-tls for debug.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: Yan Jin <jinyan12@huawei.com>
> ---
> migration/multifd.c | 10 +++++++++-
> migration/trace-events | 4 ++++
> 2 files changed, 13 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 14+ messages in thread