[v3] vmstate: fix subsection load name check

[PATCH v3] vmstate: fix subsection load name check

Posted by Alexandr Moshkov 3 weeks, 5 days ago

When loading a subset, its name is checked for the parent prefix. The
following bug may occur here:

Let's say there is a vmstate named "virtio-blk", it has a subsection
named "virtio-blk/subsection", and it also has another vmstate named
"virtio" in the fields.
Then, during the migration, when trying to load this subsection for
"virtio", the prefix condition will pass for "virtio-blk/subsection" and
then the migration will break, because this vmstate does not have such a
subsection.

In other words, if a field inside vmstate1 is set via vmstate2 with a
name that is a prefix of the parent vmstate, then the field can "steal"
a subsection belonging to the parent state.

Looks like it happens because migration stream for "virtio-blk" looks
like this:

 [virtio-blk header] [virtio-blk fields] [virtio-blk subsections]

"virtio-blk" contains "virtio" field, so migration stream is:

 [virtio-blk header] [virtio header] [virtio fields] [virtio
subsections] [virtio-blk subsections]

And when we load the subsections of the "virtio" device,
vmstate_subsection_load() uses qemu_peek_byte() to try to figure out if
this is his subsection. This is where we encounter an error.

Thus, the error occurs due to the fact that vmsd does not know how many
subsections it has when loading (this does not appear anywhere in the
migration stream), so it tries to load all the appropriate ones by
names.

To fix this issue, we call vmstate_get_subsection() and, in case of
fail, ignore it, pop the VMSD stack and will try to define this
subsection for outer VMSD

Signed-off-by: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
---
 migration/vmstate.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 4d28364f7b..8f3a69c26e 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -639,9 +639,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
         if (sub_vmsd == NULL) {
             trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
-            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
-                       idstr, vmsd->name);
-            return -ENOENT;
+            return 0;
         }
         qemu_file_skip(f, 1); /* subsection */
         qemu_file_skip(f, 1); /* len */
-- 
2.34.1

Re: [PATCH v3] vmstate: fix subsection load name check

Posted by Fabiano Rosas 3 weeks, 5 days ago

Alexandr Moshkov <dtalexundeer@yandex-team.ru> writes:

Sorry, but I'll have to nitpick on the commit message, I can't parse
this without looking at the code.

> When loading a subset, its name is checked for the parent prefix. The

What do you mean by subset?

> following bug may occur here:
>
> Let's say there is a vmstate named "virtio-blk", it has a subsection
> named "virtio-blk/subsection", and it also has another vmstate named
> "virtio" in the fields.
> Then, during the migration, when trying to load this subsection for

What is "this subsection" in this context? 'virtio-blk/subsection' ? Or do
you mean the 'virtio' field.

> "virtio", the prefix condition will pass for "virtio-blk/subsection" and

What's "the prefix condition" exactly? Could you be more explicit?

> then the migration will break, because this vmstate does not have such a
> subsection.
>
> In other words, if a field inside vmstate1 is set via vmstate2 with a
> name that is a prefix of the parent vmstate, then the field can "steal"
> a subsection belonging to the parent state.
>

This is what I read from the above paragraph. Is this it?

vmstate1 "abc"         vmstate2 "abcdef"
  field -> vmstate2
  subsection

> Looks like it happens because migration stream for "virtio-blk" looks
> like this:
>
>  [virtio-blk header] [virtio-blk fields] [virtio-blk subsections]
>
> "virtio-blk" contains "virtio" field, so migration stream is:
>
>  [virtio-blk header] [virtio header] [virtio fields] [virtio
> subsections] [virtio-blk subsections]
>

So the code sees the [virtio-blk subsections] as a continuation of the
preceding [virtio subsections] because the string "virtio", which is the
parent of [virtio subsection] is matched in the string "virtio-blk"?

> And when we load the subsections of the "virtio" device,
> vmstate_subsection_load() uses qemu_peek_byte() to try to figure out if
> this is his subsection. This is where we encounter an error.
>
> Thus, the error occurs due to the fact that vmsd does not know how many
> subsections it has when loading (this does not appear anywhere in the
> migration stream), so it tries to load all the appropriate ones by
> names.
>
> To fix this issue, we call vmstate_get_subsection() and, in case of
> fail, ignore it, pop the VMSD stack and will try to define this
> subsection for outer VMSD
>
> Signed-off-by: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
> ---
>  migration/vmstate.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 4d28364f7b..8f3a69c26e 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -639,9 +639,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>          if (sub_vmsd == NULL) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
> -            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> -                       idstr, vmsd->name);
> -            return -ENOENT;
> +            return 0;
>          }
>          qemu_file_skip(f, 1); /* subsection */
>          qemu_file_skip(f, 1); /* len */

Re: [PATCH v3] vmstate: fix subsection load name check

Posted by Alexandr Moshkov 3 weeks, 5 days ago

On 3/12/26 19:15, Fabiano Rosas wrote:
> Alexandr Moshkov<dtalexundeer@yandex-team.ru> writes:
>
> Sorry, but I'll have to nitpick on the commit message, I can't parse
> this without looking at the code.
>
>> When loading a subset, its name is checked for the parent prefix. The
> What do you mean by subset?
Sorry, i wanted to write "subsection" here..
>
>> following bug may occur here:
>>
>> Let's say there is a vmstate named "virtio-blk", it has a subsection
>> named "virtio-blk/subsection", and it also has another vmstate named
>> "virtio" in the fields.
>> Then, during the migration, when trying to load this subsection for
> What is "this subsection" in this context? 'virtio-blk/subsection' ? Or do
> you mean the 'virtio' field.
Yes, "virtio-blk/subsection".
>
>> "virtio", the prefix condition will pass for "virtio-blk/subsection" and
> What's "the prefix condition" exactly? Could you be more explicit?
Ifthe vmstate nameisnotaprefixtothe subsection,thenwethinksucha 
subsectionshouldbe skipped to load for this vmstate,becauseitdoes 
notrelatetoit.
>
>> then the migration will break, because this vmstate does not have such a
>> subsection.
>>
>> In other words, if a field inside vmstate1 is set via vmstate2 with a
>> name that is a prefix of the parent vmstate, then the field can "steal"
>> a subsection belonging to the parent state.
>>
> This is what I read from the above paragraph. Is this it?
>
> vmstate1 "abc"         vmstate2 "abcdef"
>    field -> vmstate2
>    subsection
Yes, that's right.
>> Looks like it happens because migration stream for "virtio-blk" looks
>> like this:
>>
>>   [virtio-blk header] [virtio-blk fields] [virtio-blk subsections]
>>
>> "virtio-blk" contains "virtio" field, so migration stream is:
>>
>>   [virtio-blk header] [virtio header] [virtio fields] [virtio
>> subsections] [virtio-blk subsections]
>>
> So the code sees the [virtio-blk subsections] as a continuation of the
> preceding [virtio subsections] because the string "virtio", which is the
> parent of [virtio subsection] is matched in the string "virtio-blk"?
Yes, that's right too.
>
>> And when we load the subsections of the "virtio" device,
>> vmstate_subsection_load() uses qemu_peek_byte() to try to figure out if
>> this is his subsection. This is where we encounter an error.
>>
>> Thus, the error occurs due to the fact that vmsd does not know how many
>> subsections it has when loading (this does not appear anywhere in the
>> migration stream), so it tries to load all the appropriate ones by
>> names.
>>
>> To fix this issue, we call vmstate_get_subsection() and, in case of
>> fail, ignore it, pop the VMSD stack and will try to define this
>> subsection for outer VMSD
>>
>> Signed-off-by: Alexandr Moshkov<dtalexundeer@yandex-team.ru>
>> ---
>>   migration/vmstate.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 4d28364f7b..8f3a69c26e 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -639,9 +639,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>>           sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>>           if (sub_vmsd == NULL) {
>>               trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
>> -            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
>> -                       idstr, vmsd->name);
>> -            return -ENOENT;
>> +            return 0;
>>           }
>>           qemu_file_skip(f, 1); /* subsection */
>>           qemu_file_skip(f, 1); /* len */

Re: [PATCH v3] vmstate: fix subsection load name check

Posted by Vladimir Sementsov-Ogievskiy 3 weeks, 5 days ago

On 12.03.26 13:26, Alexandr Moshkov wrote:
> When loading a subset, its name is checked for the parent prefix. The
> following bug may occur here:
> 
> Let's say there is a vmstate named "virtio-blk", it has a subsection
> named "virtio-blk/subsection", and it also has another vmstate named
> "virtio" in the fields.
> Then, during the migration, when trying to load this subsection for
> "virtio", the prefix condition will pass for "virtio-blk/subsection" and
> then the migration will break, because this vmstate does not have such a
> subsection.
> 
> In other words, if a field inside vmstate1 is set via vmstate2 with a
> name that is a prefix of the parent vmstate, then the field can "steal"
> a subsection belonging to the parent state.
> 
> Looks like it happens because migration stream for "virtio-blk" looks
> like this:
> 
>   [virtio-blk header] [virtio-blk fields] [virtio-blk subsections]
> 
> "virtio-blk" contains "virtio" field, so migration stream is:
> 
>   [virtio-blk header] [virtio header] [virtio fields] [virtio
> subsections] [virtio-blk subsections]
> 
> And when we load the subsections of the "virtio" device,
> vmstate_subsection_load() uses qemu_peek_byte() to try to figure out if
> this is his subsection. This is where we encounter an error.
> 
> Thus, the error occurs due to the fact that vmsd does not know how many
> subsections it has when loading (this does not appear anywhere in the
> migration stream), so it tries to load all the appropriate ones by
> names.
> 
> To fix this issue, we call vmstate_get_subsection() and, in case of
> fail, ignore it, pop the VMSD stack and will try to define this
> subsection for outer VMSD
> 
> Signed-off-by: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
> ---
>   migration/vmstate.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 4d28364f7b..8f3a69c26e 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -639,9 +639,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,

more contex:

         if (strncmp(vmsd->name, idstr, strlen(vmsd->name)) != 0) {
             trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(prefix)");
             /* it doesn't have a valid subsection name */
             return 0;
         }

If go this way, I think this check for prefix ^^^ should be removed.


>           sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>           if (sub_vmsd == NULL) {
>               trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
> -            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> -                       idstr, vmsd->name);
> -            return -ENOENT;
> +            return 0;
>           }
>           qemu_file_skip(f, 1); /* subsection */
>           qemu_file_skip(f, 1); /* len */


-- 
Best regards,
Vladimir

Re: [PATCH v3] vmstate: fix subsection load name check

Posted by Fabiano Rosas 3 weeks, 5 days ago

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 12.03.26 13:26, Alexandr Moshkov wrote:
>> When loading a subset, its name is checked for the parent prefix. The
>> following bug may occur here:
>> 
>> Let's say there is a vmstate named "virtio-blk", it has a subsection
>> named "virtio-blk/subsection", and it also has another vmstate named
>> "virtio" in the fields.
>> Then, during the migration, when trying to load this subsection for
>> "virtio", the prefix condition will pass for "virtio-blk/subsection" and
>> then the migration will break, because this vmstate does not have such a
>> subsection.
>> 
>> In other words, if a field inside vmstate1 is set via vmstate2 with a
>> name that is a prefix of the parent vmstate, then the field can "steal"
>> a subsection belonging to the parent state.
>> 
>> Looks like it happens because migration stream for "virtio-blk" looks
>> like this:
>> 
>>   [virtio-blk header] [virtio-blk fields] [virtio-blk subsections]
>> 
>> "virtio-blk" contains "virtio" field, so migration stream is:
>> 
>>   [virtio-blk header] [virtio header] [virtio fields] [virtio
>> subsections] [virtio-blk subsections]
>> 
>> And when we load the subsections of the "virtio" device,
>> vmstate_subsection_load() uses qemu_peek_byte() to try to figure out if
>> this is his subsection. This is where we encounter an error.
>> 
>> Thus, the error occurs due to the fact that vmsd does not know how many
>> subsections it has when loading (this does not appear anywhere in the
>> migration stream), so it tries to load all the appropriate ones by
>> names.
>> 
>> To fix this issue, we call vmstate_get_subsection() and, in case of
>> fail, ignore it, pop the VMSD stack and will try to define this
>> subsection for outer VMSD
>> 
>> Signed-off-by: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
>> ---
>>   migration/vmstate.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 4d28364f7b..8f3a69c26e 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -639,9 +639,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>
> more contex:
>
>          if (strncmp(vmsd->name, idstr, strlen(vmsd->name)) != 0) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(prefix)");
>              /* it doesn't have a valid subsection name */
>              return 0;
>          }
>
> If go this way, I think this check for prefix ^^^ should be removed.
>

+1

Re: [PATCH v3] vmstate: fix subsection load name check

Posted by Peter Maydell 3 weeks, 5 days ago

On Thu, 12 Mar 2026 at 10:26, Alexandr Moshkov
<dtalexundeer@yandex-team.ru> wrote:
>
> When loading a subset, its name is checked for the parent prefix. The
> following bug may occur here:
>
> Let's say there is a vmstate named "virtio-blk", it has a subsection
> named "virtio-blk/subsection", and it also has another vmstate named
> "virtio" in the fields.
> Then, during the migration, when trying to load this subsection for
> "virtio", the prefix condition will pass for "virtio-blk/subsection" and
> then the migration will break, because this vmstate does not have such a
> subsection.
>
> In other words, if a field inside vmstate1 is set via vmstate2 with a
> name that is a prefix of the parent vmstate, then the field can "steal"
> a subsection belonging to the parent state.
>
> Looks like it happens because migration stream for "virtio-blk" looks
> like this:
>
>  [virtio-blk header] [virtio-blk fields] [virtio-blk subsections]
>
> "virtio-blk" contains "virtio" field, so migration stream is:
>
>  [virtio-blk header] [virtio header] [virtio fields] [virtio
> subsections] [virtio-blk subsections]
>
> And when we load the subsections of the "virtio" device,
> vmstate_subsection_load() uses qemu_peek_byte() to try to figure out if
> this is his subsection. This is where we encounter an error.
>
> Thus, the error occurs due to the fact that vmsd does not know how many
> subsections it has when loading (this does not appear anywhere in the
> migration stream), so it tries to load all the appropriate ones by
> names.
>
> To fix this issue, we call vmstate_get_subsection() and, in case of
> fail, ignore it, pop the VMSD stack and will try to define this
> subsection for outer VMSD
>
> Signed-off-by: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
> ---
>  migration/vmstate.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 4d28364f7b..8f3a69c26e 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -639,9 +639,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>          if (sub_vmsd == NULL) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
> -            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> -                       idstr, vmsd->name);
> -            return -ENOENT;
> +            return 0;
>          }

Does this result in a less useful error message for the case
where the source sent us a non-existent subsection ? What do
we report once this patch is applied ?

-- PMM

Re: [PATCH v3] vmstate: fix subsection load name check

Posted by Alexandr Moshkov 3 weeks, 5 days ago

On 3/12/26 16:19, Peter Maydell wrote:
> On Thu, 12 Mar 2026 at 10:26, Alexandr Moshkov
> <dtalexundeer@yandex-team.ru> wrote:
>> When loading a subset, its name is checked for the parent prefix. The
>> following bug may occur here:
>>
>> Let's say there is a vmstate named "virtio-blk", it has a subsection
>> named "virtio-blk/subsection", and it also has another vmstate named
>> "virtio" in the fields.
>> Then, during the migration, when trying to load this subsection for
>> "virtio", the prefix condition will pass for "virtio-blk/subsection" and
>> then the migration will break, because this vmstate does not have such a
>> subsection.
>>
>> In other words, if a field inside vmstate1 is set via vmstate2 with a
>> name that is a prefix of the parent vmstate, then the field can "steal"
>> a subsection belonging to the parent state.
>>
>> Looks like it happens because migration stream for "virtio-blk" looks
>> like this:
>>
>>   [virtio-blk header] [virtio-blk fields] [virtio-blk subsections]
>>
>> "virtio-blk" contains "virtio" field, so migration stream is:
>>
>>   [virtio-blk header] [virtio header] [virtio fields] [virtio
>> subsections] [virtio-blk subsections]
>>
>> And when we load the subsections of the "virtio" device,
>> vmstate_subsection_load() uses qemu_peek_byte() to try to figure out if
>> this is his subsection. This is where we encounter an error.
>>
>> Thus, the error occurs due to the fact that vmsd does not know how many
>> subsections it has when loading (this does not appear anywhere in the
>> migration stream), so it tries to load all the appropriate ones by
>> names.
>>
>> To fix this issue, we call vmstate_get_subsection() and, in case of
>> fail, ignore it, pop the VMSD stack and will try to define this
>> subsection for outer VMSD
>>
>> Signed-off-by: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
>> ---
>>   migration/vmstate.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 4d28364f7b..8f3a69c26e 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -639,9 +639,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>>           sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>>           if (sub_vmsd == NULL) {
>>               trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
>> -            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
>> -                       idstr, vmsd->name);
>> -            return -ENOENT;
>> +            return 0;
>>           }
> Does this result in a less useful error message for the case
> where the source sent us a non-existent subsection ? What do
> we report once this patch is applied ?
I think it might lead to, but in general, it will work correctly.