Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4239 bytes --]

On Wed, Jul 26, 2017 at 03:44:39PM +0300, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
> 
> > On Tue, Jul 25, 2017 at 06:11:43PM +0300, Lluís Vilanova wrote:
> >> Peter Maydell writes:
> >> 
> >> > On 25 July 2017 at 14:19, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> >> Instead I suggest adding a trace backend generates calls to registered
> >> >> "callback" functions:
> >> >> 
> >> >> $ cat >my-instrumentation.c
> >> >> #include "trace/control.h"
> >> >> 
> >> >> static void my_cpu_in(unsigned int addr, char size, unsigned int val)
> >> >> {
> >> >> printf("my_cpu_in\n");
> >> >> }
> >> >> 
> >> >> static void my_init(void)
> >> >> {
> >> >> trace_register_event_callback("cpu_in", my_cpu_in);
> >> >> trace_enable_events("cpu_in");
> >> >> }
> >> >> trace_init(my_init);
> >> >> 
> >> >> $ ./configure --enable-trace-backends=log,callback && make -j4
> >> >> 
> >> >> This is still a clean interface that allows instrumentation code to be
> >> >> kept separate from the trace event call sites.
> >> >> 
> >> >> The instrumentation code gets compiled into QEMU, but that shouldn't be
> >> >> a huge burden since QEMU's Makefiles only recompile changed source
> >> >> files (only the first build is slow).
> >> 
> >> > Is your proposal that my-instrumentation.c gets compiled into
> >> > QEMU at this point? That doesn't seem like a great idea to
> >> > me, because it means you can only use this tracing if you
> >> > build QEMU yourself, and distros won't enable it and so
> >> > lots of our users won't have convenient access to it.
> >> > I'd much rather see a cleanly designed plugin interface
> >> > (which we should be able to implement in a manner that doesn't
> >> > let the plugin monkey patch arbitrary parts of QEMU beyond
> >> > what can already be achieved via LD_PRELOAD).
> >> 
> >> Just to be clear, what do you both mean by monkey-patching?
> >> 
> >> * Accessing unintended symbols in QEMU from the library (and from there
> >> modifying QEMU's behavior).
> >> * QEMU using symbols on the library instead of its own just because they have
> >> the same name.
> >> 
> >> As I said, the former can be accomplished by compiling QEMU with
> >> "-fvisibility=hidden".
> >> 
> >> The latter is already achieved by using dlopen with RTLD_LOCAL (the default).
> 
> > Instrumentation functions are invoked in the same memory space as QEMU,
> > so pointer arguments can be modified.
> 
> > Think of all the void *s arguments we trace.  Instrumentation code can
> > access those structs, hijack function pointers, etc.  No symbols are
> > required.
> 
> And why exactly is this a threat? Because it can be used to "extend" QEMU
> without touching its sources? Is this a realistic threat? (it's a rather brittle
> way to do it, so I'm not sure it's practical)

Unfortunately it is a problem.  I recently came across a product that
was using LD_PRELOAD= to "integrate" with QEMU.  People really abuse
these interfaces instead of integrating their features cleanly into
QEMU.

I see the use cases that Peter has been describing and am sure we can
come up with good solutions.  What I care about is that it doesn't allow
loading a .so that connects to arbitrary trace events.

> If that's the case, forcing instrumentation libraries to be exclusively GPL
> seems like a solution to me, just as GPL protects QEMU itself.
> 
> Do we agree on that? Or did I miss something else?

How the license applies depends on how user instrumentation code
interfaces with QEMU.  We probably need to ask the QEMU project's
lawyers for confirmation.  I don't think it's as simple as saying "the
interface will be protected by GPL".

Let's discuss the use cases and how instrumentation should work in
another subthread first.  Then towards the end we can come back to GPL
again.

> As a side note, I find instrumentation to be most useful for guest code events,
> which mostly contain non-pointer values (except for the CPUState*).

That's great.  I'll read the entire series to get a feel for what
instrumentation code needs to be able to do.  I'm wondering if a stable
public API is possible that does not leave room for abuse.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]