Re: [Qemu-devel] [PATCH RESEND] migration: catch unknown flags in ram_load

Re: [Qemu-devel] [PATCH RESEND] migration: catch unknown flags in ram_load

[Dr. David Alan Gilbert]

* Peter Lieven (pl@kamp.de) wrote:
> if a saved vm has unknown flags in the memory data qemu
> currently simply ignores this flag and continues which
> yields in an unpredictable result.
> 
> this patch catches all unknown flags and
> aborts the loading of the vm.

Yes, I think that's quite nice - the only thing I'd add which would help
a little is an error_report() in the case where you have invalid flags
to let you know that's the reason for the EINVAL.

(Having spent the day tracking down migration bugs...)

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  arch_init.c |   35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 995f56d..322d129 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1030,17 +1030,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      ram_addr_t addr;
>      int flags, ret = 0;
> -    int error;
>      static uint64_t seq_iter;
>  
>      seq_iter++;
>  
>      if (version_id != 4) {
>          ret = -EINVAL;
> -        goto done;
>      }
>  
> -    do {
> +    while (!ret) {
>          addr = qemu_get_be64(f);
>  
>          flags = addr & ~TARGET_PAGE_MASK;
> @@ -1069,7 +1067,6 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>                                      " in != " RAM_ADDR_FMT "\n", id, length,
>                                      block->length);
>                              ret =  -EINVAL;
> -                            goto done;
>                          }
>                          break;
>                      }
> @@ -1079,21 +1076,21 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>                      fprintf(stderr, "Unknown ramblock \"%s\", cannot "
>                              "accept migration\n", id);
>                      ret = -EINVAL;
> -                    goto done;
> +                }
> +                if (ret) {
> +                    break;
>                  }
>  
>                  total_ram_bytes -= length;
>              }
> -        }
> -
> -        if (flags & RAM_SAVE_FLAG_COMPRESS) {
> +        } else if (flags & RAM_SAVE_FLAG_COMPRESS) {
>              void *host;
>              uint8_t ch;
>  
>              host = host_from_stream_offset(f, addr, flags);
>              if (!host) {
>                  ret = -EINVAL;
> -                goto done;
> +                break;
>              }
>  
>              ch = qemu_get_byte(f);
> @@ -1104,7 +1101,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>              host = host_from_stream_offset(f, addr, flags);
>              if (!host) {
>                  ret = -EINVAL;
> -                goto done;
> +                break;
>              }
>  
>              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> @@ -1112,24 +1109,24 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>              void *host = host_from_stream_offset(f, addr, flags);
>              if (!host) {
>                  ret = -EINVAL;
> -                goto done;
> +                break;
>              }
>  
>              if (load_xbzrle(f, addr, host) < 0) {
>                  ret = -EINVAL;
> -                goto done;
> +                break;
>              }
>          } else if (flags & RAM_SAVE_FLAG_HOOK) {
>              ram_control_load_hook(f, flags);
> +        } else if (flags & RAM_SAVE_FLAG_EOS) {
> +            break;
> +        } else {
> +            ret = -EINVAL;
> +            break;
>          }
> -        error = qemu_file_get_error(f);
> -        if (error) {
> -            ret = error;
> -            goto done;
> -        }
> -    } while (!(flags & RAM_SAVE_FLAG_EOS));
> +        ret = qemu_file_get_error(f);
> +    }
>  
> -done:
>      DPRINTF("Completed load of VM with exit code %d seq iteration "
>              "%" PRIu64 "\n", ret, seq_iter);
>      return ret;
> -- 
> 1.7.9.5
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RESEND] migration: catch unknown flags in ram_load

[Amit Shah]

On (Tue) 03 Jun 2014 [18:27:15], Dr. David Alan Gilbert wrote:
> * Peter Lieven (pl@kamp.de) wrote:
> > if a saved vm has unknown flags in the memory data qemu
> > currently simply ignores this flag and continues which
> > yields in an unpredictable result.
> > 
> > this patch catches all unknown flags and
> > aborts the loading of the vm.
> 
> Yes, I think that's quite nice - the only thing I'd add which would help
> a little is an error_report() in the case where you have invalid flags
> to let you know that's the reason for the EINVAL.

Agreed.

		Amit

Re: [Qemu-devel] [PATCH RESEND] migration: catch unknown flags in ram_load

[Amit Shah]

On (Tue) 03 Jun 2014 [18:27:15], Dr. David Alan Gilbert wrote:
> * Peter Lieven (pl@kamp.de) wrote:
> > if a saved vm has unknown flags in the memory data qemu
> > currently simply ignores this flag and continues which
> > yields in an unpredictable result.
> > 
> > this patch catches all unknown flags and
> > aborts the loading of the vm.
> 
> Yes, I think that's quite nice - the only thing I'd add which would help
> a little is an error_report() in the case where you have invalid flags
> to let you know that's the reason for the EINVAL.
> 
> (Having spent the day tracking down migration bugs...)

Peter, can you please add these before we pick the patch up?

Thanks,

		Amit

Re: [Qemu-devel] [PATCH RESEND] migration: catch unknown flags in ram_load

[Peter Lieven]

On 10.06.2014 09:31, Amit Shah wrote:
> On (Tue) 03 Jun 2014 [18:27:15], Dr. David Alan Gilbert wrote:
>> * Peter Lieven (pl@kamp.de) wrote:
>>> if a saved vm has unknown flags in the memory data qemu
>>> currently simply ignores this flag and continues which
>>> yields in an unpredictable result.
>>>
>>> this patch catches all unknown flags and
>>> aborts the loading of the vm.
>> Yes, I think that's quite nice - the only thing I'd add which would help
>> a little is an error_report() in the case where you have invalid flags
>> to let you know that's the reason for the EINVAL.
>>
>> (Having spent the day tracking down migration bugs...)
> Peter, can you please add these before we pick the patch up?

On its way.

Peter