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

["Lluís Vilanova" <vilanova@ac.upc.edu>]

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)

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?

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*).


Cheers,
  Lluis