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

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> 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.

Ok, I can drop this part.

>
>> +    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
>>