RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs
accept new wire protocol of MigrateAddress struct.
It is achived by parsing 'uri' string and storing migration parameters
required for RDMA connection into well defined InetSocketAddress struct.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/migration.c | 8 ++++----
migration/rdma.c | 38 ++++++++++++++++----------------------
migration/rdma.h | 6 ++++--
3 files changed, 24 insertions(+), 28 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 61f52d2f90..b7c3b939d5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -480,8 +480,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
fd_start_incoming_migration(saddr->u.fd.str, &local_err);
}
#ifdef CONFIG_RDMA
- } else if (strstart(uri, "rdma:", &p)) {
- rdma_start_incoming_migration(p, errp);
+ } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
+ rdma_start_incoming_migration(&channel->u.rdma, &local_err);
#endif
} else if (strstart(uri, "exec:", &p)) {
exec_start_incoming_migration(p, errp);
@@ -1737,8 +1737,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
}
#ifdef CONFIG_RDMA
- } else if (strstart(uri, "rdma:", &p)) {
- rdma_start_outgoing_migration(s, p, &local_err);
+ } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
+ rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
#endif
} else if (strstart(uri, "exec:", &p)) {
exec_start_outgoing_migration(s, p, &local_err);
diff --git a/migration/rdma.c b/migration/rdma.c
index 2cd8f1cc66..32b4b8099e 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -319,7 +319,6 @@ typedef struct RDMALocalBlocks {
typedef struct RDMAContext {
char *host;
int port;
- char *host_port;
RDMAWorkRequestData wr_data[RDMA_WRID_MAX];
@@ -2455,9 +2454,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
rdma->channel = NULL;
}
g_free(rdma->host);
- g_free(rdma->host_port);
rdma->host = NULL;
- rdma->host_port = NULL;
}
@@ -2739,28 +2736,17 @@ static void qemu_rdma_return_path_dest_init(RDMAContext *rdma_return_path,
rdma_return_path->is_return_path = true;
}
-static void *qemu_rdma_data_init(const char *host_port, Error **errp)
+static void *qemu_rdma_data_init(InetSocketAddress *saddr, Error **errp)
{
RDMAContext *rdma = NULL;
- InetSocketAddress *addr;
- if (host_port) {
+ if (saddr) {
rdma = g_new0(RDMAContext, 1);
rdma->current_index = -1;
rdma->current_chunk = -1;
- addr = g_new(InetSocketAddress, 1);
- if (!inet_parse(addr, host_port, NULL)) {
- rdma->port = atoi(addr->port);
- rdma->host = g_strdup(addr->host);
- rdma->host_port = g_strdup(host_port);
- } else {
- ERROR(errp, "bad RDMA migration address '%s'", host_port);
- g_free(rdma);
- rdma = NULL;
- }
-
- qapi_free_InetSocketAddress(addr);
+ rdma->host = g_strdup(saddr->host);
+ rdma->port = atoi(saddr->port);
}
return rdma;
@@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
.private_data_len = sizeof(cap),
};
RDMAContext *rdma_return_path = NULL;
+ InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
struct rdma_cm_event *cm_event;
struct ibv_context *verbs;
int ret = -EINVAL;
int idx;
+ char arr[8];
ret = rdma_get_cm_event(rdma->channel, &cm_event);
if (ret) {
@@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
goto err_rdma_dest_wait;
}
+ isock->host = rdma->host;
+ sprintf(arr,"%d", rdma->port);
+ isock->port = arr;
+
/*
* initialize the RDMAContext for return path for postcopy after first
* connection request reached.
*/
if ((migrate_postcopy() || migrate_return_path())
&& !rdma->is_return_path) {
- rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
+ rdma_return_path = qemu_rdma_data_init(isock, NULL);
if (rdma_return_path == NULL) {
rdma_ack_cm_event(cm_event);
goto err_rdma_dest_wait;
@@ -3506,6 +3498,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
err_rdma_dest_wait:
rdma->error_state = ret;
qemu_rdma_cleanup(rdma);
+ qapi_free_InetSocketAddress(isock);
+ g_free(arr);
g_free(rdma_return_path);
return ret;
}
@@ -4114,7 +4108,8 @@ static void rdma_accept_incoming_migration(void *opaque)
}
}
-void rdma_start_incoming_migration(const char *host_port, Error **errp)
+void rdma_start_incoming_migration(InetSocketAddress *host_port,
+ Error **errp)
{
int ret;
RDMAContext *rdma;
@@ -4160,13 +4155,12 @@ err:
error_propagate(errp, local_err);
if (rdma) {
g_free(rdma->host);
- g_free(rdma->host_port);
}
g_free(rdma);
}
void rdma_start_outgoing_migration(void *opaque,
- const char *host_port, Error **errp)
+ InetSocketAddress *host_port, Error **errp)
{
MigrationState *s = opaque;
RDMAContext *rdma_return_path = NULL;
diff --git a/migration/rdma.h b/migration/rdma.h
index de2ba09dc5..ee89296555 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -14,12 +14,14 @@
*
*/
+#include "qemu/sockets.h"
+
#ifndef QEMU_MIGRATION_RDMA_H
#define QEMU_MIGRATION_RDMA_H
-void rdma_start_outgoing_migration(void *opaque, const char *host_port,
+void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *host_port,
Error **errp);
-void rdma_start_incoming_migration(const char *host_port, Error **errp);
+void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
#endif
--
2.22.3On Fri, May 12, 2023 at 02:32:36PM +0000, Het Gala wrote:
> RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs
> accept new wire protocol of MigrateAddress struct.
>
> It is achived by parsing 'uri' string and storing migration parameters
> required for RDMA connection into well defined InetSocketAddress struct.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
> migration/migration.c | 8 ++++----
> migration/rdma.c | 38 ++++++++++++++++----------------------
> migration/rdma.h | 6 ++++--
> 3 files changed, 24 insertions(+), 28 deletions(-)
>
> @@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> .private_data_len = sizeof(cap),
> };
> RDMAContext *rdma_return_path = NULL;
> + InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> struct rdma_cm_event *cm_event;
> struct ibv_context *verbs;
> int ret = -EINVAL;
> int idx;
> + char arr[8];
>
> ret = rdma_get_cm_event(rdma->channel, &cm_event);
> if (ret) {
> @@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> goto err_rdma_dest_wait;
> }
>
> + isock->host = rdma->host;
> + sprintf(arr,"%d", rdma->port);
> + isock->port = arr;
While Inet ports are 16-bit, and so 65535 fits in a char[8], nothing
at the QAPI parser level is enforcing this.
IOW, someone can pass QEMU a QAPI config with port = 235252353253253253232
and casue this sprintf to smash the stack.
Also this is assigning a stack variable to isock->port which
expects a heap variable. qapi_free_InetSocketAddress() will
call free(isock->port) which will again crash.
Just do
g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
isock->port = g_strdup_printf("%d", rdma->port);
> +
> /*
> * initialize the RDMAContext for return path for postcopy after first
> * connection request reached.
> */
> if ((migrate_postcopy() || migrate_return_path())
> && !rdma->is_return_path) {
> - rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
> + rdma_return_path = qemu_rdma_data_init(isock, NULL);
> if (rdma_return_path == NULL) {
> rdma_ack_cm_event(cm_event);
> goto err_rdma_dest_wait;
> @@ -3506,6 +3498,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> err_rdma_dest_wait:
> rdma->error_state = ret;
> qemu_rdma_cleanup(rdma);
> + qapi_free_InetSocketAddress(isock);
> + g_free(arr);
Free'ing a stack variable
> g_free(rdma_return_path);
> return ret;
> }
> @@ -4114,7 +4108,8 @@ static void rdma_accept_incoming_migration(void *opaque)
> }
> }
>
> -void rdma_start_incoming_migration(const char *host_port, Error **errp)
> +void rdma_start_incoming_migration(InetSocketAddress *host_port,
> + Error **errp)
> {
> int ret;
> RDMAContext *rdma;
> @@ -4160,13 +4155,12 @@ err:
> error_propagate(errp, local_err);
> if (rdma) {
> g_free(rdma->host);
> - g_free(rdma->host_port);
> }
> g_free(rdma);
> }
>
> void rdma_start_outgoing_migration(void *opaque,
> - const char *host_port, Error **errp)
> + InetSocketAddress *host_port, Error **errp)
> {
> MigrationState *s = opaque;
> RDMAContext *rdma_return_path = NULL;
> diff --git a/migration/rdma.h b/migration/rdma.h
> index de2ba09dc5..ee89296555 100644
> --- a/migration/rdma.h
> +++ b/migration/rdma.h
> @@ -14,12 +14,14 @@
> *
> */
>
> +#include "qemu/sockets.h"
> +
> #ifndef QEMU_MIGRATION_RDMA_H
> #define QEMU_MIGRATION_RDMA_H
>
> -void rdma_start_outgoing_migration(void *opaque, const char *host_port,
> +void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *host_port,
> Error **errp);
>
> -void rdma_start_incoming_migration(const char *host_port, Error **errp);
> +void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
>
> #endif
> --
> 2.22.3
>
With 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 :|
On 15/05/23 3:54 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:36PM +0000, Het Gala wrote:
>> RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs
>> accept new wire protocol of MigrateAddress struct.
>>
>> It is achived by parsing 'uri' string and storing migration parameters
>> required for RDMA connection into well defined InetSocketAddress struct.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>> migration/migration.c | 8 ++++----
>> migration/rdma.c | 38 ++++++++++++++++----------------------
>> migration/rdma.h | 6 ++++--
>> 3 files changed, 24 insertions(+), 28 deletions(-)
>>
>> @@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>> .private_data_len = sizeof(cap),
>> };
>> RDMAContext *rdma_return_path = NULL;
>> + InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>> struct rdma_cm_event *cm_event;
>> struct ibv_context *verbs;
>> int ret = -EINVAL;
>> int idx;
>> + char arr[8];
>>
>> ret = rdma_get_cm_event(rdma->channel, &cm_event);
>> if (ret) {
>> @@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>> goto err_rdma_dest_wait;
>> }
>>
>> + isock->host = rdma->host;
>> + sprintf(arr,"%d", rdma->port);
>> + isock->port = arr;
> While Inet ports are 16-bit, and so 65535 fits in a char[8], nothing
> at the QAPI parser level is enforcing this.
>
> IOW, someone can pass QEMU a QAPI config with port = 235252353253253253232
> and casue this sprintf to smash the stack.
>
> Also this is assigning a stack variable to isock->port which
> expects a heap variable. qapi_free_InetSocketAddress() will
> call free(isock->port) which will again crash.
>
> Just do
>
> g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
>
> isock->port = g_strdup_printf("%d", rdma->port);
Thanks Daniel. Will change this in next version of patchset. Is a
protection for isock->host and isock->port needed here ?
>> +
>> /*
>> * initialize the RDMAContext for return path for postcopy after first
>> * connection request reached.
>> */
>> if ((migrate_postcopy() || migrate_return_path())
>> && !rdma->is_return_path) {
>> - rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
>> + rdma_return_path = qemu_rdma_data_init(isock, NULL);
>> if (rdma_return_path == NULL) {
>> rdma_ack_cm_event(cm_event);
>> goto err_rdma_dest_wait;
>> @@ -3506,6 +3498,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>> err_rdma_dest_wait:
>> rdma->error_state = ret;
>> qemu_rdma_cleanup(rdma);
>> + qapi_free_InetSocketAddress(isock);
>> + g_free(arr);
> Free'ing a stack variable
Ack, will delete both statements from here.
>> g_free(rdma_return_path);
>> return ret;
>> }
>> @@ -4114,7 +4108,8 @@ static void rdma_accept_incoming_migration(void *opaque)
>> }
>> }
>>
>> -void rdma_start_incoming_migration(const char *host_port, Error **errp)
>> +void rdma_start_incoming_migration(InetSocketAddress *host_port,
>> + Error **errp)
>> {
>> int ret;
>> RDMAContext *rdma;
>> @@ -4160,13 +4155,12 @@ err:
>> error_propagate(errp, local_err);
>> if (rdma) {
>> g_free(rdma->host);
>> - g_free(rdma->host_port);
>> }
>> g_free(rdma);
>> }
>>
>> void rdma_start_outgoing_migration(void *opaque,
>> - const char *host_port, Error **errp)
>> + InetSocketAddress *host_port, Error **errp)
>> {
>> MigrationState *s = opaque;
>> RDMAContext *rdma_return_path = NULL;
>> diff --git a/migration/rdma.h b/migration/rdma.h
>> index de2ba09dc5..ee89296555 100644
>> --- a/migration/rdma.h
>> +++ b/migration/rdma.h
>> @@ -14,12 +14,14 @@
>> *
>> */
>>
>> +#include "qemu/sockets.h"
>> +
>> #ifndef QEMU_MIGRATION_RDMA_H
>> #define QEMU_MIGRATION_RDMA_H
>>
>> -void rdma_start_outgoing_migration(void *opaque, const char *host_port,
>> +void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *host_port,
>> Error **errp);
>>
>> -void rdma_start_incoming_migration(const char *host_port, Error **errp);
>> +void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
>>
>> #endif
>> --
>> 2.22.3
>>
> With regards,
> Daniel
Regards,
Het Gala
On Mon, May 15, 2023 at 08:08:57PM +0530, Het Gala wrote:
>
> On 15/05/23 3:54 pm, Daniel P. Berrangé wrote:
> > On Fri, May 12, 2023 at 02:32:36PM +0000, Het Gala wrote:
> > > RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs
> > > accept new wire protocol of MigrateAddress struct.
> > >
> > > It is achived by parsing 'uri' string and storing migration parameters
> > > required for RDMA connection into well defined InetSocketAddress struct.
> > >
> > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > > ---
> > > migration/migration.c | 8 ++++----
> > > migration/rdma.c | 38 ++++++++++++++++----------------------
> > > migration/rdma.h | 6 ++++--
> > > 3 files changed, 24 insertions(+), 28 deletions(-)
> > >
> > > @@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> > > .private_data_len = sizeof(cap),
> > > };
> > > RDMAContext *rdma_return_path = NULL;
> > > + InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> > > struct rdma_cm_event *cm_event;
> > > struct ibv_context *verbs;
> > > int ret = -EINVAL;
> > > int idx;
> > > + char arr[8];
> > > ret = rdma_get_cm_event(rdma->channel, &cm_event);
> > > if (ret) {
> > > @@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> > > goto err_rdma_dest_wait;
> > > }
> > > + isock->host = rdma->host;
> > > + sprintf(arr,"%d", rdma->port);
> > > + isock->port = arr;
> > While Inet ports are 16-bit, and so 65535 fits in a char[8], nothing
> > at the QAPI parser level is enforcing this.
> >
> > IOW, someone can pass QEMU a QAPI config with port = 235252353253253253232
> > and casue this sprintf to smash the stack.
> >
> > Also this is assigning a stack variable to isock->port which
> > expects a heap variable. qapi_free_InetSocketAddress() will
> > call free(isock->port) which will again crash.
> >
> > Just do
> >
> > g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
> >
> > isock->port = g_strdup_printf("%d", rdma->port);
> Thanks Daniel. Will change this in next version of patchset. Is a protection
> for isock->host and isock->port needed here ?
This will be validated later by getaddrinfo() so IMHO QEMU doesn't
need todo anythgin
With 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 :|
On 15/05/23 8:28 pm, Daniel P. Berrangé wrote:
> On Mon, May 15, 2023 at 08:08:57PM +0530, Het Gala wrote:
>> On 15/05/23 3:54 pm, Daniel P. Berrangé wrote:
>>> On Fri, May 12, 2023 at 02:32:36PM +0000, Het Gala wrote:
>>>> RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs
>>>> accept new wire protocol of MigrateAddress struct.
>>>>
>>>> It is achived by parsing 'uri' string and storing migration parameters
>>>> required for RDMA connection into well defined InetSocketAddress struct.
>>>>
>>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>>> ---
>>>> migration/migration.c | 8 ++++----
>>>> migration/rdma.c | 38 ++++++++++++++++----------------------
>>>> migration/rdma.h | 6 ++++--
>>>> 3 files changed, 24 insertions(+), 28 deletions(-)
>>>>
>>>> @@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>>>> .private_data_len = sizeof(cap),
>>>> };
>>>> RDMAContext *rdma_return_path = NULL;
>>>> + InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>>>> struct rdma_cm_event *cm_event;
>>>> struct ibv_context *verbs;
>>>> int ret = -EINVAL;
>>>> int idx;
>>>> + char arr[8];
>>>> ret = rdma_get_cm_event(rdma->channel, &cm_event);
>>>> if (ret) {
>>>> @@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>>>> goto err_rdma_dest_wait;
>>>> }
>>>> + isock->host = rdma->host;
>>>> + sprintf(arr,"%d", rdma->port);
>>>> + isock->port = arr;
>>> While Inet ports are 16-bit, and so 65535 fits in a char[8], nothing
>>> at the QAPI parser level is enforcing this.
>>>
>>> IOW, someone can pass QEMU a QAPI config with port = 235252353253253253232
>>> and casue this sprintf to smash the stack.
>>>
>>> Also this is assigning a stack variable to isock->port which
>>> expects a heap variable. qapi_free_InetSocketAddress() will
>>> call free(isock->port) which will again crash.
>>>
>>> Just do
>>>
>>> g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
>>>
>>> isock->port = g_strdup_printf("%d", rdma->port);
>> Thanks Daniel. Will change this in next version of patchset. Is a protection
>> for isock->host and isock->port needed here ?
> This will be validated later by getaddrinfo() so IMHO QEMU doesn't
> need todo anythgin
Yes. I will keep it as it is for now. Thanks
> With regards,
> Daniel
Regards,
Het Gala
Het Gala <het.gala@nutanix.com> wrote: > RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs > accept new wire protocol of MigrateAddress struct. > > It is achived by parsing 'uri' string and storing migration parameters > required for RDMA connection into well defined InetSocketAddress struct. > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
© 2016 - 2026 Red Hat, Inc.