| Message ID | 20240808030411.76990-1-yaozhenguo@jd.com |
|---|---|
| State | New |
| Headers | show |
| Series | qio: fix qemu crash when live migration | expand |
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
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 :| >
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 = {
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(+)