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

[Yoshiaki Tamura]

2011/1/20 Yoshiaki Tamura <address@hidden>:
> 2011/1/20 Paolo Bonzini <address@hidden>:
>> On 01/20/2011 09:57 AM, Yoshiaki Tamura wrote:
>>>
>>> 2011/1/20 Paolo Bonzini<address@hidden>:
>>>>
>>>> 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.

   if (field->flags & VMS_STRUCT) {
+       assert (!vmsd->subsections);
       ret = vmstate_load_state(f, field->vmsd, addr,
                                field->vmsd->version_id);
   }

This assertion always fail for "cpu" in my environment.

Yoshi

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