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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 26/8/24 22:12, Peter Xu wrote:
> 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.

OTOH we can make MultiFDPacket_t const to be sure we don't alter it:

-- >8 --
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index 0cbf1b88e1..a759470c9c 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -217,11 +217,11 @@ void multifd_ram_fill_packet(MultiFDSendParams *p)

  int multifd_ram_unfill_packet(MultiFDRecvParams *p, Error **errp)
  {
-    MultiFDPacket_t *packet = p->packet;
+    const 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;
+    g_autofree const char *ramblock_name = NULL;
      int i;

      if (pages_per_packet > page_count) {
diff --git a/migration/multifd.c b/migration/multifd.c
index 256ecdea56..2a8cd9174c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -230,7 +230,7 @@ void multifd_send_fill_packet(MultiFDSendParams *p)

  static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
  {
-    MultiFDPacket_t *packet = p->packet;
+    const MultiFDPacket_t *packet = p->packet;
      uint32_t magic = be32_to_cpu(packet->magic);
      uint32_t version = be32_to_cpu(packet->version);
      int ret = 0;
---

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