[libvirt] [PATCH 0/2] Fix a virperf related crasher

I've ran into a crasher. Worse crasher over RO connection. Michal Privoznik (2): Drop virPerfGetEventFd virPerfEventIsEnabled: Don't crash on shut off domains src/libvirt_private.syms | 1 - src/util/virperf.c | 22 +++------------------- src/util/virperf.h | 3 --- 3 files changed, 3 insertions(+), 23 deletions(-) -- 2.8.3

This function is not used anywhere. Moreover, the code that would use lives in virperf.c and therefore has access to the FD anyway. Well, for instance virPerfReadEvent is doing just that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virperf.c | 19 ------------------- src/util/virperf.h | 3 --- 3 files changed, 23 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e325168..333bf7c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2083,7 +2083,6 @@ virPerfEventIsEnabled; virPerfEventTypeFromString; virPerfEventTypeToString; virPerfFree; -virPerfGetEventFd; virPerfNew; virPerfReadEvent; diff --git a/src/util/virperf.c b/src/util/virperf.c index 450b3ba..a0997f9 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -241,16 +241,6 @@ bool virPerfEventIsEnabled(virPerfPtr perf, return event->enabled; } -int virPerfGetEventFd(virPerfPtr perf, - virPerfEventType type) -{ - virPerfEventPtr event = virPerfGetEvent(perf, type); - if (event == NULL) - return false; - - return event->fd; -} - int virPerfReadEvent(virPerfPtr perf, virPerfEventType type, @@ -300,15 +290,6 @@ virPerfEventIsEnabled(virPerfPtr perf ATTRIBUTE_UNUSED, } int -virPerfGetEventFd(virPerfPtr perf ATTRIBUTE_UNUSED, - virPerfEventType type ATTRIBUTE_UNUSED) -{ - virReportSystemError(ENXIO, "%s", - _("Perf not supported on this platform")); - return -1; -} - -int virPerfReadEvent(virPerfPtr perf ATTRIBUTE_UNUSED, virPerfEventType type ATTRIBUTE_UNUSED, uint64_t *value ATTRIBUTE_UNUSED) diff --git a/src/util/virperf.h b/src/util/virperf.h index 769e85a..7163410 100644 --- a/src/util/virperf.h +++ b/src/util/virperf.h @@ -51,9 +51,6 @@ int virPerfEventDisable(virPerfPtr perf, bool virPerfEventIsEnabled(virPerfPtr perf, virPerfEventType type); -int virPerfGetEventFd(virPerfPtr perf, - virPerfEventType type); - int virPerfReadEvent(virPerfPtr perf, virPerfEventType type, uint64_t *value); -- 2.8.3

On Fri, Jun 03, 2016 at 11:02:51 +0200, Michal Privoznik wrote:
This function is not used anywhere. Moreover, the code that would use lives in virperf.c and therefore has access to the FD anyway. Well, for instance virPerfReadEvent is doing just that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virperf.c | 19 ------------------- src/util/virperf.h | 3 --- 3 files changed, 23 deletions(-)
ACK

So imagine the following. You connect read only to a daemon and try to fetch stats for a shut off domain, e.g.: virsh -r domstats $dom but all of a sudden, virsh instead of printing the stats throws the following error at you: error: Disconnected from qemu:///system due to I/O error error: End of file while reading data: Input/output error The daemon crashed. This is its backtrace: #0 0x00007fa43e3751a8 in virPerfEventIsEnabled (perf=0x0, type=VIR_PERF_EVENT_MBMT) at util/virperf.c:241 #1 0x00007fa424a9f042 in qemuDomainGetStatsPerf (driver=0x7fa3f4022a30, dom=0x7fa3f40e24c0, record=0x7fa41c000e20, maxparams=0x7fa4360b38d0, privflags=1) at qemu/qemu_driver.c:19110 #2 0x00007fa424a9f2e7 in qemuDomainGetStats (conn=0x7fa41c001b20, dom=0x7fa3f40e24c0, stats=127, record=0x7fa4360b3970, flags=1) at qemu/qemu_driver.c:19213 #3 0x00007fa424a9f672 in qemuConnectGetAllDomainStats (conn=0x7fa41c001b20, doms=0x7fa41c0017f0, ndoms=1, stats=127, retStats=0x7fa4360b3a50, flags=0) at qemu/qemu_driver.c:19303 #4 0x00007fa43e4e15f6 in virDomainListGetStats (doms=0x7fa41c0017f0, stats=0, retStats=0x7fa4360b3a50, flags=0) at libvirt-domain.c:11615 Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7f28d1a38700 (LWP 16154)] 0x00007f28da4fa1a8 in virPerfEventIsEnabled (perf=0x0, type=VIR_PERF_EVENT_MBMT) at util/virperf.c:241 241 return event->enabled; Problem is, shut off domains don't have priv->perf allocated. Therefore if in frame #1 qemuDomainGetStatsPerf() tries to check if perf events are enabled, NULL is passed to virPerfEventIsEnabled() which due to some incredible implementation dereference it. Fix this by checking whether passed object is not NULL. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virperf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virperf.c b/src/util/virperf.c index a0997f9..4661ba3 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -99,6 +99,9 @@ static virPerfEventPtr virPerfGetEvent(virPerfPtr perf, virPerfEventType type) { + if (!perf) + return NULL; + if (type >= VIR_PERF_EVENT_LAST) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Event '%d' is not supported"), -- 2.8.3

On Fri, Jun 03, 2016 at 11:02:52 +0200, Michal Privoznik wrote:
So imagine the following. You connect read only to a daemon and try to fetch stats for a shut off domain, e.g.:
virsh -r domstats $dom
but all of a sudden, virsh instead of printing the stats throws the following error at you:
error: Disconnected from qemu:///system due to I/O error error: End of file while reading data: Input/output error
The daemon crashed. This is its backtrace:
#0 0x00007fa43e3751a8 in virPerfEventIsEnabled (perf=0x0, type=VIR_PERF_EVENT_MBMT) at util/virperf.c:241 #1 0x00007fa424a9f042 in qemuDomainGetStatsPerf (driver=0x7fa3f4022a30, dom=0x7fa3f40e24c0, record=0x7fa41c000e20, maxparams=0x7fa4360b38d0, privflags=1) at qemu/qemu_driver.c:19110 #2 0x00007fa424a9f2e7 in qemuDomainGetStats (conn=0x7fa41c001b20, dom=0x7fa3f40e24c0, stats=127, record=0x7fa4360b3970, flags=1) at qemu/qemu_driver.c:19213 #3 0x00007fa424a9f672 in qemuConnectGetAllDomainStats (conn=0x7fa41c001b20, doms=0x7fa41c0017f0, ndoms=1, stats=127, retStats=0x7fa4360b3a50, flags=0) at qemu/qemu_driver.c:19303 #4 0x00007fa43e4e15f6 in virDomainListGetStats (doms=0x7fa41c0017f0, stats=0, retStats=0x7fa4360b3a50, flags=0) at libvirt-domain.c:11615
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7f28d1a38700 (LWP 16154)] 0x00007f28da4fa1a8 in virPerfEventIsEnabled (perf=0x0, type=VIR_PERF_EVENT_MBMT) at util/virperf.c:241 241 return event->enabled;
Problem is, shut off domains don't have priv->perf allocated. Therefore if in frame #1 qemuDomainGetStatsPerf() tries to check if perf events are enabled, NULL is passed to virPerfEventIsEnabled() which due to some incredible implementation dereference it. Fix this by checking whether passed object is not NULL.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virperf.c | 3 +++ 1 file changed, 3 insertions(+)
ACK, there might be some instances where we would report an error and ignore it or not report it and return one but that's good for a later cleanup.
participants (2)
-
Michal Privoznik
-
Peter Krempa