Re: [Qemu-devel] Re: [PATCH] savevm: fix corruption in vmstate_subsection_load().

[Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>]

2011/1/20 Paolo Bonzini <pbonzini@redhat.com>:
> On 01/20/2011 09:57 AM, Yoshiaki Tamura wrote:
>>
>> 2011/1/20 Paolo Bonzini<pbonzini@redhat.com>:
>>>
>>> On 12/14/2010 10:07 AM, Yoshiaki Tamura wrote:
>>>>
>>>> Although it's rare to happen in live migration, when the head of a
>>>> byte stream contains 0x05
>>>
>>> IIUC, this happens if you have VMS_STRUCT and the field after the
>>> VMS_STRUCT
>>> starts with 0x5.
>>>
>>> I think you should also add this in vmstate_subsection_load:
>>>
>>>    sub_vmsd = vmstate_get_subsection(sub, idstr);
>>>    if (sub_vmsd == NULL) {
>>>        return -ENOENT;
>>>    }
>>> +   assert (!sub_vmsd->subsections);
>>>    ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
>>>
>>> and this in vmstate_load_state:
>>>
>>>    if (field->flags&  VMS_STRUCT) {
>>> +       assert (!vmsd->subsections);
>>>        ret = vmstate_load_state(f, field->vmsd, addr,
>>>                                 field->vmsd->version_id);
>>>    }
>>
>> Hi Paolo,
>>
>> You mean, having subsection nested and under VMS_STRUCT are
>> violations?
>
> I believe so, because the protocol doesn't allow you to distinguish:
>
> - in the case of nested subsections, whether 2 consecutive subsections are
> siblings, or the second is nested into the first.  In fact, your patch also
> fixes a latent bug in case a device supports more than one subsection, and
> both are present in the data stream.  When vmstate_subsection_load is called
> for the first subsection, it would see a 0x5 byte (the beginning of the
> second subsection), eat it and then fail with ENOENT.  The second subsection
> would then fail to load.
>
> - in the case of VMS_STRUCT, whether a 0x5 byte after the VMS_STRUCT is a
> subsection or part of the parent data stream.  This is, I believe, the
> source of your bug.

Thank you for the explanation.  It's very helpful because I
didn't know the background of subsection.  Kemari is kind of
stress test of live migration.

> I don't think it is possible to fix these problems in the file format while
> preserving backwards compatibility with pre-subsection QEMU (which was a
> fundamental requirement of subsections).  So, I think your patch is correct
> and fixes the practical bugs.  However, we can be even stronger and assert
> that the problematic vmstate descriptions are not used.
>
> Even better, asserts matching the ones above could be added to
> vmstate_subsection_save and vmstate_save_state, as well.

I see.  Let me fold the assertion you pointed to the original
patch for now.  Because I'm not an expert in subsection, I would
like to leave further improvements to the others.

Yoshi

>
> Paolo
>
>