1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/char/exynos4210_uart: Fix crash on trying to load VM state

Peter Maydell posted 1 patch 2 years, 3 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220120151648.433736-1-peter.maydell@linaro.org
Maintainers: Igor Mitsyanko <i.mitsyanko@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
hw/char/exynos4210_uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] hw/char/exynos4210_uart: Fix crash on trying to load VM state
Posted by Peter Maydell 2 years, 3 months ago 
The exynos4210_uart_post_load() function assumes that it is passed
the Exynos4210UartState, but it has been attached to the
VMStateDescription for the Exynos4210UartFIFO type.  The result is a
SIGSEGV when attempting to load VM state for any machine type
including this device.

Fix the bug by attaching the post-load function to the VMSD for the
Exynos4210UartState.  This is the logical place for it, because the
actions it does relate to the entire UART state, not just the FIFO.

Thanks to the bug reporter @TrungNguyen1909 for the clear bug
description and the suggested fix.

Fixes: c9d3396d80fe7ece9b
   ("hw/char/exynos4210_uart: Implement post_load function")
Buglink: https://gitlab.com/qemu-project/qemu/-/issues/638
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/exynos4210_uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 80d401a3795..addcd59b028 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -628,7 +628,6 @@ static const VMStateDescription vmstate_exynos4210_uart_fifo = {
     .name = "exynos4210.uart.fifo",
     .version_id = 1,
     .minimum_version_id = 1,
-    .post_load = exynos4210_uart_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(sp, Exynos4210UartFIFO),
         VMSTATE_UINT32(rp, Exynos4210UartFIFO),
@@ -641,6 +640,7 @@ static const VMStateDescription vmstate_exynos4210_uart = {
     .name = "exynos4210.uart",
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = exynos4210_uart_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(rx, Exynos4210UartState, 1,
                        vmstate_exynos4210_uart_fifo, Exynos4210UartFIFO),
-- 
2.25.1
Re: [PATCH] hw/char/exynos4210_uart: Fix crash on trying to load VM state
Posted by Guenter Roeck 2 years, 3 months ago 
On 1/20/22 7:16 AM, Peter Maydell wrote:
> The exynos4210_uart_post_load() function assumes that it is passed
> the Exynos4210UartState, but it has been attached to the
> VMStateDescription for the Exynos4210UartFIFO type.  The result is a
> SIGSEGV when attempting to load VM state for any machine type
> including this device.
> 
> Fix the bug by attaching the post-load function to the VMSD for the
> Exynos4210UartState.  This is the logical place for it, because the
> actions it does relate to the entire UART state, not just the FIFO.
> 
> Thanks to the bug reporter @TrungNguyen1909 for the clear bug
> description and the suggested fix.
> 
> Fixes: c9d3396d80fe7ece9b
>     ("hw/char/exynos4210_uart: Implement post_load function")
> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/638
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   hw/char/exynos4210_uart.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index 80d401a3795..addcd59b028 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -628,7 +628,6 @@ static const VMStateDescription vmstate_exynos4210_uart_fifo = {
>       .name = "exynos4210.uart.fifo",
>       .version_id = 1,
>       .minimum_version_id = 1,
> -    .post_load = exynos4210_uart_post_load,
>       .fields = (VMStateField[]) {
>           VMSTATE_UINT32(sp, Exynos4210UartFIFO),
>           VMSTATE_UINT32(rp, Exynos4210UartFIFO),
> @@ -641,6 +640,7 @@ static const VMStateDescription vmstate_exynos4210_uart = {
>       .name = "exynos4210.uart",
>       .version_id = 1,
>       .minimum_version_id = 1,
> +    .post_load = exynos4210_uart_post_load,
>       .fields = (VMStateField[]) {
>           VMSTATE_STRUCT(rx, Exynos4210UartState, 1,
>                          vmstate_exynos4210_uart_fifo, Exynos4210UartFIFO),
>
Re: [PATCH] hw/char/exynos4210_uart: Fix crash on trying to load VM state
Posted by Philippe Mathieu-Daudé via 2 years, 3 months ago 
On 20/1/22 16:16, Peter Maydell wrote:
> The exynos4210_uart_post_load() function assumes that it is passed
> the Exynos4210UartState, but it has been attached to the
> VMStateDescription for the Exynos4210UartFIFO type.  The result is a
> SIGSEGV when attempting to load VM state for any machine type
> including this device.
> 
> Fix the bug by attaching the post-load function to the VMSD for the
> Exynos4210UartState.  This is the logical place for it, because the
> actions it does relate to the entire UART state, not just the FIFO.
> 
> Thanks to the bug reporter @TrungNguyen1909 for the clear bug
> description and the suggested fix.
> 
> Fixes: c9d3396d80fe7ece9b
>     ("hw/char/exynos4210_uart: Implement post_load function")
> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/638

Apparently GitLab doesn't recognize "Buglink":
https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern
which might be why we use "Resolves: " to have GitLab
automatically close issues.

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/char/exynos4210_uart.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
Re: [PATCH] hw/char/exynos4210_uart: Fix crash on trying to load VM state
Posted by Peter Maydell 2 years, 3 months ago 
On Sat, 22 Jan 2022 at 09:50, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 20/1/22 16:16, Peter Maydell wrote:
> > The exynos4210_uart_post_load() function assumes that it is passed
> > the Exynos4210UartState, but it has been attached to the
> > VMStateDescription for the Exynos4210UartFIFO type.  The result is a
> > SIGSEGV when attempting to load VM state for any machine type
> > including this device.
> >
> > Fix the bug by attaching the post-load function to the VMSD for the
> > Exynos4210UartState.  This is the logical place for it, because the
> > actions it does relate to the entire UART state, not just the FIFO.
> >
> > Thanks to the bug reporter @TrungNguyen1909 for the clear bug
> > description and the suggested fix.
> >
> > Fixes: c9d3396d80fe7ece9b
> >     ("hw/char/exynos4210_uart: Implement post_load function")
> > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/638
>
> Apparently GitLab doesn't recognize "Buglink":
> https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern
> which might be why we use "Resolves: " to have GitLab
> automatically close issues.


Thanks; I can never remember which tag is the right one. I think
I just fished this one out of a random commit in the git history,
but I should have checked the docs, where we do have this
documented.

-- PMM

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.