diff mbox series

[v7,08/10] tools/misc: Add xen-vmtrace tool

Message ID 20210121212718.2441-9-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series Implement support for external IPT monitoring | expand

Commit Message

Andrew Cooper Jan. 21, 2021, 9:27 p.m. UTC
From: Michał Leszczyński <michal.leszczynski@cert.pl>

Add an demonstration tool that uses xc_vmtrace_* calls in order
to manage external IPT monitoring for DomU.

Signed-off-by: Michał Leszczyński <michal.leszczynski@cert.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
---
 tools/misc/.gitignore    |   1 +
 tools/misc/Makefile      |   4 ++
 tools/misc/xen-vmtrace.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+)
 create mode 100644 tools/misc/xen-vmtrace.c

Comments

Ian Jackson Jan. 22, 2021, 3:33 p.m. UTC | #1
Andrew Cooper writes ("[PATCH v7 08/10] tools/misc: Add xen-vmtrace tool"):
> From: Michał Leszczyński <michal.leszczynski@cert.pl>
...
> +    if ( signal(SIGINT, int_handler) == SIG_ERR )
> +        err(1, "Failed to register signal handler\n");

How bad is it if this signal handler is not effective ?

> +    if ( xc_vmtrace_disable(xch, domid, vcpu) )
> +        perror("xc_vmtrace_disable()");

I guess the tracing will remain on, pointlessly, which has a perf
impact but nothing else ?

How is it possible for the user to clean this up ?

Also: at the very least, you need to trap SIGTERM SIGHUP SIGPIPE.

It would be good to exit with the right signal by re-raising it.

> +static volatile int interrupted = 0;

sig_atomic_t

Ian.
Andrew Cooper Jan. 25, 2021, 3:30 p.m. UTC | #2
On 22/01/2021 15:33, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH v7 08/10] tools/misc: Add xen-vmtrace tool"):
>> From: Michał Leszczyński <michal.leszczynski@cert.pl>
> ...
>> +    if ( signal(SIGINT, int_handler) == SIG_ERR )
>> +        err(1, "Failed to register signal handler\n");
> How bad is it if this signal handler is not effective ?

I believe far less so now that I've fixed up everything to use a (fixed)
XENMEM_acquire_resource, so Xen doesn't crash if this process dies in
the wrong order WRT the domain shutting down.

But I would have to defer to Michał on that.

>> +    if ( xc_vmtrace_disable(xch, domid, vcpu) )
>> +        perror("xc_vmtrace_disable()");
> I guess the tracing will remain on, pointlessly, which has a perf
> impact but nothing else ?

The perf hit is substantial, but it is safe to leave enabled.

> How is it possible for the user to clean this up ?

For now, enable/disable can only fail with -EINVAL for calls made in the
wrong context, so a failure here is benign in practice.

I specifically didn't opt for reference counting the enable/disable
calls, because there cannot (usefully) be two users of this interface.

>
> Also: at the very least, you need to trap SIGTERM SIGHUP SIGPIPE.
>
> It would be good to exit with the right signal by re-raising it.

This is example code, not a production utility.

Anything more production-wise using this needs to account for the fact
that Intel Processor Trace can't pause on a full buffer.  (It ought to
be able to on forthcoming hardware, but this facility isn't available yet.)

The use-cases thus far are always "small delta of execution between
introspection events", using a massive buffer as the mitigation for
hardware wrapping.

No amount of additional code here can prevent stream corruption problems
with the buffer wrapping.  As a result, it is kept as simple as possible
as a demonstration of how to use the API.

~Andrew
Ian Jackson Jan. 26, 2021, 11:59 a.m. UTC | #3
Andrew Cooper writes ("Re: [PATCH v7 08/10] tools/misc: Add xen-vmtrace tool"):
> On 22/01/2021 15:33, Ian Jackson wrote:
> > Andrew Cooper writes ("[PATCH v7 08/10] tools/misc: Add xen-vmtrace tool"):
> >> +    if ( xc_vmtrace_disable(xch, domid, vcpu) )
> >> +        perror("xc_vmtrace_disable()");
> > I guess the tracing will remain on, pointlessly, which has a perf
> > impact but nothing else ?
> 
> The perf hit is substantial, but it is safe to leave enabled.

And how does one clean up after an unclean exit of this program ?

> > How is it possible for the user to clean this up ?
> 
> For now, enable/disable can only fail with -EINVAL for calls made in the
> wrong context, so a failure here is benign in practice.
> 
> I specifically didn't opt for reference counting the enable/disable
> calls, because there cannot (usefully) be two users of this interface.

Right.

> > Also: at the very least, you need to trap SIGTERM SIGHUP SIGPIPE.
> >
> > It would be good to exit with the right signal by re-raising it.
> 
> This is example code, not a production utility.

Perhaps the utility could print some kind of health warning about it
possibly leaving this perf-impacting feature enabled, and how to clean
up ?

Ian.
Andrew Cooper Jan. 26, 2021, 12:55 p.m. UTC | #4
On 26/01/2021 11:59, Ian Jackson wrote:
>>> Also: at the very least, you need to trap SIGTERM SIGHUP SIGPIPE.
>>>
>>> It would be good to exit with the right signal by re-raising it.
>> This is example code, not a production utility.
> Perhaps the utility could print some kind of health warning about it
> possibly leaving this perf-impacting feature enabled, and how to clean
> up ?

Why?  The theoretical fallout here is not conceptually different to
libxl or qemu segfaulting, or any of the myriad other random utilities
we have.

Printing "Warning - this program, just like everything else in the Xen
tree, might in exceptional circumstances segfault and leave the domain
in a weird state" is obvious, and doesn't need stating.

The domain is stuffed. `xl destroy` may or may not make the problem go away.

~Andrew
Ian Jackson Jan. 26, 2021, 1:32 p.m. UTC | #5
Andrew Cooper writes ("Re: [PATCH v7 08/10] tools/misc: Add xen-vmtrace tool"):
> On 26/01/2021 11:59, Ian Jackson wrote:
> >[Andrew:]
> >> This is example code, not a production utility.
> > Perhaps the utility could print some kind of health warning about it
> > possibly leaving this perf-impacting feature enabled, and how to clean
> > up ?
> 
> Why?  The theoretical fallout here is not conceptually different to
> libxl or qemu segfaulting, or any of the myriad other random utilities
> we have.
> 
> Printing "Warning - this program, just like everything else in the Xen
> tree, might in exceptional circumstances segfault and leave the domain
> in a weird state" is obvious, and doesn't need stating.
> 
> The domain is stuffed. `xl destroy` may or may not make the problem go away.

Firstly, I don't agree with this pessimistic analysis of our current
tooling.  Secondly, I would consider many such behaviours bugs;
certainly we have bugs but we shouldn't introduce more of them.

You are justifying the poor robustness of this tool on the grounds
that it's "example code, not a production utility".

But we are shipping it to bin/ and there is nothing telling anyone
that trying to use it (perhaps wrapped in something of their own
devising) is a bad idea.

Either this is code users might be expected to run in production in
which we need to make it at least have a minimal level of engineering
robustness (which is perhaps too difficult at this stage), or we need
to communicate to our users that it's a programming example, not a
useful utility.

Note that *even if it is a programming example*, we should highlight
its most important deficiencies.  Otherwise it is a hazardously
misleading example.

I hope that makes sense.

Thanks,
Ian.
Andrew Cooper Jan. 26, 2021, 3:59 p.m. UTC | #6
On 26/01/2021 13:32, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH v7 08/10] tools/misc: Add xen-vmtrace tool"):
>> On 26/01/2021 11:59, Ian Jackson wrote:
>>> [Andrew:]
>>>> This is example code, not a production utility.
>>> Perhaps the utility could print some kind of health warning about it
>>> possibly leaving this perf-impacting feature enabled, and how to clean
>>> up ?
>> Why?  The theoretical fallout here is not conceptually different to
>> libxl or qemu segfaulting, or any of the myriad other random utilities
>> we have.
>>
>> Printing "Warning - this program, just like everything else in the Xen
>> tree, might in exceptional circumstances segfault and leave the domain
>> in a weird state" is obvious, and doesn't need stating.
>>
>> The domain is stuffed. `xl destroy` may or may not make the problem go away.
> Firstly, I don't agree with this pessimistic analysis of our current
> tooling.  Secondly, I would consider many such behaviours bugs;
> certainly we have bugs but we shouldn't introduce more of them.
>
> You are justifying the poor robustness of this tool on the grounds
> that it's "example code, not a production utility".
>
> But we are shipping it to bin/ and there is nothing telling anyone
> that trying to use it (perhaps wrapped in something of their own
> devising) is a bad idea.
>
> Either this is code users might be expected to run in production in
> which we need to make it at least have a minimal level of engineering
> robustness (which is perhaps too difficult at this stage), or we need
> to communicate to our users that it's a programming example, not a
> useful utility.
>
> Note that *even if it is a programming example*, we should highlight
> its most important deficiencies.  Otherwise it is a hazardously
> misleading example.
>
> I hope that makes sense.

First of all - I'm not the author of this code.  I'm merely the person
who did the latest round of cleanup, and sent the series.

This code is a damn sight more robust than the other utilities, because
in the case that something does go wrong, the domain will still function
correctly.  Almost everything else, qemu included, will leave the VM
totally broken, as it becomes paused waiting on a request which has
escaped into the ether.


I also don't feel that now is an appropriate time to be insisting that
the goalpost's move when it comes to submissions into tools/,
particularly as you were happy (well - didn't comment on) with this
pattern back in v3.

What exact colour do you want this bikeshed?  Anything non-trivial is
going to miss 4.15.

~Andrew
diff mbox series

Patch

diff --git a/tools/misc/.gitignore b/tools/misc/.gitignore
index b2c3b21f57..ce6f937d0c 100644
--- a/tools/misc/.gitignore
+++ b/tools/misc/.gitignore
@@ -1,3 +1,4 @@ 
 xen-access
 xen-memshare
 xen-ucode
+xen-vmtrace
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 912c5d4f0e..c5017e6c70 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -25,6 +25,7 @@  INSTALL_SBIN-$(CONFIG_X86)     += xen-lowmemd
 INSTALL_SBIN-$(CONFIG_X86)     += xen-memshare
 INSTALL_SBIN-$(CONFIG_X86)     += xen-mfndump
 INSTALL_SBIN-$(CONFIG_X86)     += xen-ucode
+INSTALL_SBIN-$(CONFIG_X86)     += xen-vmtrace
 INSTALL_SBIN                   += xencov
 INSTALL_SBIN                   += xenhypfs
 INSTALL_SBIN                   += xenlockprof
@@ -90,6 +91,9 @@  xen-hvmcrash: xen-hvmcrash.o
 xen-memshare: xen-memshare.o
 	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
 
+xen-vmtrace: xen-vmtrace.o
+	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS)
+
 xenperf: xenperf.o
 	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
 
diff --git a/tools/misc/xen-vmtrace.c b/tools/misc/xen-vmtrace.c
new file mode 100644
index 0000000000..47fea871cf
--- /dev/null
+++ b/tools/misc/xen-vmtrace.c
@@ -0,0 +1,154 @@ 
+/******************************************************************************
+ * tools/vmtrace.c
+ *
+ * Demonstrative tool for collecting Intel Processor Trace data from Xen.
+ *  Could be used to externally monitor a given vCPU in given DomU.
+ *
+ * Copyright (C) 2020 by CERT Polska - NASK PIB
+ *
+ * Authors: Michał Leszczyński, michal.leszczynski@cert.pl
+ * Date:    June, 2020
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; under version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <err.h>
+#include <errno.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+
+#include <xenctrl.h>
+#include <xenforeignmemory.h>
+
+#define MSR_RTIT_CTL                        0x00000570
+#define  RTIT_CTL_OS                        (1 <<  2)
+#define  RTIT_CTL_USR                       (1 <<  3)
+#define  RTIT_CTL_BRANCH_EN                 (1 << 13)
+
+static volatile int interrupted = 0;
+
+static xc_interface *xch;
+static xenforeignmemory_handle *fh;
+
+void int_handler(int signum)
+{
+    interrupted = 1;
+}
+
+int main(int argc, char **argv)
+{
+    uint32_t domid, vcpu;
+    int rc, exit = 1;
+    size_t size;
+    char *buf = NULL;
+    xenforeignmemory_resource_handle *fres = NULL;
+    uint64_t last_offset = 0;
+
+    if ( signal(SIGINT, int_handler) == SIG_ERR )
+        err(1, "Failed to register signal handler\n");
+
+    if ( argc != 3 )
+    {
+        fprintf(stderr, "Usage: %s <domid> <vcpu_id>\n", argv[0]);
+        fprintf(stderr, "It's recommended to redirect thisprogram's output to file\n");
+        fprintf(stderr, "or to pipe it's output to xxd or other program.\n");
+        return 1;
+    }
+
+    domid = atoi(argv[1]);
+    vcpu  = atoi(argv[2]);
+
+    xch = xc_interface_open(NULL, NULL, 0);
+    fh = xenforeignmemory_open(NULL, 0);
+
+    if ( !xch )
+        err(1, "xc_interface_open()");
+    if ( !fh )
+        err(1, "xenforeignmemory_open()");
+
+    rc = xenforeignmemory_resource_size(
+        fh, domid, XENMEM_resource_vmtrace_buf, vcpu, &size);
+    if ( rc )
+        err(1, "xenforeignmemory_resource_size()");
+
+    fres = xenforeignmemory_map_resource(
+        fh, domid, XENMEM_resource_vmtrace_buf, vcpu,
+        0, size >> XC_PAGE_SHIFT, (void **)&buf, PROT_READ, 0);
+    if ( !fres )
+        err(1, "xenforeignmemory_map_resource()");
+
+    if ( xc_vmtrace_set_option(
+             xch, domid, vcpu, MSR_RTIT_CTL,
+             RTIT_CTL_BRANCH_EN | RTIT_CTL_USR | RTIT_CTL_OS) )
+    {
+        perror("xc_vmtrace_set_option()");
+        goto out;
+    }
+
+    if ( xc_vmtrace_enable(xch, domid, vcpu) )
+    {
+        perror("xc_vmtrace_enable()");
+        goto out;
+    }
+
+    while ( !interrupted )
+    {
+        xc_dominfo_t dominfo;
+        uint64_t offset;
+
+        if ( xc_vmtrace_output_position(xch, domid, vcpu, &offset) )
+        {
+            perror("xc_vmtrace_output_position()");
+            goto out;
+        }
+
+        if ( offset > last_offset )
+            fwrite(buf + last_offset, offset - last_offset, 1, stdout);
+        else if ( offset < last_offset )
+        {
+            /* buffer wrapped */
+            fwrite(buf + last_offset, size - last_offset, 1, stdout);
+            fwrite(buf, offset, 1, stdout);
+        }
+
+        last_offset = offset;
+        usleep(1000 * 100);
+
+        if ( xc_domain_getinfo(xch, domid, 1, &dominfo) != 1 ||
+             dominfo.domid != domid || dominfo.shutdown )
+            break;
+    }
+
+    exit = 0;
+
+ out:
+    if ( xc_vmtrace_disable(xch, domid, vcpu) )
+        perror("xc_vmtrace_disable()");
+
+    if ( fres && xenforeignmemory_unmap_resource(fh, fres) )
+        perror("xenforeignmemory_unmap_resource()");
+
+    return exit;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */