Re: [Qemu-devel] [PATCH 2/4] multifd: Drop x-multifd-page-count parameter

[Juan Quintela]

Daniel P. Berrangé <address@hidden> wrote:
> On Wed, Feb 06, 2019 at 02:23:29PM +0100, Juan Quintela wrote:
>> Libvirt don't want to expose (and explain it).  And testing looks like
>> 128 is good for all use cases, so just drop it.
>
> One significant concern inline...
>
>> 
>> Signed-off-by: Juan Quintela <address@hidden>
>> ---
>>  hmp.c                 |  7 -------
>>  migration/migration.c | 30 ------------------------------
>>  migration/migration.h |  1 -
>>  migration/ram.c       | 13 ++++++++-----
>>  qapi/migration.json   | 13 +------------
>>  5 files changed, 9 insertions(+), 55 deletions(-)
>
>
>> @@ -718,7 +721,7 @@ static void multifd_send_fill_packet(MultiFDSendParams 
>> *p)
>>      packet->magic = cpu_to_be32(MULTIFD_MAGIC);
>>      packet->version = cpu_to_be32(MULTIFD_VERSION);
>>      packet->flags = cpu_to_be32(p->flags);
>> -    packet->size = cpu_to_be32(migrate_multifd_page_count());
>> +    packet->size = cpu_to_be32(MULTIFD_PAGE_COUNT);
>>      packet->used = cpu_to_be32(p->pages->used);
>>      packet->packet_num = cpu_to_be64(p->packet_num);
>>
>
> Here the source QEMU sends the page size - which is now
> a hardcoded constant - to the target QEMU.
>
>> @@ -756,10 +759,10 @@ static int 
>> multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>>      p->flags = be32_to_cpu(packet->flags);
>>  
>>      packet->size = be32_to_cpu(packet->size);
>> -    if (packet->size > migrate_multifd_page_count()) {
>> +    if (packet->size > MULTIFD_PAGE_COUNT) {
>>          error_setg(errp, "multifd: received packet "
>>                     "with size %d and expected maximum size %d",
>> -                   packet->size, migrate_multifd_page_count()) ;
>> +                   packet->size, MULTIFD_PAGE_COUNT) ;
>>          return -1;
>>      }
>>
>
> Here the dest QEMU receives the page size that the source QEMU used, and
> checks that it is not larger than its constant.
>
> IIUC, the implication here is that if we ever increase the size of this
> constant in future QEMU, we will break live migration from new to old
> QEMU due to this check.  In fact your previous patch in this series has
> done exactly that, so this appears to mean QEMU 4.0 -> QEMU 3.2
> multifd migration is broken now.
>
> Alternatively if we decrease the size of the constant in future
> QEMU, we will break live migration from old QEMU to new QEMU which
> is even worse.
>
> This problem existed before this patch, if the management app was
> not explicitly using migrate-set-parameters to set the page count
> on both sides of QEMU. So we're already broken, but at least the
> feature was marked experimental.
>
> What is the purpose of this packet size check ?  Is it something
> we can safely remove, so that we can increase or decrease the
> size at will without breaking migration compat.

We have a "dinamyc" array of pages of that size.  What we check is that
the array fits into the part that we have assigned.

We "could" wait until this moment to create the arrays, I need to look
into that.  Notice that what the check does is making sure that whatewer
we receive is not bigger than the space that we have allocated.

At this point, that check can only fail if we are "being" attacked and
we have a malformed string.  We check during negotiation that this value
is ok.

We should check this *also* in the initial packet, and then this check
should never be true.

>From a management point of view, what do you preffer here?

Later, Juan.