[PATCH] qio: fix qemu crash when live migration

[PATCH] qio: fix qemu crash when live migration

[yaozhenguo]

qemu will crash in live migration cleanup process at source host.
BT is as below:

0  0x00007f740fc9e165 in g_source_destroy () at /usr/lib64/libglib-2.0.so.0
1  0x000055a2982a0f6e in qio_net_listener_set_client_func_full
2  0x000055a298345130 in tcp_chr_update_read_handler
3  0x000055a298341598 in qemu_chr_fe_set_handlers_full
4  0x000055a298341655 in qemu_chr_fe_set_handlers
5  0x000055a298191e75 in vhost_user_blk_event
6  0x000055a298292b79 in object_deinit
7  object_finalize
8  object_unref
9  0x000055a298292b3c in object_property_del_all
10 object_finalize
11 object_unref
12 0x000055a298291d7d in object_property_del_child
13 object_unparent
14 0x000055a29834a3c4 in qemu_chr_cleanup
15 0x000055a298160d87 in qemu_cleanup
16 0x000055a297e6bff1 in main

Crash reason is that qio_net_listener_finalize is called before
qio_net_listener_set_client_func_full. so, listener->io_source
is used after free. fix this by adding more checks.

Signed-off-by: yaozhenguo <yaozhenguo@jd.com>
---
 io/net-listener.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/io/net-listener.c b/io/net-listener.c
index 47405965a6..c02965f919 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -143,6 +143,11 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
 {
     size_t i;
 
+
+    if (!listener->nsioc || !listener->io_source || !listener->name) {
+        return;
+    }
+
     if (listener->io_notify) {
         listener->io_notify(listener->io_data);
     }
@@ -264,6 +269,10 @@ void qio_net_listener_disconnect(QIONetListener *listener)
 {
     size_t i;
 
+    if (!listener->nsioc || !listener->io_source || !listener->name) {
+        return;
+    }
+
     if (!listener->connected) {
         return;
     }
@@ -301,6 +310,10 @@ static void qio_net_listener_finalize(Object *obj)
     g_free(listener->io_source);
     g_free(listener->sioc);
     g_free(listener->name);
+
+    listener->io_source = NULL;
+    listener->sioc = NULL;
+    listener->name = NULL;
 }
 
 static const TypeInfo qio_net_listener_info = {
-- 
2.43.0

Re: [PATCH] qio: fix qemu crash when live migration

[Daniel P. Berrangé]

On Thu, Aug 08, 2024 at 11:04:11AM +0800, yaozhenguo wrote:
> qemu will crash in live migration cleanup process at source host.
> BT is as below:
> 
> 0  0x00007f740fc9e165 in g_source_destroy () at /usr/lib64/libglib-2.0.so.0
> 1  0x000055a2982a0f6e in qio_net_listener_set_client_func_full
> 2  0x000055a298345130 in tcp_chr_update_read_handler
> 3  0x000055a298341598 in qemu_chr_fe_set_handlers_full
> 4  0x000055a298341655 in qemu_chr_fe_set_handlers
> 5  0x000055a298191e75 in vhost_user_blk_event
> 6  0x000055a298292b79 in object_deinit
> 7  object_finalize
> 8  object_unref
> 9  0x000055a298292b3c in object_property_del_all
> 10 object_finalize
> 11 object_unref
> 12 0x000055a298291d7d in object_property_del_child
> 13 object_unparent
> 14 0x000055a29834a3c4 in qemu_chr_cleanup
> 15 0x000055a298160d87 in qemu_cleanup
> 16 0x000055a297e6bff1 in main
> 
> Crash reason is that qio_net_listener_finalize is called before
> qio_net_listener_set_client_func_full. so, listener->io_source
> is used after free. fix this by adding more checks.

If finalize() has been called, then not only has listener->io_source
been freed, but 'listener' itself has also been freed, thus....

> 
> Signed-off-by: yaozhenguo <yaozhenguo@jd.com>
> ---
>  io/net-listener.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/io/net-listener.c b/io/net-listener.c
> index 47405965a6..c02965f919 100644
> --- a/io/net-listener.c
> +++ b/io/net-listener.c
> @@ -143,6 +143,11 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
>  {
>      size_t i;
>  
> +
> +    if (!listener->nsioc || !listener->io_source || !listener->name) {
> +        return;
> +    }

....this is still accessing freed memory for 'listener'.


What is the call path of the stack triggering qio_net_listener_finalize ?

Whatever callpath has done that needs to be setting SocketChardev->listener
field to NULL, because tcp_chr_update_read_handler will check for NULL
before calling qio_net_listener_set_client_func_full.

> +
>      if (listener->io_notify) {
>          listener->io_notify(listener->io_data);
>      }
> @@ -264,6 +269,10 @@ void qio_net_listener_disconnect(QIONetListener *listener)
>  {
>      size_t i;
>  
> +    if (!listener->nsioc || !listener->io_source || !listener->name) {
> +        return;
> +    }
> +
>      if (!listener->connected) {
>          return;
>      }
> @@ -301,6 +310,10 @@ static void qio_net_listener_finalize(Object *obj)
>      g_free(listener->io_source);
>      g_free(listener->sioc);
>      g_free(listener->name);
> +
> +    listener->io_source = NULL;
> +    listener->sioc = NULL;
> +    listener->name = NULL;
>  }
>  
>  static const TypeInfo qio_net_listener_info = {
> -- 
> 2.43.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qio: fix qemu crash when live migration

[Zhenguo Yao]

Hi Daniel. Sorry,  I don't notice that this is a fixed issue by
b8a7f51f59e28d5a8e0c07ed3919cc9695560ed2(chardev/char-socket: set
s->listener = NULL in char_socket_finalize).
the following process can lead this issue：

char_socket_finalize->object_unref(OBJECT(s->listener)); // free
io_source, free s->listener. but s->listener is not clear.
char_socket_finalize->qemu_chr_be_event(chr, CHR_EVENT_CLOSED)->
qio_net_listener_set_client_func_full //
g_source_destroy(listener->io_source[i]); memory fault

Daniel P. Berrangé <berrange@redhat.com> 于2024年8月8日周四 16:50写道：
>
> On Thu, Aug 08, 2024 at 11:04:11AM +0800, yaozhenguo wrote:
> > qemu will crash in live migration cleanup process at source host.
> > BT is as below:
> >
> > 0  0x00007f740fc9e165 in g_source_destroy () at /usr/lib64/libglib-2.0.so.0
> > 1  0x000055a2982a0f6e in qio_net_listener_set_client_func_full
> > 2  0x000055a298345130 in tcp_chr_update_read_handler
> > 3  0x000055a298341598 in qemu_chr_fe_set_handlers_full
> > 4  0x000055a298341655 in qemu_chr_fe_set_handlers
> > 5  0x000055a298191e75 in vhost_user_blk_event
> > 6  0x000055a298292b79 in object_deinit
> > 7  object_finalize
> > 8  object_unref
> > 9  0x000055a298292b3c in object_property_del_all
> > 10 object_finalize
> > 11 object_unref
> > 12 0x000055a298291d7d in object_property_del_child
> > 13 object_unparent
> > 14 0x000055a29834a3c4 in qemu_chr_cleanup
> > 15 0x000055a298160d87 in qemu_cleanup
> > 16 0x000055a297e6bff1 in main
> >
> > Crash reason is that qio_net_listener_finalize is called before
> > qio_net_listener_set_client_func_full. so, listener->io_source
> > is used after free. fix this by adding more checks.
>
> If finalize() has been called, then not only has listener->io_source
> been freed, but 'listener' itself has also been freed, thus....
>
> >
> > Signed-off-by: yaozhenguo <yaozhenguo@jd.com>
> > ---
> >  io/net-listener.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/io/net-listener.c b/io/net-listener.c
> > index 47405965a6..c02965f919 100644
> > --- a/io/net-listener.c
> > +++ b/io/net-listener.c
> > @@ -143,6 +143,11 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
> >  {
> >      size_t i;
> >
> > +
> > +    if (!listener->nsioc || !listener->io_source || !listener->name) {
> > +        return;
> > +    }
>
> ....this is still accessing freed memory for 'listener'.
>
>
> What is the call path of the stack triggering qio_net_listener_finalize ?
>
> Whatever callpath has done that needs to be setting SocketChardev->listener
> field to NULL, because tcp_chr_update_read_handler will check for NULL
> before calling qio_net_listener_set_client_func_full.
>
> > +
> >      if (listener->io_notify) {
> >          listener->io_notify(listener->io_data);
> >      }
> > @@ -264,6 +269,10 @@ void qio_net_listener_disconnect(QIONetListener *listener)
> >  {
> >      size_t i;
> >
> > +    if (!listener->nsioc || !listener->io_source || !listener->name) {
> > +        return;
> > +    }
> > +
> >      if (!listener->connected) {
> >          return;
> >      }
> > @@ -301,6 +310,10 @@ static void qio_net_listener_finalize(Object *obj)
> >      g_free(listener->io_source);
> >      g_free(listener->sioc);
> >      g_free(listener->name);
> > +
> > +    listener->io_source = NULL;
> > +    listener->sioc = NULL;
> > +    listener->name = NULL;
> >  }
> >
> >  static const TypeInfo qio_net_listener_info = {
> > --
> > 2.43.0
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>