| Message ID | 1406042801-28212-1-git-send-email-lersek@redhat.com |
|---|---|
| State | Accepted |
| Commit | 3afca1d6d413592c2b78cf28f52fa24a586d8f56 |
| Headers | show |
On (Tue) 22 Jul 2014 [17:26:41], Laszlo Ersek wrote: > "vmstate_xhci_event" was introduced in commit 37352df3 ("xhci: add live > migration support"), and first released in v1.6.0. The field list in this > VMSD is not terminated with the VMSTATE_END_OF_LIST() macro. > > During normal use (ie. migration), the issue is practically invisible, > because the "vmstate_xhci_event" object (with the unterminated field list) > is only ever referenced -- via "vmstate_xhci_intr" -- if xhci_er_full() > returns true, for the "ev_buffer" test. Since that field_exists() check > (apparently) almost always returns false, we almost never traverse > "vmstate_xhci_event" during migration, which hides the bug. > > However, Amit's vmstate checker forces recursion into this VMSD as well, > and the lack of VMSTATE_END_OF_LIST() breaks the field list terminator > check (field->name != NULL) in dump_vmstate_vmsd(). The result is > undefined behavior, which in my case translates to infinite recursion > (because the loop happens to overflow into "vmstate_xhci_intr", which then > links back to "vmstate_xhci_event"). > > Add the missing terminator. > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> Thanks for spotting this! Reviewed-by: Amit Shah <amit.shah@redhat.com> Amit
Il 22/07/2014 17:26, Laszlo Ersek ha scritto: > "vmstate_xhci_event" was introduced in commit 37352df3 ("xhci: add live > migration support"), and first released in v1.6.0. The field list in this > VMSD is not terminated with the VMSTATE_END_OF_LIST() macro. > > During normal use (ie. migration), the issue is practically invisible, > because the "vmstate_xhci_event" object (with the unterminated field list) > is only ever referenced -- via "vmstate_xhci_intr" -- if xhci_er_full() > returns true, for the "ev_buffer" test. Since that field_exists() check > (apparently) almost always returns false, we almost never traverse > "vmstate_xhci_event" during migration, which hides the bug. > > However, Amit's vmstate checker forces recursion into this VMSD as well, > and the lack of VMSTATE_END_OF_LIST() breaks the field list terminator > check (field->name != NULL) in dump_vmstate_vmsd(). The result is > undefined behavior, which in my case translates to infinite recursion > (because the loop happens to overflow into "vmstate_xhci_intr", which then > links back to "vmstate_xhci_event"). > > Add the missing terminator. > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > hw/usb/hcd-xhci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index 7f2af89..58c4b11 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -3737,6 +3737,7 @@ static const VMStateDescription vmstate_xhci_event = { > VMSTATE_UINT32(flags, XHCIEvent), > VMSTATE_UINT8(slotid, XHCIEvent), > VMSTATE_UINT8(epid, XHCIEvent), > + VMSTATE_END_OF_LIST() > } > }; > > Cc: qemu-stable@nongnu.org Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
On 07/22/14 17:42, Paolo Bonzini wrote: > Il 22/07/2014 17:26, Laszlo Ersek ha scritto: >> "vmstate_xhci_event" was introduced in commit 37352df3 ("xhci: add live >> migration support"), and first released in v1.6.0. The field list in this >> VMSD is not terminated with the VMSTATE_END_OF_LIST() macro. >> >> During normal use (ie. migration), the issue is practically invisible, >> because the "vmstate_xhci_event" object (with the unterminated field list) >> is only ever referenced -- via "vmstate_xhci_intr" -- if xhci_er_full() >> returns true, for the "ev_buffer" test. Since that field_exists() check >> (apparently) almost always returns false, we almost never traverse >> "vmstate_xhci_event" during migration, which hides the bug. >> >> However, Amit's vmstate checker forces recursion into this VMSD as well, >> and the lack of VMSTATE_END_OF_LIST() breaks the field list terminator >> check (field->name != NULL) in dump_vmstate_vmsd(). The result is >> undefined behavior, which in my case translates to infinite recursion >> (because the loop happens to overflow into "vmstate_xhci_intr", which then >> links back to "vmstate_xhci_event"). >> >> Add the missing terminator. >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> hw/usb/hcd-xhci.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c >> index 7f2af89..58c4b11 100644 >> --- a/hw/usb/hcd-xhci.c >> +++ b/hw/usb/hcd-xhci.c >> @@ -3737,6 +3737,7 @@ static const VMStateDescription vmstate_xhci_event = { >> VMSTATE_UINT32(flags, XHCIEvent), >> VMSTATE_UINT8(slotid, XHCIEvent), >> VMSTATE_UINT8(epid, XHCIEvent), >> + VMSTATE_END_OF_LIST() >> } >> }; >> >> > > Cc: qemu-stable@nongnu.org As far as I can see, this address was present in my original To: list. :) (Admittedly, not with CC.) > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thank you! Laszlo
On 22 July 2014 16:48, Laszlo Ersek <lersek@redhat.com> wrote: > On 07/22/14 17:42, Paolo Bonzini wrote: >> Cc: qemu-stable@nongnu.org > > As far as I can see, this address was present in my original To: list. > :) (Admittedly, not with CC.) Including the line in the commit message means it can get pulled back out of the git log automatically for application to the stable branch. I'll commit this to master shortly. thanks -- PMM
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 7f2af89..58c4b11 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3737,6 +3737,7 @@ static const VMStateDescription vmstate_xhci_event = { VMSTATE_UINT32(flags, XHCIEvent), VMSTATE_UINT8(slotid, XHCIEvent), VMSTATE_UINT8(epid, XHCIEvent), + VMSTATE_END_OF_LIST() } };
"vmstate_xhci_event" was introduced in commit 37352df3 ("xhci: add live migration support"), and first released in v1.6.0. The field list in this VMSD is not terminated with the VMSTATE_END_OF_LIST() macro. During normal use (ie. migration), the issue is practically invisible, because the "vmstate_xhci_event" object (with the unterminated field list) is only ever referenced -- via "vmstate_xhci_intr" -- if xhci_er_full() returns true, for the "ev_buffer" test. Since that field_exists() check (apparently) almost always returns false, we almost never traverse "vmstate_xhci_event" during migration, which hides the bug. However, Amit's vmstate checker forces recursion into this VMSD as well, and the lack of VMSTATE_END_OF_LIST() breaks the field list terminator check (field->name != NULL) in dump_vmstate_vmsd(). The result is undefined behavior, which in my case translates to infinite recursion (because the loop happens to overflow into "vmstate_xhci_intr", which then links back to "vmstate_xhci_event"). Add the missing terminator. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/usb/hcd-xhci.c | 1 + 1 file changed, 1 insertion(+)