[libvirt] [PATCH 1/2] daemon: Fix segfault by reloading daemon right after start
Libvirt could crash with segfault if user issue "service reload" right after "service start". One possible way to crash libvirt is to run reload during initialization of QEMU driver. It could happen when qemu driver will initialize qemu_driver_lock but don't have a time to set it's "config" and the SIGHUP arrives. The reload handler tries to get qemu_drv->config during "virStorageAutostart" and dereference it which ends with segfault. Let's ignore all reload requests until all drivers are initialized. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179981 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/libvirtd.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 86accaa..835e7dc 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -785,6 +785,11 @@ static void daemonReloadHandler(virNetServerPtr srv ATTRIBUTE_UNUSED, siginfo_t *sig ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + if (!driversInitialized) { + VIR_WARN("Drivers are not initialized, reload ignored"); + return; + } + VIR_INFO("Reloading configuration on SIGHUP"); virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL); -- 2.0.5
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1bbbe9b..b6280a5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -644,6 +644,8 @@ qemuStateInitialize(bool privileged, return -1; } + kill(getpid(), SIGHUP); + qemu_driver->inhibitCallback = callback; qemu_driver->inhibitOpaque = opaque; -- 2.0.5
On Wed, Feb 18, 2015 at 04:24:31PM +0100, Pavel Hrdina wrote:
Libvirt could crash with segfault if user issue "service reload" right after "service start". One possible way to crash libvirt is to run reload during initialization of QEMU driver.
It could happen when qemu driver will initialize qemu_driver_lock but don't have a time to set it's "config" and the SIGHUP arrives. The reload handler tries to get qemu_drv->config during "virStorageAutostart" and dereference it which ends with segfault.
Let's ignore all reload requests until all drivers are initialized.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179981
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/libvirtd.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 86accaa..835e7dc 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -785,6 +785,11 @@ static void daemonReloadHandler(virNetServerPtr srv ATTRIBUTE_UNUSED, siginfo_t *sig ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + if (!driversInitialized) { + VIR_WARN("Drivers are not initialized, reload ignored"); + return; + } + VIR_INFO("Reloading configuration on SIGHUP"); virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
Why don't we just not register the reload handler until we get to the point where it is safe. eg register it only after virStateInitialize is complete, and unregister it before we call virStateCleanup. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Feb 18, 2015 at 03:29:35PM +0000, Daniel P. Berrange wrote:
On Wed, Feb 18, 2015 at 04:24:31PM +0100, Pavel Hrdina wrote:
Libvirt could crash with segfault if user issue "service reload" right after "service start". One possible way to crash libvirt is to run reload during initialization of QEMU driver.
It could happen when qemu driver will initialize qemu_driver_lock but don't have a time to set it's "config" and the SIGHUP arrives. The reload handler tries to get qemu_drv->config during "virStorageAutostart" and dereference it which ends with segfault.
Let's ignore all reload requests until all drivers are initialized.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179981
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/libvirtd.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 86accaa..835e7dc 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -785,6 +785,11 @@ static void daemonReloadHandler(virNetServerPtr srv ATTRIBUTE_UNUSED, siginfo_t *sig ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + if (!driversInitialized) { + VIR_WARN("Drivers are not initialized, reload ignored"); + return; + } + VIR_INFO("Reloading configuration on SIGHUP"); virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
Why don't we just not register the reload handler until we get to the point where it is safe. eg register it only after virStateInitialize is complete, and unregister it before we call virStateCleanup.
In that case we should register empty handler for SIGHUP otherwise reload could exit libvirtd during driver initialization. But you have a point that my patch still doesn't fix the issue that reload could be triggered during virStateCleanup. I'll send a v2 where at first we will set empty handler. After virStateInitialize is complete we set the proper reload handler and before virStateCleanup we set again empty handler. Thanks for review. Pavel
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Feb 18, 2015 at 05:08:28PM +0100, Pavel Hrdina wrote:
On Wed, Feb 18, 2015 at 03:29:35PM +0000, Daniel P. Berrange wrote:
On Wed, Feb 18, 2015 at 04:24:31PM +0100, Pavel Hrdina wrote:
Libvirt could crash with segfault if user issue "service reload" right after "service start". One possible way to crash libvirt is to run reload during initialization of QEMU driver.
It could happen when qemu driver will initialize qemu_driver_lock but don't have a time to set it's "config" and the SIGHUP arrives. The reload handler tries to get qemu_drv->config during "virStorageAutostart" and dereference it which ends with segfault.
Let's ignore all reload requests until all drivers are initialized.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179981
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/libvirtd.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 86accaa..835e7dc 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -785,6 +785,11 @@ static void daemonReloadHandler(virNetServerPtr srv ATTRIBUTE_UNUSED, siginfo_t *sig ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + if (!driversInitialized) { + VIR_WARN("Drivers are not initialized, reload ignored"); + return; + } + VIR_INFO("Reloading configuration on SIGHUP"); virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
Why don't we just not register the reload handler until we get to the point where it is safe. eg register it only after virStateInitialize is complete, and unregister it before we call virStateCleanup.
In that case we should register empty handler for SIGHUP otherwise reload could exit libvirtd during driver initialization. But you have a point that my patch still doesn't fix the issue that reload could be triggered during virStateCleanup.
Oh that's a good point. I forgot about default SIGHUP behaviour.
I'll send a v2 where at first we will set empty handler. After virStateInitialize is complete we set the proper reload handler and before virStateCleanup we set again empty handler.
Maybe your current approach is actually simpler than having to create a dummy sighup handler function. We'd just need to reset driversInitialized to false again before virSTateCleanup Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Feb 18, 2015 at 04:13:15PM +0000, Daniel P. Berrange wrote:
On Wed, Feb 18, 2015 at 05:08:28PM +0100, Pavel Hrdina wrote:
On Wed, Feb 18, 2015 at 03:29:35PM +0000, Daniel P. Berrange wrote:
On Wed, Feb 18, 2015 at 04:24:31PM +0100, Pavel Hrdina wrote:
Libvirt could crash with segfault if user issue "service reload" right after "service start". One possible way to crash libvirt is to run reload during initialization of QEMU driver.
It could happen when qemu driver will initialize qemu_driver_lock but don't have a time to set it's "config" and the SIGHUP arrives. The reload handler tries to get qemu_drv->config during "virStorageAutostart" and dereference it which ends with segfault.
Let's ignore all reload requests until all drivers are initialized.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179981
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/libvirtd.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 86accaa..835e7dc 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -785,6 +785,11 @@ static void daemonReloadHandler(virNetServerPtr srv ATTRIBUTE_UNUSED, siginfo_t *sig ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + if (!driversInitialized) { + VIR_WARN("Drivers are not initialized, reload ignored"); + return; + } + VIR_INFO("Reloading configuration on SIGHUP"); virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
Why don't we just not register the reload handler until we get to the point where it is safe. eg register it only after virStateInitialize is complete, and unregister it before we call virStateCleanup.
In that case we should register empty handler for SIGHUP otherwise reload could exit libvirtd during driver initialization. But you have a point that my patch still doesn't fix the issue that reload could be triggered during virStateCleanup.
Oh that's a good point. I forgot about default SIGHUP behaviour.
I'll send a v2 where at first we will set empty handler. After virStateInitialize is complete we set the proper reload handler and before virStateCleanup we set again empty handler.
Maybe your current approach is actually simpler than having to create a dummy sighup handler function. We'd just need to reset driversInitialized to false again before virSTateCleanup
Agree, that's the missing peace. I'll sent v2.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Feb 18, 2015 at 05:08:28PM +0100, Pavel Hrdina wrote:
On Wed, Feb 18, 2015 at 03:29:35PM +0000, Daniel P. Berrange wrote:
On Wed, Feb 18, 2015 at 04:24:31PM +0100, Pavel Hrdina wrote:
Libvirt could crash with segfault if user issue "service reload" right after "service start". One possible way to crash libvirt is to run reload during initialization of QEMU driver.
It could happen when qemu driver will initialize qemu_driver_lock but don't have a time to set it's "config" and the SIGHUP arrives. The reload handler tries to get qemu_drv->config during "virStorageAutostart" and dereference it which ends with segfault.
Let's ignore all reload requests until all drivers are initialized.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179981
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/libvirtd.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 86accaa..835e7dc 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -785,6 +785,11 @@ static void daemonReloadHandler(virNetServerPtr srv ATTRIBUTE_UNUSED, siginfo_t *sig ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + if (!driversInitialized) { + VIR_WARN("Drivers are not initialized, reload ignored"); + return; + } + VIR_INFO("Reloading configuration on SIGHUP"); virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
Why don't we just not register the reload handler until we get to the point where it is safe. eg register it only after virStateInitialize is complete, and unregister it before we call virStateCleanup.
In that case we should register empty handler for SIGHUP otherwise reload could exit libvirtd during driver initialization. But you have a point that my patch still doesn't fix the issue that reload could be triggered during virStateCleanup.
I'll send a v2 where at first we will set empty handler. After virStateInitialize is complete we set the proper reload handler and before virStateCleanup we set again empty handler.
Actually that would require to handle return value of virNetServerAddSignalHandler every time we need to re-register the handler for SIGHUP. So I still think that the first approach is still better. Pavel
Thanks for review.
Pavel
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Libvirt could crash with segfault if user issue "service reload" right after "service start". One possible way to crash libvirt is to run reload during initialization of QEMU driver. It could happen when qemu driver will initialize qemu_driver_lock but don't have a time to set it's "config" and the SIGHUP arrives. The reload handler tries to get qemu_drv->config during "virStorageAutostart" and dereference it which ends with segfault. Let's ignore all reload requests until all drivers are initialized. In addition set "driversInitialized" before we enter "virStateCleanup" to ignore reload request while we are shutting down. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179981 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- diff to v1: - ignore reload also during virStateCleanup daemon/libvirtd.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 86accaa..2366d63 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -785,6 +785,11 @@ static void daemonReloadHandler(virNetServerPtr srv ATTRIBUTE_UNUSED, siginfo_t *sig ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + if (!driversInitialized) { + VIR_WARN("Drivers are not initialized, reload ignored"); + return; + } + VIR_INFO("Reloading configuration on SIGHUP"); virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL); @@ -1519,8 +1524,10 @@ int main(int argc, char **argv) { daemonConfigFree(config); - if (driversInitialized) + if (driversInitialized) { + driversInitialized = false; virStateCleanup(); + } return ret; } -- 2.0.5
On Wed, Feb 18, 2015 at 05:34:39PM +0100, Pavel Hrdina wrote:
Libvirt could crash with segfault if user issue "service reload" right after "service start". One possible way to crash libvirt is to run reload during initialization of QEMU driver.
It could happen when qemu driver will initialize qemu_driver_lock but don't have a time to set it's "config" and the SIGHUP arrives. The reload handler tries to get qemu_drv->config during "virStorageAutostart" and dereference it which ends with segfault.
Let's ignore all reload requests until all drivers are initialized. In addition set "driversInitialized" before we enter "virStateCleanup" to ignore reload request while we are shutting down.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179981
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> ---
diff to v1: - ignore reload also during virStateCleanup
daemon/libvirtd.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Pavel Hrdina