* [PATCH 0/3] Misc fixes for 8.2
@ 2023-11-15 17:24 David Woodhouse
2023-11-15 17:24 ` [PATCH 1/3] net: do not delete nics in net_cleanup() David Woodhouse
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: David Woodhouse @ 2023-11-15 17:24 UTC (permalink / raw
To: qemu-devel
Cc: Stefano Stabellini, Anthony Perard, Paul Durrant, Kevin Wolf,
Hanna Reitz, Jason Wang, Paolo Bonzini, xen-devel, qemu-block
Fix a use-after-free (or double-free) due to net_cleanup() freeing NICs
that don't belong to it, fix a newly-introduced launch failure with a
documented command line, and clean up code to avoid a Coverity warning.
David Woodhouse (3):
net: do not delete nics in net_cleanup()
vl: disable default serial when xen-console is enabled
hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive
hw/block/xen-block.c | 24 +++++++++++++++++++++---
net/net.c | 28 ++++++++++++++++++++++------
system/vl.c | 1 +
3 files changed, 44 insertions(+), 9 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] net: do not delete nics in net_cleanup()
2023-11-15 17:24 [PATCH 0/3] Misc fixes for 8.2 David Woodhouse
@ 2023-11-15 17:24 ` David Woodhouse
2023-11-15 17:24 ` [PATCH 2/3] vl: disable default serial when xen-console is enabled David Woodhouse
2023-11-15 17:24 ` [PATCH 3/3] hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive David Woodhouse
2 siblings, 0 replies; 9+ messages in thread
From: David Woodhouse @ 2023-11-15 17:24 UTC (permalink / raw
To: qemu-devel
Cc: Stefano Stabellini, Anthony Perard, Paul Durrant, Kevin Wolf,
Hanna Reitz, Jason Wang, Paolo Bonzini, xen-devel, qemu-block
From: David Woodhouse <dwmw@amazon.co.uk>
In net_cleanup() we only need to delete the netdevs, as those may have
state which outlives Qemu when it exits, and thus may actually need to
be cleaned up on exit.
The nics, on the other hand, are owned by the device which created them.
Most devices don't bother to clean up on exit because they don't have
any state which will outlive Qemu... but XenBus devices do need to clean
up their nodes in XenStore, and do have an exit handler to delete them.
When the XenBus exit handler destroys the xen-net-device, it attempts
to delete its nic after net_cleanup() had already done so. And crashes.
Fix this by only deleting netdevs as we walk the list. As the comment
notes, we can't use QTAILQ_FOREACH_SAFE() as each deletion may remove
*multiple* entries, including the "safely" saved 'next' pointer. But
we can store the *previous* entry, since nics are safe.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
---
net/net.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/net/net.c b/net/net.c
index c0c0cbe99e..bbe33da176 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1499,18 +1499,34 @@ static void net_vm_change_state_handler(void *opaque, bool running,
void net_cleanup(void)
{
- NetClientState *nc;
+ NetClientState *nc, **p = &QTAILQ_FIRST(&net_clients);
/*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.
+ /*
+ * Walk the net_clients list and remove the netdevs but *not* any
+ * NET_CLIENT_DRIVER_NIC entries. The latter are owned by the device
+ * model which created them, and in some cases (e.g. xen-net-device)
+ * the device itself may do cleanup at exit and will be upset if we
+ * just delete its NIC from underneath it.
+ *
+ * Since qemu_del_net_client() may delete multiple entries, using
+ * QTAILQ_FOREACH_SAFE() is not safe here. The only safe pointer
+ * to keep as a bookmark is a NET_CLIENT_DRIVER_NIC entry, so keep
+ * 'p' pointing to either the head of the list, or the 'next' field
+ * of the latest NET_CLIENT_DRIVER_NIC, and operate on *p as we walk
+ * the list.
+ *
+ * The 'nc' variable isn't part of the list traversal; it's purely
+ * for convenience as too much '(*p)->' has a tendency to make the
+ * readers' eyes bleed.
*/
- 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));
+ /* Skip NET_CLIENT_DRIVER_NIC entries */
+ p = &QTAILQ_NEXT(nc, next);
} else {
qemu_del_net_client(nc);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] vl: disable default serial when xen-console is enabled
2023-11-15 17:24 [PATCH 0/3] Misc fixes for 8.2 David Woodhouse
2023-11-15 17:24 ` [PATCH 1/3] net: do not delete nics in net_cleanup() David Woodhouse
@ 2023-11-15 17:24 ` David Woodhouse
2023-11-21 10:57 ` Marc-André Lureau
2023-11-21 10:59 ` Paul Durrant
2023-11-15 17:24 ` [PATCH 3/3] hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive David Woodhouse
2 siblings, 2 replies; 9+ messages in thread
From: David Woodhouse @ 2023-11-15 17:24 UTC (permalink / raw
To: qemu-devel
Cc: Stefano Stabellini, Anthony Perard, Paul Durrant, Kevin Wolf,
Hanna Reitz, Jason Wang, Paolo Bonzini, xen-devel, qemu-block
From: David Woodhouse <dwmw@amazon.co.uk>
If a Xen console is configured on the command line, do not add a default
serial port.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
system/vl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/system/vl.c b/system/vl.c
index 5af7ced2a1..8109231834 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -198,6 +198,7 @@ static const struct {
const char *driver;
int *flag;
} default_list[] = {
+ { .driver = "xen-console", .flag = &default_serial },
{ .driver = "isa-serial", .flag = &default_serial },
{ .driver = "isa-parallel", .flag = &default_parallel },
{ .driver = "isa-fdc", .flag = &default_floppy },
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive
2023-11-15 17:24 [PATCH 0/3] Misc fixes for 8.2 David Woodhouse
2023-11-15 17:24 ` [PATCH 1/3] net: do not delete nics in net_cleanup() David Woodhouse
2023-11-15 17:24 ` [PATCH 2/3] vl: disable default serial when xen-console is enabled David Woodhouse
@ 2023-11-15 17:24 ` David Woodhouse
2023-11-15 20:45 ` Paul Durrant
2 siblings, 1 reply; 9+ messages in thread
From: David Woodhouse @ 2023-11-15 17:24 UTC (permalink / raw
To: qemu-devel
Cc: Stefano Stabellini, Anthony Perard, Paul Durrant, Kevin Wolf,
Hanna Reitz, Jason Wang, Paolo Bonzini, xen-devel, qemu-block
From: David Woodhouse <dwmw@amazon.co.uk>
Coverity couldn't see that nr_existing was always going to be zero when
qemu_xen_xs_directory() returned NULL in the ENOENT case (CID 1523906).
Perhaps more to the point, neither could Peter at first glance. Improve
the code to hopefully make it clearer to Coverity and human reviewers
alike.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
hw/block/xen-block.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 6d64ede94f..aed1d5c330 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -91,9 +91,27 @@ static bool xen_block_find_free_vdev(XenBlockDevice *blockdev, Error **errp)
existing_frontends = qemu_xen_xs_directory(xenbus->xsh, XBT_NULL, fe_path,
&nr_existing);
- if (!existing_frontends && errno != ENOENT) {
- error_setg_errno(errp, errno, "cannot read %s", fe_path);
- return false;
+ if (!existing_frontends) {
+ if (errno == ENOENT) {
+ /*
+ * If the frontend directory doesn't exist because there are
+ * no existing vbd devices, that's fine. Just ensure that we
+ * don't dereference the NULL existing_frontends pointer, by
+ * checking that nr_existing is zero so the loop below is not
+ * entered.
+ *
+ * In fact this is redundant since nr_existing is initialized
+ * to zero, but setting it again here makes it abundantly clear
+ * to Coverity, and to the human reader who doesn't know the
+ * semantics of qemu_xen_xs_directory() off the top of their
+ * head.
+ */
+ nr_existing = 0;
+ } else {
+ /* All other errors accessing the frontend directory are fatal. */
+ error_setg_errno(errp, errno, "cannot read %s", fe_path);
+ return false;
+ }
}
memset(used_devs, 0, sizeof(used_devs));
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive
2023-11-15 17:24 ` [PATCH 3/3] hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive David Woodhouse
@ 2023-11-15 20:45 ` Paul Durrant
0 siblings, 0 replies; 9+ messages in thread
From: Paul Durrant @ 2023-11-15 20:45 UTC (permalink / raw
To: David Woodhouse, qemu-devel
Cc: Stefano Stabellini, Anthony Perard, Kevin Wolf, Hanna Reitz,
Jason Wang, Paolo Bonzini, xen-devel, qemu-block
On 15/11/2023 12:24, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> Coverity couldn't see that nr_existing was always going to be zero when
> qemu_xen_xs_directory() returned NULL in the ENOENT case (CID 1523906).
>
> Perhaps more to the point, neither could Peter at first glance. Improve
> the code to hopefully make it clearer to Coverity and human reviewers
> alike.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> hw/block/xen-block.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
Reviewed-by: Paul Durrant <paul@xen.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled
2023-11-15 17:24 ` [PATCH 2/3] vl: disable default serial when xen-console is enabled David Woodhouse
@ 2023-11-21 10:57 ` Marc-André Lureau
2023-11-21 10:58 ` Marc-André Lureau
2023-11-21 10:59 ` Paul Durrant
1 sibling, 1 reply; 9+ messages in thread
From: Marc-André Lureau @ 2023-11-21 10:57 UTC (permalink / raw
To: David Woodhouse
Cc: qemu-devel, Stefano Stabellini, Anthony Perard, Paul Durrant,
Kevin Wolf, Hanna Reitz, Jason Wang, Paolo Bonzini, xen-devel,
qemu-block
Hi
On Wed, Nov 15, 2023 at 9:28 PM David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> If a Xen console is configured on the command line, do not add a default
> serial port.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> system/vl.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/system/vl.c b/system/vl.c
> index 5af7ced2a1..8109231834 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -198,6 +198,7 @@ static const struct {
> const char *driver;
> int *flag;
> } default_list[] = {
> + { .driver = "xen-console", .flag = &default_serial },
> { .driver = "isa-serial", .flag = &default_serial },
> { .driver = "isa-parallel", .flag = &default_parallel },
> { .driver = "isa-fdc", .flag = &default_floppy },
Consistent with the rest of the lines (no conditional compilation nor
driver #define..)
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
btw, while quickly testing this (do we have any test for xen-console?):
$ qemu --accel kvm,xen-version=0x40011,kernel-irqchip=split -device
xen-console,chardev=foo -chardev stdio,id=foo
(and close gtk window)
Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
387 if (nc->incoming_queue) {
(gdb) bt
#0 0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
#1 0x0000555555c11a14 in qemu_del_nic (nic=0x555558b6f930) at ../net/net.c:459
#2 0x00005555559e398b in xen_netdev_unrealize (xendev=0x555558b6b510)
at ../hw/net/xen_nic.c:550
#3 0x0000555555b6e22f in xen_device_unrealize (dev=0x555558b6b510) at
../hw/xen/xen-bus.c:973
#4 0x0000555555b6e351 in xen_device_exit (n=0x555558b6b5e0, data=0x0)
at ../hw/xen/xen-bus.c:1002
#5 0x00005555560bc3fc in notifier_list_notify (list=0x5555570b5fc0
<exit_notifiers>, data=0x0) at ../util/notify.c:39
#6 0x0000555555ba1d49 in qemu_run_exit_notifiers () at ../system/runstate.c:800
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled
2023-11-21 10:57 ` Marc-André Lureau
@ 2023-11-21 10:58 ` Marc-André Lureau
2023-11-21 11:42 ` David Woodhouse
0 siblings, 1 reply; 9+ messages in thread
From: Marc-André Lureau @ 2023-11-21 10:58 UTC (permalink / raw
To: David Woodhouse
Cc: qemu-devel, Stefano Stabellini, Anthony Perard, Paul Durrant,
Kevin Wolf, Hanna Reitz, Jason Wang, Paolo Bonzini, xen-devel,
qemu-block
Hi
On Tue, Nov 21, 2023 at 2:57 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Wed, Nov 15, 2023 at 9:28 PM David Woodhouse <dwmw2@infradead.org> wrote:
> >
> > From: David Woodhouse <dwmw@amazon.co.uk>
> >
> > If a Xen console is configured on the command line, do not add a default
> > serial port.
> >
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> > system/vl.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/system/vl.c b/system/vl.c
> > index 5af7ced2a1..8109231834 100644
> > --- a/system/vl.c
> > +++ b/system/vl.c
> > @@ -198,6 +198,7 @@ static const struct {
> > const char *driver;
> > int *flag;
> > } default_list[] = {
> > + { .driver = "xen-console", .flag = &default_serial },
> > { .driver = "isa-serial", .flag = &default_serial },
> > { .driver = "isa-parallel", .flag = &default_parallel },
> > { .driver = "isa-fdc", .flag = &default_floppy },
>
> Consistent with the rest of the lines (no conditional compilation nor
> driver #define..)
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> btw, while quickly testing this (do we have any test for xen-console?):
>
> $ qemu --accel kvm,xen-version=0x40011,kernel-irqchip=split -device
> xen-console,chardev=foo -chardev stdio,id=foo
> (and close gtk window)
>
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> 387 if (nc->incoming_queue) {
> (gdb) bt
> #0 0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> #1 0x0000555555c11a14 in qemu_del_nic (nic=0x555558b6f930) at ../net/net.c:459
> #2 0x00005555559e398b in xen_netdev_unrealize (xendev=0x555558b6b510)
> at ../hw/net/xen_nic.c:550
> #3 0x0000555555b6e22f in xen_device_unrealize (dev=0x555558b6b510) at
> ../hw/xen/xen-bus.c:973
> #4 0x0000555555b6e351 in xen_device_exit (n=0x555558b6b5e0, data=0x0)
> at ../hw/xen/xen-bus.c:1002
> #5 0x00005555560bc3fc in notifier_list_notify (list=0x5555570b5fc0
> <exit_notifiers>, data=0x0) at ../util/notify.c:39
> #6 0x0000555555ba1d49 in qemu_run_exit_notifiers () at ../system/runstate.c:800
Ok, I found related "[PATCH 1/3] net: do not delete nics in net_cleanup()"
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled
2023-11-15 17:24 ` [PATCH 2/3] vl: disable default serial when xen-console is enabled David Woodhouse
2023-11-21 10:57 ` Marc-André Lureau
@ 2023-11-21 10:59 ` Paul Durrant
1 sibling, 0 replies; 9+ messages in thread
From: Paul Durrant @ 2023-11-21 10:59 UTC (permalink / raw
To: David Woodhouse, qemu-devel
Cc: Stefano Stabellini, Anthony Perard, Kevin Wolf, Hanna Reitz,
Jason Wang, Paolo Bonzini, xen-devel, qemu-block
On 15/11/2023 17:24, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> If a Xen console is configured on the command line, do not add a default
> serial port.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> system/vl.c | 1 +
> 1 file changed, 1 insertion(+)
>
Reviewed-by: Paul Durrant <paul@xen.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled
2023-11-21 10:58 ` Marc-André Lureau
@ 2023-11-21 11:42 ` David Woodhouse
0 siblings, 0 replies; 9+ messages in thread
From: David Woodhouse @ 2023-11-21 11:42 UTC (permalink / raw
To: Marc-André Lureau
Cc: qemu-devel, Stefano Stabellini, Anthony Perard, Paul Durrant,
Kevin Wolf, Hanna Reitz, Jason Wang, Paolo Bonzini, xen-devel,
qemu-block
[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]
On Tue, 2023-11-21 at 14:58 +0400, Marc-André Lureau wrote:
>
> > Consistent with the rest of the lines (no conditional compilation nor
> > driver #define..)
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Thanks.
> > btw, while quickly testing this (do we have any test for xen-console?):
> >
> > $ qemu --accel kvm,xen-version=0x40011,kernel-irqchip=split -device
> > xen-console,chardev=foo -chardev stdio,id=foo
> > (and close gtk window)
> >
> > Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> > 0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> > 387 if (nc->incoming_queue) {
> > (gdb) bt
> > #0 0x0000555555c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> > #1 0x0000555555c11a14 in qemu_del_nic (nic=0x555558b6f930) at ../net/net.c:459
> > #2 0x00005555559e398b in xen_netdev_unrealize (xendev=0x555558b6b510)
> > at ../hw/net/xen_nic.c:550
> > #3 0x0000555555b6e22f in xen_device_unrealize (dev=0x555558b6b510) at
> > ../hw/xen/xen-bus.c:973
> > #4 0x0000555555b6e351 in xen_device_exit (n=0x555558b6b5e0, data=0x0)
> > at ../hw/xen/xen-bus.c:1002
> > #5 0x00005555560bc3fc in notifier_list_notify (list=0x5555570b5fc0
> > <exit_notifiers>, data=0x0) at ../util/notify.c:39
> > #6 0x0000555555ba1d49 in qemu_run_exit_notifiers () at ../system/runstate.c:800
>
> Ok, I found related "[PATCH 1/3] net: do not delete nics in net_cleanup()"
Yep, and I think I saw that go by in a pull request not many hours ago,
so it should be fixed by -rc1. Thanks for testing.
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-11-21 11:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-15 17:24 [PATCH 0/3] Misc fixes for 8.2 David Woodhouse
2023-11-15 17:24 ` [PATCH 1/3] net: do not delete nics in net_cleanup() David Woodhouse
2023-11-15 17:24 ` [PATCH 2/3] vl: disable default serial when xen-console is enabled David Woodhouse
2023-11-21 10:57 ` Marc-André Lureau
2023-11-21 10:58 ` Marc-André Lureau
2023-11-21 11:42 ` David Woodhouse
2023-11-21 10:59 ` Paul Durrant
2023-11-15 17:24 ` [PATCH 3/3] hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive David Woodhouse
2023-11-15 20:45 ` Paul Durrant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.