| Message ID | 20200228092420.103757-4-quintela@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/15] multifd: Add multifd-compression parameter | expand |
On Fri, 28 Feb 2020 at 09:26, Juan Quintela <quintela@redhat.com> wrote: > > It will be used later. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Hi; Coverity thinks there might be a buffer overrun here. It's probably wrong, but it's not completely obvious why it can't happen, so an assert somewhere would help... (This is CID 1487239.) > +MultiFDCompression migrate_multifd_compression(void) > +{ > + MigrationState *s; > + > + s = migrate_get_current(); > + > + return s->parameters.multifd_compression; This function returns an enum of type MultiFDCompression, whose (autogenerated from QAPI) definition is: typedef enum MultiFDCompression { MULTIFD_COMPRESSION_NONE, MULTIFD_COMPRESSION_ZLIB, #if defined(CONFIG_ZSTD) MULTIFD_COMPRESSION_ZSTD, #endif /* defined(CONFIG_ZSTD) */ MULTIFD_COMPRESSION__MAX, } MultiFDCompression; > @@ -604,6 +745,7 @@ int multifd_save_setup(Error **errp) > multifd_send_state->pages = multifd_pages_init(page_count); > qemu_sem_init(&multifd_send_state->channels_ready, 0); > atomic_set(&multifd_send_state->exiting, 0); > + multifd_send_state->ops = multifd_ops[migrate_multifd_compression()]; Here we take the result of the function and use it as an array index into multifd_ops, whose definition is static MultiFDMethods *multifd_ops[MULTIFD_COMPRESSION__MAX] = { ... } Coverity doesn't see any reason why the return value from migrate_multifd_compression() can't be MULTIFD_COMPRESSION__MAX, so it complains that if it is then we are going to index off the end of the array. An assert in migrate_multifd_compression() that the value being returned is within the expected range would probably placate it. Alternatively, if the qapi type codegen didn't put the __MAX value as a possible value of the enum type then Coverity and probably also the compiler wouldn't consider it to be a possible value of this kind of variable. But that might have other awkward side-effects. thanks -- PMM
Ping! Any opinions, especially from qapi codegen people, on the right thing to do here? On Tue, 12 Apr 2022 at 20:04, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 28 Feb 2020 at 09:26, Juan Quintela <quintela@redhat.com> wrote: > > > > It will be used later. > > > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > Hi; Coverity thinks there might be a buffer overrun here. > It's probably wrong, but it's not completely obvious why > it can't happen, so an assert somewhere would help... > (This is CID 1487239.) > > > +MultiFDCompression migrate_multifd_compression(void) > > +{ > > + MigrationState *s; > > + > > + s = migrate_get_current(); > > + > > + return s->parameters.multifd_compression; > > This function returns an enum of type MultiFDCompression, > whose (autogenerated from QAPI) definition is: > > typedef enum MultiFDCompression { > MULTIFD_COMPRESSION_NONE, > MULTIFD_COMPRESSION_ZLIB, > #if defined(CONFIG_ZSTD) > MULTIFD_COMPRESSION_ZSTD, > #endif /* defined(CONFIG_ZSTD) */ > MULTIFD_COMPRESSION__MAX, > } MultiFDCompression; > > > @@ -604,6 +745,7 @@ int multifd_save_setup(Error **errp) > > multifd_send_state->pages = multifd_pages_init(page_count); > > qemu_sem_init(&multifd_send_state->channels_ready, 0); > > atomic_set(&multifd_send_state->exiting, 0); > > + multifd_send_state->ops = multifd_ops[migrate_multifd_compression()]; > > Here we take the result of the function and use it as an > array index into multifd_ops, whose definition is > static MultiFDMethods *multifd_ops[MULTIFD_COMPRESSION__MAX] = { ... } > > Coverity doesn't see any reason why the return value from > migrate_multifd_compression() can't be MULTIFD_COMPRESSION__MAX, > so it complains that if it is then we are going to index off the > end of the array. > > An assert in migrate_multifd_compression() that the value being > returned is within the expected range would probably placate it. > > Alternatively, if the qapi type codegen didn't put the __MAX > value as a possible value of the enum type then Coverity > and probably also the compiler wouldn't consider it to be > a possible value of this kind of variable. But that might > have other awkward side-effects. > > thanks > -- PMM
Ping^2! I remain unsure how we should resolve this Coverity complaint and would like opinions from QAPI codegen people. thanks -- PMM On Fri, 13 May 2022 at 18:56, Peter Maydell <peter.maydell@linaro.org> wrote: > > Ping! Any opinions, especially from qapi codegen people, > on the right thing to do here? > > On Tue, 12 Apr 2022 at 20:04, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Fri, 28 Feb 2020 at 09:26, Juan Quintela <quintela@redhat.com> wrote: > > > > > > It will be used later. > > > > > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > > > > Hi; Coverity thinks there might be a buffer overrun here. > > It's probably wrong, but it's not completely obvious why > > it can't happen, so an assert somewhere would help... > > (This is CID 1487239.) > > > > > +MultiFDCompression migrate_multifd_compression(void) > > > +{ > > > + MigrationState *s; > > > + > > > + s = migrate_get_current(); > > > + > > > + return s->parameters.multifd_compression; > > > > This function returns an enum of type MultiFDCompression, > > whose (autogenerated from QAPI) definition is: > > > > typedef enum MultiFDCompression { > > MULTIFD_COMPRESSION_NONE, > > MULTIFD_COMPRESSION_ZLIB, > > #if defined(CONFIG_ZSTD) > > MULTIFD_COMPRESSION_ZSTD, > > #endif /* defined(CONFIG_ZSTD) */ > > MULTIFD_COMPRESSION__MAX, > > } MultiFDCompression; > > > > > @@ -604,6 +745,7 @@ int multifd_save_setup(Error **errp) > > > multifd_send_state->pages = multifd_pages_init(page_count); > > > qemu_sem_init(&multifd_send_state->channels_ready, 0); > > > atomic_set(&multifd_send_state->exiting, 0); > > > + multifd_send_state->ops = multifd_ops[migrate_multifd_compression()]; > > > > Here we take the result of the function and use it as an > > array index into multifd_ops, whose definition is > > static MultiFDMethods *multifd_ops[MULTIFD_COMPRESSION__MAX] = { ... } > > > > Coverity doesn't see any reason why the return value from > > migrate_multifd_compression() can't be MULTIFD_COMPRESSION__MAX, > > so it complains that if it is then we are going to index off the > > end of the array. > > > > An assert in migrate_multifd_compression() that the value being > > returned is within the expected range would probably placate it. > > > > Alternatively, if the qapi type codegen didn't put the __MAX > > value as a possible value of the enum type then Coverity > > and probably also the compiler wouldn't consider it to be > > a possible value of this kind of variable. But that might > > have other awkward side-effects. > > > > thanks > > -- PMM
This fell through the cracks. My apologies. Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 28 Feb 2020 at 09:26, Juan Quintela <quintela@redhat.com> wrote: >> >> It will be used later. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> > > Hi; Coverity thinks there might be a buffer overrun here. > It's probably wrong, It is; details below. > but it's not completely obvious why > it can't happen, so an assert somewhere would help... > (This is CID 1487239.) > >> +MultiFDCompression migrate_multifd_compression(void) >> +{ >> + MigrationState *s; >> + >> + s = migrate_get_current(); >> + >> + return s->parameters.multifd_compression; > > This function returns an enum of type MultiFDCompression, > whose (autogenerated from QAPI) definition is: > > typedef enum MultiFDCompression { > MULTIFD_COMPRESSION_NONE, > MULTIFD_COMPRESSION_ZLIB, > #if defined(CONFIG_ZSTD) > MULTIFD_COMPRESSION_ZSTD, > #endif /* defined(CONFIG_ZSTD) */ > MULTIFD_COMPRESSION__MAX, > } MultiFDCompression; > Generated from { 'enum': 'MultiFDCompression', 'data': [ 'none', 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] } >> @@ -604,6 +745,7 @@ int multifd_save_setup(Error **errp) >> multifd_send_state->pages = multifd_pages_init(page_count); >> qemu_sem_init(&multifd_send_state->channels_ready, 0); >> atomic_set(&multifd_send_state->exiting, 0); >> + multifd_send_state->ops = multifd_ops[migrate_multifd_compression()]; > > Here we take the result of the function and use it as an > array index into multifd_ops, whose definition is > static MultiFDMethods *multifd_ops[MULTIFD_COMPRESSION__MAX] = { ... } > > Coverity doesn't see any reason why the return value from > migrate_multifd_compression() can't be MULTIFD_COMPRESSION__MAX, > so it complains that if it is then we are going to index off the > end of the array. Yes. migrate_multifd_compression() returns current_migration->parameters.multifd_compression. .multifd_compression is zero-initialized to MULTIFD_COMPRESSION_NONE, and modified only by qmp_migrate_set_parameters(). qmp_migrate_set_parameters() can be called on behalf of QMP command migrate-set-parameters, and on behalf of HMP command migrate_set_parameter. In either case, the value is the result of parsing a string with qapi_enum_parse(), via visit_type_enum() and visit_type_MultiFDCompression() with an input visitor (qobject for QMP, string for HMP). Never assigns the enum's __MAX. > An assert in migrate_multifd_compression() that the value being > returned is within the expected range would probably placate it. Yes. > Alternatively, if the qapi type codegen didn't put the __MAX > value as a possible value of the enum type then Coverity > and probably also the compiler wouldn't consider it to be > a possible value of this kind of variable. But that might > have other awkward side-effects. Yes. Coding the __MAX as a member of the enum is easy to write and easy to understand, but gets in the way in places. We could do something like typedef enum MultiFDCompression { MULTIFD_COMPRESSION_NONE, MULTIFD_COMPRESSION_ZLIB, #if defined(CONFIG_ZSTD) MULTIFD_COMPRESSION_ZSTD, #endif /* defined(CONFIG_ZSTD) */ } MultiFDCompression; #define MULTIFD_COMPRESSION__MAX ??? where ??? is 3 if defined(CONFIG_ZSTD), else 2. The more conditionals, the more awkward this gets. The alternative is holes in the enum, like this: typedef enum MultiFDCompression { MULTIFD_COMPRESSION_NONE = 0, MULTIFD_COMPRESSION_ZLIB = 1, #if defined(CONFIG_ZSTD) MULTIFD_COMPRESSION_ZSTD = 2, #endif /* defined(CONFIG_ZSTD) */ } MultiFDCompression; #define MULTIFD_COMPRESSION__MAX 3 Also puts holes into the lookup tables. We'd need to review code to make sure we're not breaking "no holes" assumptions. Changing the __MAX from enum member to macro could conceivably break something, too. The quick fix is an assertion.
diff --git a/migration/migration.c b/migration/migration.c index bc744d1734..eb7b0a2df0 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2245,6 +2245,15 @@ int migrate_multifd_channels(void) return s->parameters.multifd_channels; } +MultiFDCompression migrate_multifd_compression(void) +{ + MigrationState *s; + + s = migrate_get_current(); + + return s->parameters.multifd_compression; +} + int migrate_use_xbzrle(void) { MigrationState *s; diff --git a/migration/migration.h b/migration/migration.h index 8473ddfc88..59fea02482 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -300,6 +300,7 @@ bool migrate_auto_converge(void); bool migrate_use_multifd(void); bool migrate_pause_before_switchover(void); int migrate_multifd_channels(void); +MultiFDCompression migrate_multifd_compression(void); int migrate_use_xbzrle(void); int64_t migrate_xbzrle_cache_size(void); diff --git a/migration/multifd.c b/migration/multifd.c index b3e8ae9bcc..97433e5135 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -38,6 +38,134 @@ typedef struct { uint64_t unused2[4]; /* Reserved for future use */ } __attribute__((packed)) MultiFDInit_t; +/* Multifd without compression */ + +/** + * nocomp_send_setup: setup send side + * + * For no compression this function does nothing. + * + * Returns 0 for success or -1 for error + * + * @p: Params for the channel that we are using + * @errp: pointer to an error + */ +static int nocomp_send_setup(MultiFDSendParams *p, Error **errp) +{ + return 0; +} + +/** + * nocomp_send_cleanup: cleanup send side + * + * For no compression this function does nothing. + * + * @p: Params for the channel that we are using + */ +static void nocomp_send_cleanup(MultiFDSendParams *p, Error **errp) +{ + return; +} + +/** + * nocomp_send_prepare: prepare date to be able to send + * + * For no compression we just have to calculate the size of the + * packet. + * + * Returns 0 for success or -1 for error + * + * @p: Params for the channel that we are using + * @used: number of pages used + * @errp: pointer to an error + */ +static int nocomp_send_prepare(MultiFDSendParams *p, uint32_t used, + Error **errp) +{ + p->next_packet_size = used * qemu_target_page_size(); + p->flags |= MULTIFD_FLAG_NOCOMP; + return 0; +} + +/** + * nocomp_send_write: do the actual write of the data + * + * For no compression we just have to write the data. + * + * Returns 0 for success or -1 for error + * + * @p: Params for the channel that we are using + * @used: number of pages used + * @errp: pointer to an error + */ +static int nocomp_send_write(MultiFDSendParams *p, uint32_t used, Error **errp) +{ + return qio_channel_writev_all(p->c, p->pages->iov, used, errp); +} + +/** + * nocomp_recv_setup: setup receive side + * + * For no compression this function does nothing. + * + * Returns 0 for success or -1 for error + * + * @p: Params for the channel that we are using + * @errp: pointer to an error + */ +static int nocomp_recv_setup(MultiFDRecvParams *p, Error **errp) +{ + return 0; +} + +/** + * nocomp_recv_cleanup: setup receive side + * + * For no compression this function does nothing. + * + * @p: Params for the channel that we are using + */ +static void nocomp_recv_cleanup(MultiFDRecvParams *p) +{ +} + +/** + * nocomp_recv_pages: read the data from the channel into actual pages + * + * For no compression we just need to read things into the correct place. + * + * Returns 0 for success or -1 for error + * + * @p: Params for the channel that we are using + * @used: number of pages used + * @errp: pointer to an error + */ +static int nocomp_recv_pages(MultiFDRecvParams *p, uint32_t used, Error **errp) +{ + uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK; + + if (flags != MULTIFD_FLAG_NOCOMP) { + error_setg(errp, "multifd %d: flags received %x flags expected %x", + p->id, flags, MULTIFD_FLAG_NOCOMP); + return -1; + } + return qio_channel_readv_all(p->c, p->pages->iov, used, errp); +} + +static MultiFDMethods multifd_nocomp_ops = { + .send_setup = nocomp_send_setup, + .send_cleanup = nocomp_send_cleanup, + .send_prepare = nocomp_send_prepare, + .send_write = nocomp_send_write, + .recv_setup = nocomp_recv_setup, + .recv_cleanup = nocomp_recv_cleanup, + .recv_pages = nocomp_recv_pages +}; + +static MultiFDMethods *multifd_ops[MULTIFD_COMPRESSION__MAX] = { + [MULTIFD_COMPRESSION_NONE] = &multifd_nocomp_ops, +}; + static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp) { MultiFDInit_t msg = {}; @@ -246,6 +374,8 @@ struct { * We will use atomic operations. Only valid values are 0 and 1. */ int exiting; + /* multifd ops */ + MultiFDMethods *ops; } *multifd_send_state; /* @@ -397,6 +527,7 @@ void multifd_save_cleanup(void) } for (i = 0; i < migrate_multifd_channels(); i++) { MultiFDSendParams *p = &multifd_send_state->params[i]; + Error *local_err = NULL; socket_send_channel_destroy(p->c); p->c = NULL; @@ -410,6 +541,10 @@ void multifd_save_cleanup(void) p->packet_len = 0; g_free(p->packet); p->packet = NULL; + multifd_send_state->ops->send_cleanup(p, &local_err); + if (local_err) { + migrate_set_error(migrate_get_current(), local_err); + } } qemu_sem_destroy(&multifd_send_state->channels_ready); g_free(multifd_send_state->params); @@ -494,7 +629,14 @@ static void *multifd_send_thread(void *opaque) uint64_t packet_num = p->packet_num; flags = p->flags; - p->next_packet_size = used * qemu_target_page_size(); + if (used) { + ret = multifd_send_state->ops->send_prepare(p, used, + &local_err); + if (ret != 0) { + qemu_mutex_unlock(&p->mutex); + break; + } + } multifd_send_fill_packet(p); p->flags = 0; p->num_packets++; @@ -513,8 +655,7 @@ static void *multifd_send_thread(void *opaque) } if (used) { - ret = qio_channel_writev_all(p->c, p->pages->iov, - used, &local_err); + ret = multifd_send_state->ops->send_write(p, used, &local_err); if (ret != 0) { break; } @@ -604,6 +745,7 @@ int multifd_save_setup(Error **errp) multifd_send_state->pages = multifd_pages_init(page_count); qemu_sem_init(&multifd_send_state->channels_ready, 0); atomic_set(&multifd_send_state->exiting, 0); + multifd_send_state->ops = multifd_ops[migrate_multifd_compression()]; for (i = 0; i < thread_count; i++) { MultiFDSendParams *p = &multifd_send_state->params[i]; @@ -623,6 +765,18 @@ int multifd_save_setup(Error **errp) p->name = g_strdup_printf("multifdsend_%d", i); socket_send_channel_create(multifd_new_send_channel_async, p); } + + for (i = 0; i < thread_count; i++) { + MultiFDSendParams *p = &multifd_send_state->params[i]; + Error *local_err = NULL; + int ret; + + ret = multifd_send_state->ops->send_setup(p, &local_err); + if (ret) { + error_propagate(errp, local_err); + return ret; + } + } return 0; } @@ -634,6 +788,8 @@ struct { QemuSemaphore sem_sync; /* global number of generated multifd packets */ uint64_t packet_num; + /* multifd ops */ + MultiFDMethods *ops; } *multifd_recv_state; static void multifd_recv_terminate_threads(Error *err) @@ -673,7 +829,6 @@ static void multifd_recv_terminate_threads(Error *err) int multifd_load_cleanup(Error **errp) { int i; - int ret = 0; if (!migrate_use_multifd()) { return 0; @@ -706,6 +861,7 @@ int multifd_load_cleanup(Error **errp) p->packet_len = 0; g_free(p->packet); p->packet = NULL; + multifd_recv_state->ops->recv_cleanup(p); } qemu_sem_destroy(&multifd_recv_state->sem_sync); g_free(multifd_recv_state->params); @@ -713,7 +869,7 @@ int multifd_load_cleanup(Error **errp) g_free(multifd_recv_state); multifd_recv_state = NULL; - return ret; + return 0; } void multifd_recv_sync_main(void) @@ -778,6 +934,8 @@ static void *multifd_recv_thread(void *opaque) used = p->pages->used; flags = p->flags; + /* recv methods don't know how to handle the SYNC flag */ + p->flags &= ~MULTIFD_FLAG_SYNC; trace_multifd_recv(p->id, p->packet_num, used, flags, p->next_packet_size); p->num_packets++; @@ -785,8 +943,7 @@ static void *multifd_recv_thread(void *opaque) qemu_mutex_unlock(&p->mutex); if (used) { - ret = qio_channel_readv_all(p->c, p->pages->iov, - used, &local_err); + ret = multifd_recv_state->ops->recv_pages(p, used, &local_err); if (ret != 0) { break; } @@ -825,6 +982,7 @@ int multifd_load_setup(Error **errp) multifd_recv_state->params = g_new0(MultiFDRecvParams, thread_count); atomic_set(&multifd_recv_state->count, 0); qemu_sem_init(&multifd_recv_state->sem_sync, 0); + multifd_recv_state->ops = multifd_ops[migrate_multifd_compression()]; for (i = 0; i < thread_count; i++) { MultiFDRecvParams *p = &multifd_recv_state->params[i]; @@ -839,6 +997,18 @@ int multifd_load_setup(Error **errp) p->packet = g_malloc0(p->packet_len); p->name = g_strdup_printf("multifdrecv_%d", i); } + + for (i = 0; i < thread_count; i++) { + MultiFDRecvParams *p = &multifd_recv_state->params[i]; + Error *local_err = NULL; + int ret; + + ret = multifd_recv_state->ops->recv_setup(p, &local_err); + if (ret) { + error_propagate(errp, local_err); + return ret; + } + } return 0; } @@ -896,4 +1066,3 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp) return atomic_read(&multifd_recv_state->count) == migrate_multifd_channels(); } - diff --git a/migration/multifd.h b/migration/multifd.h index d8b0205977..54075ffa7d 100644 --- a/migration/multifd.h +++ b/migration/multifd.h @@ -25,6 +25,11 @@ int multifd_queue_page(QEMUFile *f, RAMBlock *block, ram_addr_t offset); #define MULTIFD_FLAG_SYNC (1 << 0) +/* We reserve 3 bits for compression methods */ +#define MULTIFD_FLAG_COMPRESSION_MASK (7 << 1) +/* we need to be compatible. Before compression value was 0 */ +#define MULTIFD_FLAG_NOCOMP (0 << 1) + /* This value needs to be a multiple of qemu_target_page_size() */ #define MULTIFD_PACKET_SIZE (512 * 1024) @@ -96,6 +101,8 @@ typedef struct { uint64_t num_pages; /* syncs main thread and channels */ QemuSemaphore sem_sync; + /* used for compression methods */ + void *data; } MultiFDSendParams; typedef struct { @@ -133,7 +140,26 @@ typedef struct { uint64_t num_pages; /* syncs main thread and channels */ QemuSemaphore sem_sync; + /* used for de-compression methods */ + void *data; } MultiFDRecvParams; +typedef struct { + /* Setup for sending side */ + int (*send_setup)(MultiFDSendParams *p, Error **errp); + /* Cleanup for sending side */ + void (*send_cleanup)(MultiFDSendParams *p, Error **errp); + /* Prepare the send packet */ + int (*send_prepare)(MultiFDSendParams *p, uint32_t used, Error **errp); + /* Write the send packet */ + int (*send_write)(MultiFDSendParams *p, uint32_t used, Error **errp); + /* Setup for receiving side */ + int (*recv_setup)(MultiFDRecvParams *p, Error **errp); + /* Cleanup for receiving side */ + void (*recv_cleanup)(MultiFDRecvParams *p); + /* Read all pages */ + int (*recv_pages)(MultiFDRecvParams *p, uint32_t used, Error **errp); +} MultiFDMethods; + #endif diff --git a/migration/ram.c b/migration/ram.c index ed23ed1c7c..73a141bb60 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -43,6 +43,7 @@ #include "page_cache.h" #include "qemu/error-report.h" #include "qapi/error.h" +#include "qapi/qapi-types-migration.h" #include "qapi/qapi-events-migration.h" #include "qapi/qmp/qerror.h" #include "trace.h"