Re: [PATCH v5 18/18] migration/multifd: Stop changing the packet on recv side

[Peter Xu]

On Mon, Aug 26, 2024 at 04:53:22PM -0300, Fabiano Rosas wrote:
> As observed by Philippe, the multifd_ram_unfill_packet() function
> currently leaves the MultiFDPacket structure with mixed
> endianness. This is harmless, but ultimately not very clean. Aside
> from that, the packet is also written to on the recv side to ensure
> the ramblock name is null-terminated.
> 
> Stop touching the received packet and do the necessary work using
> stack variables instead.
> 
> While here tweak the error strings and fix the space before
> semicolons. Also remove the "100 times bigger" comment because it's
> just one possible explanation for a size mismatch and it doesn't even
> match the code.
> 
> CC: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/multifd-nocomp.c | 38 ++++++++++++++++----------------------
>  migration/multifd.c        | 18 ++++++++----------
>  2 files changed, 24 insertions(+), 32 deletions(-)
> 
> diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
> index f294d1b0b2..0cbf1b88e1 100644
> --- a/migration/multifd-nocomp.c
> +++ b/migration/multifd-nocomp.c
> @@ -220,33 +220,29 @@ int multifd_ram_unfill_packet(MultiFDRecvParams *p, 
> Error **errp)
>      MultiFDPacket_t *packet = p->packet;
>      uint32_t page_count = multifd_ram_page_count();
>      uint32_t page_size = multifd_ram_page_size();
> +    uint32_t pages_per_packet = be32_to_cpu(packet->pages_alloc);
> +    const char *ramblock_name;
>      int i;
>  
> -    packet->pages_alloc = be32_to_cpu(packet->pages_alloc);
> -    /*
> -     * If we received a packet that is 100 times bigger than expected
> -     * just stop migration.  It is a magic number.
> -     */
> -    if (packet->pages_alloc > page_count) {
> -        error_setg(errp, "multifd: received packet "
> -                   "with size %u and expected a size of %u",
> -                   packet->pages_alloc, page_count) ;
> +    if (pages_per_packet > page_count) {
> +        error_setg(errp, "multifd: received packet with %u pages, expected 
> %u",
> +                   pages_per_packet, page_count);
>          return -1;
>      }
>  
>      p->normal_num = be32_to_cpu(packet->normal_pages);
> -    if (p->normal_num > packet->pages_alloc) {
> -        error_setg(errp, "multifd: received packet "
> -                   "with %u normal pages and expected maximum pages are %u",
> -                   p->normal_num, packet->pages_alloc) ;
> +    if (p->normal_num > pages_per_packet) {
> +        error_setg(errp, "multifd: received packet with %u non-zero pages, "
> +                   "which exceeds maximum expected pages %u",
> +                   p->normal_num, pages_per_packet);
>          return -1;
>      }
>  
>      p->zero_num = be32_to_cpu(packet->zero_pages);
> -    if (p->zero_num > packet->pages_alloc - p->normal_num) {
> -        error_setg(errp, "multifd: received packet "
> -                   "with %u zero pages and expected maximum zero pages are 
> %u",
> -                   p->zero_num, packet->pages_alloc - p->normal_num) ;
> +    if (p->zero_num > pages_per_packet - p->normal_num) {
> +        error_setg(errp,
> +                   "multifd: received packet with %u zero pages, expected 
> maximum %u",
> +                   p->zero_num, pages_per_packet - p->normal_num);
>          return -1;
>      }
>  
> @@ -254,12 +250,10 @@ int multifd_ram_unfill_packet(MultiFDRecvParams *p, 
> Error **errp)
>          return 0;
>      }
>  
> -    /* make sure that ramblock is 0 terminated */
> -    packet->ramblock[255] = 0;
> -    p->block = qemu_ram_block_by_name(packet->ramblock);
> +    ramblock_name = g_strndup(packet->ramblock, 255);

This one is leaked?

IMHO the "temp var for endianess" is better justified than this specific
one, where I think always null-terminating the packet->ramblock[] doesn't
sound too bad - it makes sure all future ref to packet->ramblock is safe.

> +    p->block = qemu_ram_block_by_name(ramblock_name);
>      if (!p->block) {
> -        error_setg(errp, "multifd: unknown ram block %s",
> -                   packet->ramblock);
> +        error_setg(errp, "multifd: unknown ram block %s", ramblock_name);
>          return -1;
>      }
>  
> diff --git a/migration/multifd.c b/migration/multifd.c
> index b89715fdc2..256ecdea56 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -231,21 +231,19 @@ void multifd_send_fill_packet(MultiFDSendParams *p)
>  static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>  {
>      MultiFDPacket_t *packet = p->packet;
> +    uint32_t magic = be32_to_cpu(packet->magic);
> +    uint32_t version = be32_to_cpu(packet->version);
>      int ret = 0;
>  
> -    packet->magic = be32_to_cpu(packet->magic);
> -    if (packet->magic != MULTIFD_MAGIC) {
> -        error_setg(errp, "multifd: received packet "
> -                   "magic %x and expected magic %x",
> -                   packet->magic, MULTIFD_MAGIC);
> +    if (magic != MULTIFD_MAGIC) {
> +        error_setg(errp, "multifd: received packet magic %x, expected %x",
> +                   magic, MULTIFD_MAGIC);
>          return -1;
>      }
>  
> -    packet->version = be32_to_cpu(packet->version);
> -    if (packet->version != MULTIFD_VERSION) {
> -        error_setg(errp, "multifd: received packet "
> -                   "version %u and expected version %u",
> -                   packet->version, MULTIFD_VERSION);
> +    if (version != MULTIFD_VERSION) {
> +        error_setg(errp, "multifd: received packet version %u, expected %u",
> +                   version, MULTIFD_VERSION);
>          return -1;
>      }
>  
> -- 
> 2.35.3
> 

-- 
Peter Xu