| Message ID | 20231017182545.97973-4-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update QEMU qnic driver to "new" XenDevice model | expand |
On Tue, 2023-10-17 at 19:25 +0100, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > When QEMU is exiting, qemu_cleanup() calls net_cleanup(), which deletes > the NIC from underneath the xen-net-device. When xen_netdev_unrealize() > is later called via the xenbus exit notifier, it crashes. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > hw/net/xen_nic.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c > index 84914c329c..8d25fb3101 100644 > --- a/hw/net/xen_nic.c > +++ b/hw/net/xen_nic.c > @@ -25,6 +25,8 @@ > #include "qapi/qmp/qdict.h" > #include "qapi/error.h" > > +#include "sysemu/runstate.h" > + > #include <sys/socket.h> > #include <sys/ioctl.h> > #include <sys/wait.h> > @@ -530,7 +532,11 @@ static void xen_netdev_unrealize(XenDevice *xendev) > /* Disconnect from the frontend in case this has not already happened */ > xen_netdev_disconnect(xendev, NULL); > > - if (netdev->nic) { > + /* > + * WTF? In RUN_STATE_SHUTDOWN, qemu_cleanup()→net_cleanup() already deleted > + * our NIC from underneath us! > + */ > + if (netdev->nic && !runstate_check(RUN_STATE_SHUTDOWN)) { > qemu_del_nic(netdev->nic); > } > } I wonder if this is the better answer? There's no point deleting the *NICs*, is there? It's the other net clients we really want to clean up? --- a/net/net.c +++ b/net/net.c @@ -1499,18 +1499,22 @@ static void net_vm_change_state_handler(void *opaque, bool running, void net_cleanup(void) { - NetClientState *nc; + NetClientState *nc, **p = &net_clients.tqh_first; /*cleanup colo compare module for COLO*/ colo_compare_cleanup(); - /* We may del multiple entries during qemu_del_net_client(), - * so QTAILQ_FOREACH_SAFE() is also not safe here. + /* + * We may del multiple entries during qemu_del_net_client(), so + * QTAILQ_FOREACH_SAFE() is not safe here. The only safe pointer + * to keep is a NET_CLIENT_DRIVER_NIC entry, as we don't want + * to delete those (we'd upset the devices which own them, if we + * did). */ - while (!QTAILQ_EMPTY(&net_clients)) { - nc = QTAILQ_FIRST(&net_clients); + while (*p) { + nc = *p; if (nc->info->type == NET_CLIENT_DRIVER_NIC) { - qemu_del_nic(qemu_get_nic(nc)); + p = &nc->next.tqe_next; } else { qemu_del_net_client(nc); }
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c index 84914c329c..8d25fb3101 100644 --- a/hw/net/xen_nic.c +++ b/hw/net/xen_nic.c @@ -25,6 +25,8 @@ #include "qapi/qmp/qdict.h" #include "qapi/error.h" +#include "sysemu/runstate.h" + #include <sys/socket.h> #include <sys/ioctl.h> #include <sys/wait.h> @@ -530,7 +532,11 @@ static void xen_netdev_unrealize(XenDevice *xendev) /* Disconnect from the frontend in case this has not already happened */ xen_netdev_disconnect(xendev, NULL); - if (netdev->nic) { + /* + * WTF? In RUN_STATE_SHUTDOWN, qemu_cleanup()→net_cleanup() already deleted + * our NIC from underneath us! + */ + if (netdev->nic && !runstate_check(RUN_STATE_SHUTDOWN)) { qemu_del_nic(netdev->nic); } }