* Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain
@ 2020-01-24 19:58 Rich Persaud
2020-01-24 20:22 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 6+ messages in thread
From: Rich Persaud @ 2020-01-24 19:58 UTC (permalink / raw)
To: Jason Andryuk, Marek Marczykowski-Górecki
Cc: Wei Liu, Eric Chanudet, Ian Jackson, Christopher Clark,
Anthony PERARD, xen-devel
On Jan 24, 2020, at 09:07, Jason Andryuk <jandryuk@gmail.com> wrote:
>
> On Tue, Jan 21, 2020 at 6:46 PM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
>
>>>> +
>>>> + sdss->qmp_proxy_spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
>>>> + sdss->qmp_proxy_spawn.midproc_cb = libxl__spawn_record_pid;
>>>> + sdss->qmp_proxy_spawn.confirm_cb = qmp_proxy_confirm;
>>>> + sdss->qmp_proxy_spawn.failure_cb = qmp_proxy_startup_failed;
>>>> + sdss->qmp_proxy_spawn.detached_cb = qmp_proxy_detached;
>>>> +
>>>> + const int arraysize = 6;
>>>> + GCNEW_ARRAY(args, arraysize);
>>>> + args[nr++] = STUBDOM_QMP_PROXY_PATH;
>>>> + args[nr++] = GCSPRINTF("--state-path=%s", sdss->qmp_proxy_spawn.xspath);
>>>> + args[nr++] = GCSPRINTF("%u", dm_domid);
>>>> + args[nr++] = GCSPRINTF("%s/device-model/%u/qmp-vchan", dom_path, guest_domid);
>>> Thinking of OpenXT"s qmp-helper, this path isn't useful. But it is
>>> for vchan-socket-proxy, so qmp-helper could just change to ignore it.
>> For vchan we could use also a port number (and then it will encode it
>> into a xenstore path). This is in fact how we use libvchan in Qubes. I
>> opted for explicit path only because of lack of idea for some meaningful
>> port number ;) But I'm open for suggestions.
>> I guess that would be useful for Argo version then.
>
> The argo version hard codes the port number, so it's not a command
> line argument. The port number would need to get passed to the
> stubdom or it would need to be standardized.
>
> I think the arguments for vchan-socket-proxy make sense. Since it's
> the one that's submitted upstream, it makes sense to use them.
>
> Put another way, do we want this to support alternate implementations
> for a qmp proxy? Should the arguments be generic for that case?
One advantage of the server+client approach of vchan-socket-proxy is the absence of patches for Qemu. OpenXT qmp-helper requires a Qemu patch for Argo support. If there was a qmp socket proxy with Argo support, unpatched Qemu could be used with libxl and Argo access controls.
A generalized qmp-socket-proxy may be useful to other projects. It would be good if the design supported single-purpose (client or server) binaries, e.g. common functions in a library shared by separate client and server source files, with conditional compilation for vchan and Argo interfaces.
Rich
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain 2020-01-24 19:58 [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain Rich Persaud @ 2020-01-24 20:22 ` Marek Marczykowski-Górecki 0 siblings, 0 replies; 6+ messages in thread From: Marek Marczykowski-Górecki @ 2020-01-24 20:22 UTC (permalink / raw) To: Rich Persaud Cc: Wei Liu, Jason Andryuk, Eric Chanudet, Ian Jackson, Christopher Clark, Anthony PERARD, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 3896 bytes --] On Fri, Jan 24, 2020 at 02:58:56PM -0500, Rich Persaud wrote: > On Jan 24, 2020, at 09:07, Jason Andryuk <jandryuk@gmail.com> wrote: > > > > On Tue, Jan 21, 2020 at 6:46 PM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> wrote: > > > >>>> + > >>>> + sdss->qmp_proxy_spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000; > >>>> + sdss->qmp_proxy_spawn.midproc_cb = libxl__spawn_record_pid; > >>>> + sdss->qmp_proxy_spawn.confirm_cb = qmp_proxy_confirm; > >>>> + sdss->qmp_proxy_spawn.failure_cb = qmp_proxy_startup_failed; > >>>> + sdss->qmp_proxy_spawn.detached_cb = qmp_proxy_detached; > >>>> + > >>>> + const int arraysize = 6; > >>>> + GCNEW_ARRAY(args, arraysize); > >>>> + args[nr++] = STUBDOM_QMP_PROXY_PATH; > >>>> + args[nr++] = GCSPRINTF("--state-path=%s", sdss->qmp_proxy_spawn.xspath); > >>>> + args[nr++] = GCSPRINTF("%u", dm_domid); > >>>> + args[nr++] = GCSPRINTF("%s/device-model/%u/qmp-vchan", dom_path, guest_domid); > >>> Thinking of OpenXT"s qmp-helper, this path isn't useful. But it is > >>> for vchan-socket-proxy, so qmp-helper could just change to ignore it. > >> For vchan we could use also a port number (and then it will encode it > >> into a xenstore path). This is in fact how we use libvchan in Qubes. I > >> opted for explicit path only because of lack of idea for some meaningful > >> port number ;) But I'm open for suggestions. > >> I guess that would be useful for Argo version then. > > > > The argo version hard codes the port number, so it's not a command > > line argument. The port number would need to get passed to the > > stubdom or it would need to be standardized. > > > > I think the arguments for vchan-socket-proxy make sense. Since it's > > the one that's submitted upstream, it makes sense to use them. > > > > Put another way, do we want this to support alternate implementations > > for a qmp proxy? Should the arguments be generic for that case? > > > One advantage of the server+client approach of vchan-socket-proxy is the absence of patches for Qemu. OpenXT qmp-helper requires a Qemu patch for Argo support. If there was a qmp socket proxy with Argo support, unpatched Qemu could be used with libxl and Argo access controls. > > A generalized qmp-socket-proxy may be useful to other projects. It would be good if the design supported single-purpose (client or server) binaries, e.g. common functions in a library shared by separate client and server source files, with conditional compilation for vchan and Argo interfaces. I don't think it's worth separating client and server sources in the current shape. The whole file has less than 500 lines and majority of it is the common code. After connection setup, data processing is symmetric (the whole data_loop and its helper functions). What may be worth doing, is adding a place to plug qemu->libxl data filtering/sanitization. This data filtering should indeed be in a separate source file and only linked into server binary. I'm not yet sure what I'd like to filter data with. To be honest, I'm a bit uncomfortable with processing untrusted data in C... Does Argo have bindings for some other (memory safe) language? That would be a strong argument to use Argo here exclusively. Alternatively, I can think of delegating filtering to a separate process (pass data over stdin/stdout pipes). That could be very flexible, but could be also an overkill. Also: should such filter see data in both directions? I think yes, to have some context what libxl could expect (filter-out unexpected responses, responses not matching schema for a particular message type etc). -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Xen-devel] [PATCH v4 00/16] Add support for qemu-xen runnning in a Linux-based stubdomain. @ 2020-01-15 2:39 Marek Marczykowski-Górecki 2020-01-15 2:39 ` [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain Marek Marczykowski-Górecki 0 siblings, 1 reply; 6+ messages in thread From: Marek Marczykowski-Górecki @ 2020-01-15 2:39 UTC (permalink / raw) To: xen-devel Cc: Simon Gaiser, Wei Liu, Ian Jackson, Marek Marczykowski-Górecki, Eric Shelton General idea is to allow freely set device_model_version and device_model_stubdomain_override and choose the right options based on this choice. Also, allow to specific path to stubdomain kernel/ramdisk, for greater flexibility. First two patches add documentation about expected toolstack-stubdomain-qemu interface, both for MiniOS stubdomain and Linux stubdomain. Initial version has no QMP support - in initial patches it is completely disabled, which means no suspend/restore and no PCI passthrough. Later patches add QMP over libvchan connection support. The actual connection is made in a separate process. As discussed on Xen Summit 2019, this allows to apply some basic checks and/or filtering (not part of this series), to limit libxl exposure for potentially malicious stubdomain. The actual stubdomain implementation is here: https://github.com/marmarek/qubes-vmm-xen-stubdom-linux (branch for-upstream, tag for-upstream-v3) See readme there for build instructions. Beware: building on Debian is dangerous, as it require installing "dracut", which will remove initramfs-tools. You may end up with broken initrd on your host. Few comments/questions about the stubdomain code: 1. There are extra patches for qemu that are necessary to run it in stubdomain. While it is desirable to upstream them, I think it can be done after merging libxl part. Stubdomain's qemu build will in most cases be separate anyway, to limit qemu's dependencies (so the stubdomain size). 2. By default Linux hvc-xen console frontend is unreliable for data transfer (qemu state save/restore) - it drops data sent faster than client is reading it. To fix it, console device needs to be switched into raw mode (`stty raw /dev/hvc1`). Especially for restoring qemu state it is tricky, as it would need to be done before opening the device, but stty (obviously) needs to open the device first. To solve this problem, for now the repository contains kernel patch which changes the default for all hvc consoles. Again, this isn't practical problem, as the kernel for stubdomain is built separately. But it would be nice to have something working with vanilla kernel. I see those options: - convert it to kernel cmdline parameter (hvc_console_raw=1 ?) - use channels instead of consoles (and on the kernel side change the default to "raw" only for channels); while in theory better design, libxl part will be more complex, as channels can be connected to sockets but not files, so libxl would need to read/write to it exactly when qemu write/read the data, not before/after as it is done now Remaining parts for eliminating dom0's instance of qemu: - do not force QDISK backend for CDROM - multiple consoles support in xenconsoled Changes in v2: - apply review comments by Jason Andryuk Changes in v3: - rework qemu arguments handling (separate xenstore keys, instead of \x1b separator) - add QMP over libvchan, instead of console - add protocol documentation - a lot of minor changes, see individual patches for full changes list - split xenconsoled patches into separate series Changes in v4: - extract vchan connection into a separate process - rebase on master - various fixes Cc: Simon Gaiser <simon@invisiblethingslab.com> Cc: Eric Shelton <eshelton@pobox.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Eric Shelton (1): libxl: Handle Linux stubdomain specific QEMU options. Marek Marczykowski-Górecki (15): Document ioemu MiniOS stubdomain protocol Document ioemu Linux stubdomain protocol libxl: fix qemu-trad cmdline for no sdl/vnc case libxl: Allow running qemu-xen in stubdomain libxl: write qemu arguments into separate xenstore keys xl: add stubdomain related options to xl config parser tools/libvchan: notify server when client is connected libxl: add save/restore support for qemu-xen in stubdomain tools: add missing libxenvchan cflags tools: add simple vchan-socket-proxy libxl: use vchan for QMP access with Linux stubdomain Regenerate autotools files libxl: require qemu in dom0 even if stubdomain is in use libxl: ignore emulated IDE disks beyond the first 4 libxl: consider also qemu in stubdomain in libxl__dm_active check .gitignore | 1 +- configure | 14 +- docs/configure | 14 +- docs/man/xl.cfg.5.pod.in | 23 +- docs/misc/stubdom.txt | 103 ++++++- stubdom/configure | 14 +- tools/Rules.mk | 2 +- tools/config.h.in | 3 +- tools/configure | 46 +-- tools/configure.ac | 9 +- tools/libvchan/Makefile | 7 +- tools/libvchan/init.c | 3 +- tools/libvchan/init.c.rej | 60 ++++- tools/libvchan/vchan-socket-proxy.c | 469 +++++++++++++++++++++++++++++- tools/libxl/libxl_create.c | 37 +- tools/libxl/libxl_dm.c | 437 ++++++++++++++++++++++----- tools/libxl/libxl_internal.h | 19 +- tools/libxl/libxl_mem.c | 6 +- tools/libxl/libxl_qmp.c | 25 +- tools/libxl/libxl_types.idl | 3 +- tools/xl/xl_parse.c | 7 +- 21 files changed, 1151 insertions(+), 151 deletions(-) create mode 100644 tools/libvchan/init.c.rej create mode 100644 tools/libvchan/vchan-socket-proxy.c base-commit: fae249d23413b2bf7d98a97d8f649cf7d102c1ae -- git-series 0.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain 2020-01-15 2:39 [Xen-devel] [PATCH v4 00/16] Add support for qemu-xen runnning in a Linux-based stubdomain Marek Marczykowski-Górecki @ 2020-01-15 2:39 ` Marek Marczykowski-Górecki 2020-01-21 20:17 ` Jason Andryuk 0 siblings, 1 reply; 6+ messages in thread From: Marek Marczykowski-Górecki @ 2020-01-15 2:39 UTC (permalink / raw) To: xen-devel Cc: Anthony PERARD, Ian Jackson, Marek Marczykowski-Górecki, Wei Liu Access to QMP of QEMU in Linux stubdomain is possible over vchan connection. Handle the actual vchan connection in a separate process (vchan-socket-proxy). This simplified integration with QMP (already quite complex), but also allows preliminary filtering of (potentially malicious) QMP input. Since only one client can be connected to vchan server at the same time and it is not enforced by the libxenvchan itself, additional client-side locking is needed. It is implicitly implemented by vchan-socket-proxy, as it handle only one connection at a time. Note that qemu supports only one simultaneous client on a control socket anyway (but in UNIX socket case, it enforce it server-side), so it doesn't add any extra limitation. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v4: - new patch, in place of both "libxl: use vchan for QMP access ..." --- tools/configure.ac | 9 ++- tools/libxl/libxl_dm.c | 159 ++++++++++++++++++++++++++++++++++-- tools/libxl/libxl_internal.h | 1 +- 3 files changed, 161 insertions(+), 8 deletions(-) diff --git a/tools/configure.ac b/tools/configure.ac index 8d86c42..20bbdbf 100644 --- a/tools/configure.ac +++ b/tools/configure.ac @@ -192,6 +192,15 @@ AC_SUBST(qemu_xen) AC_SUBST(qemu_xen_path) AC_SUBST(qemu_xen_systemd) +AC_ARG_WITH([stubdom-qmp-proxy], + AC_HELP_STRING([--stubdom-qmp-proxy@<:@=PATH@:>@], + [Use supplied binary PATH as a QMP proxy into stubdomain]),[ + stubdom_qmp_proxy="$withval" +],[ + stubdom_qmp_proxy="$bindir/vchan-socket-proxy" +]) +AC_DEFINE_UNQUOTED([STUBDOM_QMP_PROXY_PATH], ["$stubdom_qmp_proxy"], [QMP proxy path]) + AC_ARG_WITH([system-seabios], AS_HELP_STRING([--with-system-seabios@<:@=PATH@:>@], [Use system supplied seabios PATH instead of building and installing diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 528ca3e..23ac7e4 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -1183,7 +1183,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, "-xen-domid", GCSPRINTF("%d", guest_domid), NULL); - /* There is currently no way to access the QMP socket in the stubdom */ + /* QMP access to qemu running in stubdomain is done over vchan, stubdomain setup it itself */ if (!is_stubdom) { flexarray_append(dm_args, "-chardev"); if (state->dm_monitor_fd >= 0) { @@ -2178,6 +2178,23 @@ static void stubdom_pvqemu_unpaused(libxl__egc *egc, static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait, int rc, const char *p); +static void spawn_qmp_proxy(libxl__egc *egc, + libxl__stub_dm_spawn_state *sdss); + +static void qmp_proxy_confirm(libxl__egc *egc, libxl__spawn_state *spawn, + const char *xsdata); + +static void qmp_proxy_startup_failed(libxl__egc *egc, + libxl__spawn_state *spawn, + int rc); + +static void qmp_proxy_detached(libxl__egc *egc, + libxl__spawn_state *spawn); + +static void qmp_proxy_spawn_outcome(libxl__egc *egc, + libxl__stub_dm_spawn_state *sdss, + int rc); + char *libxl__stub_dm_name(libxl__gc *gc, const char *guest_name) { return GCSPRINTF("%s-dm", guest_name); @@ -2460,24 +2477,150 @@ static void spawn_stub_launch_dm(libxl__egc *egc, goto out; } + sdss->qmp_proxy_spawn.ao = ao; + if (libxl__stubdomain_is_linux(&guest_config->b_info)) { + spawn_qmp_proxy(egc, sdss); + } else { + qmp_proxy_spawn_outcome(egc, sdss, 0); + } + + return; + +out: + assert(ret); + qmp_proxy_spawn_outcome(egc, sdss, ret); +} + +static void spawn_qmp_proxy(libxl__egc *egc, + libxl__stub_dm_spawn_state *sdss) +{ + STATE_AO_GC(sdss->qmp_proxy_spawn.ao); + const uint32_t guest_domid = sdss->dm.guest_domid; + const uint32_t dm_domid = sdss->pvqemu.guest_domid; + const char *dom_path = libxl__xs_get_dompath(gc, dm_domid); + char **args; + int nr = 0; + int rc, logfile_w, null; + + if (access(STUBDOM_QMP_PROXY_PATH, X_OK) < 0) { + LOGED(ERROR, guest_domid, "qmp proxy %s is not executable", STUBDOM_QMP_PROXY_PATH); + rc = ERROR_FAIL; + goto out; + } + + sdss->qmp_proxy_spawn.what = GCSPRINTF("domain %d device model qmp proxy", guest_domid); + sdss->qmp_proxy_spawn.pidpath = GCSPRINTF("%s/image/qmp-proxy-pid", dom_path); + sdss->qmp_proxy_spawn.xspath = GCSPRINTF("%s/image/qmp-proxy-state", dom_path); + + sdss->qmp_proxy_spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000; + sdss->qmp_proxy_spawn.midproc_cb = libxl__spawn_record_pid; + sdss->qmp_proxy_spawn.confirm_cb = qmp_proxy_confirm; + sdss->qmp_proxy_spawn.failure_cb = qmp_proxy_startup_failed; + sdss->qmp_proxy_spawn.detached_cb = qmp_proxy_detached; + + const int arraysize = 6; + GCNEW_ARRAY(args, arraysize); + args[nr++] = STUBDOM_QMP_PROXY_PATH; + args[nr++] = GCSPRINTF("--state-path=%s", sdss->qmp_proxy_spawn.xspath); + args[nr++] = GCSPRINTF("%u", dm_domid); + args[nr++] = GCSPRINTF("%s/device-model/%u/qmp-vchan", dom_path, guest_domid); + args[nr++] = (char*)libxl__qemu_qmp_path(gc, guest_domid); + args[nr++] = NULL; + assert(nr == arraysize); + + logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qmp-proxy-%s", + sdss->dm_config.c_info.name)); + if (logfile_w < 0) { + rc = logfile_w; + goto out; + } + null = open("/dev/null", O_RDWR); + if (null < 0) { + LOGED(ERROR, guest_domid, "unable to open /dev/null"); + rc = ERROR_FAIL; + goto out_close; + } + + rc = libxl__spawn_spawn(egc, &sdss->qmp_proxy_spawn); + if (rc < 0) + goto out_close; + if (!rc) { /* inner child */ + setsid(); + libxl__exec(gc, null, null, logfile_w, STUBDOM_QMP_PROXY_PATH, args, NULL); + /* unreachable */ + } + + rc = 0; + +out_close: + if (logfile_w >= 0) + close(logfile_w); + if (null >= 0) + close(null); +out: + if (rc) + qmp_proxy_spawn_outcome(egc, sdss, rc); +} + +static void qmp_proxy_confirm(libxl__egc *egc, libxl__spawn_state *spawn, + const char *xsdata) +{ + STATE_AO_GC(spawn->ao); + + if (!xsdata) + return; + + if (strcmp(xsdata, "running")) + return; + + libxl__spawn_initiate_detach(gc, spawn); +} + +static void qmp_proxy_startup_failed(libxl__egc *egc, + libxl__spawn_state *spawn, + int rc) +{ + libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(spawn, *sdss, qmp_proxy_spawn); + qmp_proxy_spawn_outcome(egc, sdss, rc); +} + +static void qmp_proxy_detached(libxl__egc *egc, + libxl__spawn_state *spawn) +{ + libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(spawn, *sdss, qmp_proxy_spawn); + qmp_proxy_spawn_outcome(egc, sdss, 0); +} + +static void qmp_proxy_spawn_outcome(libxl__egc *egc, + libxl__stub_dm_spawn_state *sdss, + int rc) +{ + STATE_AO_GC(sdss->qmp_proxy_spawn.ao); + int need_pvqemu = libxl__need_xenpv_qemu(gc, &sdss->dm_config); + + if (rc) goto out; + + if (need_pvqemu < 0) { + rc = need_pvqemu; + goto out; + } + sdss->pvqemu.spawn.ao = ao; - sdss->pvqemu.guest_domid = dm_domid; sdss->pvqemu.guest_config = &sdss->dm_config; sdss->pvqemu.build_state = &sdss->dm_state; sdss->pvqemu.callback = spawn_stubdom_pvqemu_cb; - - if (!need_qemu) { + if (need_pvqemu) { + libxl__spawn_local_dm(egc, &sdss->pvqemu); + } else { /* If dom0 qemu not needed, do not launch it */ spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, 0); - } else { - libxl__spawn_local_dm(egc, &sdss->pvqemu); } return; out: - assert(ret); - spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, ret); + assert(rc); + spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, rc); } static void spawn_stubdom_pvqemu_cb(libxl__egc *egc, diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 2b4a1cc..895bb65 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -4129,6 +4129,7 @@ typedef struct { libxl__destroy_domid_state dis; libxl__multidev multidev; libxl__xswait_state xswait; + libxl__spawn_state qmp_proxy_spawn; } libxl__stub_dm_spawn_state; _hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*); -- git-series 0.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain 2020-01-15 2:39 ` [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain Marek Marczykowski-Górecki @ 2020-01-21 20:17 ` Jason Andryuk 2020-01-21 23:46 ` Marek Marczykowski-Górecki 0 siblings, 1 reply; 6+ messages in thread From: Jason Andryuk @ 2020-01-21 20:17 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Anthony PERARD, xen-devel, Ian Jackson, Wei Liu On Tue, Jan 14, 2020 at 9:42 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > Access to QMP of QEMU in Linux stubdomain is possible over vchan > connection. Handle the actual vchan connection in a separate process > (vchan-socket-proxy). This simplified integration with QMP (already > quite complex), but also allows preliminary filtering of (potentially > malicious) QMP input. > Since only one client can be connected to vchan server at the same time > and it is not enforced by the libxenvchan itself, additional client-side > locking is needed. It is implicitly implemented by vchan-socket-proxy, > as it handle only one connection at a time. Note that qemu supports only > one simultaneous client on a control socket anyway (but in UNIX socket > case, it enforce it server-side), so it doesn't add any extra > limitation. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > Changes in v4: > - new patch, in place of both "libxl: use vchan for QMP access ..." > --- > tools/configure.ac | 9 ++- > tools/libxl/libxl_dm.c | 159 ++++++++++++++++++++++++++++++++++-- > tools/libxl/libxl_internal.h | 1 +- > 3 files changed, 161 insertions(+), 8 deletions(-) > > diff --git a/tools/configure.ac b/tools/configure.ac > index 8d86c42..20bbdbf 100644 > --- a/tools/configure.ac > +++ b/tools/configure.ac > @@ -192,6 +192,15 @@ AC_SUBST(qemu_xen) > AC_SUBST(qemu_xen_path) > AC_SUBST(qemu_xen_systemd) > > +AC_ARG_WITH([stubdom-qmp-proxy], > + AC_HELP_STRING([--stubdom-qmp-proxy@<:@=PATH@:>@], > + [Use supplied binary PATH as a QMP proxy into stubdomain]),[ Thanks for making it configurable :) > + stubdom_qmp_proxy="$withval" > +],[ > + stubdom_qmp_proxy="$bindir/vchan-socket-proxy" > +]) > +AC_DEFINE_UNQUOTED([STUBDOM_QMP_PROXY_PATH], ["$stubdom_qmp_proxy"], [QMP proxy path]) > + > AC_ARG_WITH([system-seabios], > AS_HELP_STRING([--with-system-seabios@<:@=PATH@:>@], > [Use system supplied seabios PATH instead of building and installing > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 528ca3e..23ac7e4 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -1183,7 +1183,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, > "-xen-domid", > GCSPRINTF("%d", guest_domid), NULL); > > - /* There is currently no way to access the QMP socket in the stubdom */ > + /* QMP access to qemu running in stubdomain is done over vchan, stubdomain setup it itself */ I think this would be clearer: /* QMP access to qemu running in stubdomain is done over vchan. The stubdomain init script * adds the appropriate monitor options for vchan-socket-proxy. */ In the block below, the -no-shutdown option is added to qemu, which will not be done for linux stubdomain. -no-shutdown Don't exit QEMU on guest shutdown, but instead only stop the emulation. This allows for instance switching to monitor to commit changes to the disk image. It's something I noticed, but I don't know if it matters to us. > if (!is_stubdom) { > flexarray_append(dm_args, "-chardev"); > if (state->dm_monitor_fd >= 0) { > @@ -2178,6 +2178,23 @@ static void stubdom_pvqemu_unpaused(libxl__egc *egc, <snip> > @@ -2460,24 +2477,150 @@ static void spawn_stub_launch_dm(libxl__egc *egc, > goto out; > } > > + sdss->qmp_proxy_spawn.ao = ao; > + if (libxl__stubdomain_is_linux(&guest_config->b_info)) { > + spawn_qmp_proxy(egc, sdss); > + } else { > + qmp_proxy_spawn_outcome(egc, sdss, 0); > + } > + > + return; > + > +out: > + assert(ret); > + qmp_proxy_spawn_outcome(egc, sdss, ret); > +} > + > +static void spawn_qmp_proxy(libxl__egc *egc, > + libxl__stub_dm_spawn_state *sdss) > +{ > + STATE_AO_GC(sdss->qmp_proxy_spawn.ao); > + const uint32_t guest_domid = sdss->dm.guest_domid; > + const uint32_t dm_domid = sdss->pvqemu.guest_domid; > + const char *dom_path = libxl__xs_get_dompath(gc, dm_domid); > + char **args; > + int nr = 0; > + int rc, logfile_w, null; > + > + if (access(STUBDOM_QMP_PROXY_PATH, X_OK) < 0) { > + LOGED(ERROR, guest_domid, "qmp proxy %s is not executable", STUBDOM_QMP_PROXY_PATH); > + rc = ERROR_FAIL; > + goto out; > + } > + > + sdss->qmp_proxy_spawn.what = GCSPRINTF("domain %d device model qmp proxy", guest_domid); > + sdss->qmp_proxy_spawn.pidpath = GCSPRINTF("%s/image/qmp-proxy-pid", dom_path); > + sdss->qmp_proxy_spawn.xspath = GCSPRINTF("%s/image/qmp-proxy-state", dom_path); Since this is the vchan-socket-proxy in dom0, should it write to "device-model/%u/qmp-proxy-state" underneath dom0? > + > + sdss->qmp_proxy_spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000; > + sdss->qmp_proxy_spawn.midproc_cb = libxl__spawn_record_pid; > + sdss->qmp_proxy_spawn.confirm_cb = qmp_proxy_confirm; > + sdss->qmp_proxy_spawn.failure_cb = qmp_proxy_startup_failed; > + sdss->qmp_proxy_spawn.detached_cb = qmp_proxy_detached; > + > + const int arraysize = 6; > + GCNEW_ARRAY(args, arraysize); > + args[nr++] = STUBDOM_QMP_PROXY_PATH; > + args[nr++] = GCSPRINTF("--state-path=%s", sdss->qmp_proxy_spawn.xspath); > + args[nr++] = GCSPRINTF("%u", dm_domid); > + args[nr++] = GCSPRINTF("%s/device-model/%u/qmp-vchan", dom_path, guest_domid); Thinking of OpenXT"s qmp-helper, this path isn't useful. But it is for vchan-socket-proxy, so qmp-helper could just change to ignore it. > + args[nr++] = (char*)libxl__qemu_qmp_path(gc, guest_domid); qmp-helper takes just the stub_domid and domid. The domid is just used to generate the above path, but taking the path would be cleaner. > + args[nr++] = NULL; > + assert(nr == arraysize); This generally looks good. Regards, Jason _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain 2020-01-21 20:17 ` Jason Andryuk @ 2020-01-21 23:46 ` Marek Marczykowski-Górecki 2020-01-24 14:05 ` Jason Andryuk 0 siblings, 1 reply; 6+ messages in thread From: Marek Marczykowski-Górecki @ 2020-01-21 23:46 UTC (permalink / raw) To: Jason Andryuk; +Cc: Anthony PERARD, xen-devel, Ian Jackson, Wei Liu [-- Attachment #1.1: Type: text/plain, Size: 7145 bytes --] On Tue, Jan 21, 2020 at 03:17:39PM -0500, Jason Andryuk wrote: > On Tue, Jan 14, 2020 at 9:42 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: > > > > Access to QMP of QEMU in Linux stubdomain is possible over vchan > > connection. Handle the actual vchan connection in a separate process > > (vchan-socket-proxy). This simplified integration with QMP (already > > quite complex), but also allows preliminary filtering of (potentially > > malicious) QMP input. > > Since only one client can be connected to vchan server at the same time > > and it is not enforced by the libxenvchan itself, additional client-side > > locking is needed. It is implicitly implemented by vchan-socket-proxy, > > as it handle only one connection at a time. Note that qemu supports only > > one simultaneous client on a control socket anyway (but in UNIX socket > > case, it enforce it server-side), so it doesn't add any extra > > limitation. > > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > --- > > Changes in v4: > > - new patch, in place of both "libxl: use vchan for QMP access ..." > > --- > > tools/configure.ac | 9 ++- > > tools/libxl/libxl_dm.c | 159 ++++++++++++++++++++++++++++++++++-- > > tools/libxl/libxl_internal.h | 1 +- > > 3 files changed, 161 insertions(+), 8 deletions(-) > > > > diff --git a/tools/configure.ac b/tools/configure.ac > > index 8d86c42..20bbdbf 100644 > > --- a/tools/configure.ac > > +++ b/tools/configure.ac > > @@ -192,6 +192,15 @@ AC_SUBST(qemu_xen) > > AC_SUBST(qemu_xen_path) > > AC_SUBST(qemu_xen_systemd) > > > > +AC_ARG_WITH([stubdom-qmp-proxy], > > + AC_HELP_STRING([--stubdom-qmp-proxy@<:@=PATH@:>@], > > + [Use supplied binary PATH as a QMP proxy into stubdomain]),[ > > Thanks for making it configurable :) > > > + stubdom_qmp_proxy="$withval" > > +],[ > > + stubdom_qmp_proxy="$bindir/vchan-socket-proxy" > > +]) > > +AC_DEFINE_UNQUOTED([STUBDOM_QMP_PROXY_PATH], ["$stubdom_qmp_proxy"], [QMP proxy path]) > > + > > AC_ARG_WITH([system-seabios], > > AS_HELP_STRING([--with-system-seabios@<:@=PATH@:>@], > > [Use system supplied seabios PATH instead of building and installing > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > index 528ca3e..23ac7e4 100644 > > --- a/tools/libxl/libxl_dm.c > > +++ b/tools/libxl/libxl_dm.c > > @@ -1183,7 +1183,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, > > "-xen-domid", > > GCSPRINTF("%d", guest_domid), NULL); > > > > - /* There is currently no way to access the QMP socket in the stubdom */ > > + /* QMP access to qemu running in stubdomain is done over vchan, stubdomain setup it itself */ > > I think this would be clearer: > /* QMP access to qemu running in stubdomain is done over vchan. The > stubdomain init script > * adds the appropriate monitor options for vchan-socket-proxy. */ Yes, clearer. > In the block below, the -no-shutdown option is added to qemu, which > will not be done for linux stubdomain. > -no-shutdown > Don't exit QEMU on guest shutdown, but instead only stop the > emulation. This allows for instance switching to monitor to commit > changes to the disk image. > > It's something I noticed, but I don't know if it matters to us. I'll move it outside, looks like unrelated change. > > if (!is_stubdom) { > > flexarray_append(dm_args, "-chardev"); > > if (state->dm_monitor_fd >= 0) { > > @@ -2178,6 +2178,23 @@ static void stubdom_pvqemu_unpaused(libxl__egc *egc, > > <snip> > > > @@ -2460,24 +2477,150 @@ static void spawn_stub_launch_dm(libxl__egc *egc, > > goto out; > > } > > > > + sdss->qmp_proxy_spawn.ao = ao; > > + if (libxl__stubdomain_is_linux(&guest_config->b_info)) { > > + spawn_qmp_proxy(egc, sdss); > > + } else { > > + qmp_proxy_spawn_outcome(egc, sdss, 0); > > + } > > + > > + return; > > + > > +out: > > + assert(ret); > > + qmp_proxy_spawn_outcome(egc, sdss, ret); > > +} > > + > > +static void spawn_qmp_proxy(libxl__egc *egc, > > + libxl__stub_dm_spawn_state *sdss) > > +{ > > + STATE_AO_GC(sdss->qmp_proxy_spawn.ao); > > + const uint32_t guest_domid = sdss->dm.guest_domid; > > + const uint32_t dm_domid = sdss->pvqemu.guest_domid; > > + const char *dom_path = libxl__xs_get_dompath(gc, dm_domid); > > + char **args; > > + int nr = 0; > > + int rc, logfile_w, null; > > + > > + if (access(STUBDOM_QMP_PROXY_PATH, X_OK) < 0) { > > + LOGED(ERROR, guest_domid, "qmp proxy %s is not executable", STUBDOM_QMP_PROXY_PATH); > > + rc = ERROR_FAIL; > > + goto out; > > + } > > + > > + sdss->qmp_proxy_spawn.what = GCSPRINTF("domain %d device model qmp proxy", guest_domid); > > + sdss->qmp_proxy_spawn.pidpath = GCSPRINTF("%s/image/qmp-proxy-pid", dom_path); > > + sdss->qmp_proxy_spawn.xspath = GCSPRINTF("%s/image/qmp-proxy-state", dom_path); > > Since this is the vchan-socket-proxy in dom0, should it write to > "device-model/%u/qmp-proxy-state" underneath dom0? Yes, that would be more consistent. But pid should stay where it is (it's also where dom0 qemu pid is being written). > > + > > + sdss->qmp_proxy_spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000; > > + sdss->qmp_proxy_spawn.midproc_cb = libxl__spawn_record_pid; > > + sdss->qmp_proxy_spawn.confirm_cb = qmp_proxy_confirm; > > + sdss->qmp_proxy_spawn.failure_cb = qmp_proxy_startup_failed; > > + sdss->qmp_proxy_spawn.detached_cb = qmp_proxy_detached; > > + > > + const int arraysize = 6; > > + GCNEW_ARRAY(args, arraysize); > > + args[nr++] = STUBDOM_QMP_PROXY_PATH; > > + args[nr++] = GCSPRINTF("--state-path=%s", sdss->qmp_proxy_spawn.xspath); > > + args[nr++] = GCSPRINTF("%u", dm_domid); > > + args[nr++] = GCSPRINTF("%s/device-model/%u/qmp-vchan", dom_path, guest_domid); > > Thinking of OpenXT"s qmp-helper, this path isn't useful. But it is > for vchan-socket-proxy, so qmp-helper could just change to ignore it. For vchan we could use also a port number (and then it will encode it into a xenstore path). This is in fact how we use libvchan in Qubes. I opted for explicit path only because of lack of idea for some meaningful port number ;) But I'm open for suggestions. I guess that would be useful for Argo version then. > > + args[nr++] = (char*)libxl__qemu_qmp_path(gc, guest_domid); > > qmp-helper takes just the stub_domid and domid. The domid is just > used to generate the above path, but taking the path would be cleaner. > > > + args[nr++] = NULL; > > + assert(nr == arraysize); > > This generally looks good. > > Regards, > Jason -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain 2020-01-21 23:46 ` Marek Marczykowski-Górecki @ 2020-01-24 14:05 ` Jason Andryuk 0 siblings, 0 replies; 6+ messages in thread From: Jason Andryuk @ 2020-01-24 14:05 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Anthony PERARD, xen-devel, Ian Jackson, Wei Liu On Tue, Jan 21, 2020 at 6:46 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: <snip> > > > +static void spawn_qmp_proxy(libxl__egc *egc, > > > + libxl__stub_dm_spawn_state *sdss) > > > +{ > > > + STATE_AO_GC(sdss->qmp_proxy_spawn.ao); > > > + const uint32_t guest_domid = sdss->dm.guest_domid; > > > + const uint32_t dm_domid = sdss->pvqemu.guest_domid; > > > + const char *dom_path = libxl__xs_get_dompath(gc, dm_domid); > > > + char **args; > > > + int nr = 0; > > > + int rc, logfile_w, null; > > > + > > > + if (access(STUBDOM_QMP_PROXY_PATH, X_OK) < 0) { > > > + LOGED(ERROR, guest_domid, "qmp proxy %s is not executable", STUBDOM_QMP_PROXY_PATH); > > > + rc = ERROR_FAIL; > > > + goto out; > > > + } > > > + > > > + sdss->qmp_proxy_spawn.what = GCSPRINTF("domain %d device model qmp proxy", guest_domid); > > > + sdss->qmp_proxy_spawn.pidpath = GCSPRINTF("%s/image/qmp-proxy-pid", dom_path); > > > + sdss->qmp_proxy_spawn.xspath = GCSPRINTF("%s/image/qmp-proxy-state", dom_path); > > > > Since this is the vchan-socket-proxy in dom0, should it write to > > "device-model/%u/qmp-proxy-state" underneath dom0? > > Yes, that would be more consistent. But pid should stay where it is > (it's also where dom0 qemu pid is being written). Hmm, that split between pids and device-model info is a little weird. But it is specified in docs misc/xenstore-paths.pandoc > > > + > > > + sdss->qmp_proxy_spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000; > > > + sdss->qmp_proxy_spawn.midproc_cb = libxl__spawn_record_pid; > > > + sdss->qmp_proxy_spawn.confirm_cb = qmp_proxy_confirm; > > > + sdss->qmp_proxy_spawn.failure_cb = qmp_proxy_startup_failed; > > > + sdss->qmp_proxy_spawn.detached_cb = qmp_proxy_detached; > > > + > > > + const int arraysize = 6; > > > + GCNEW_ARRAY(args, arraysize); > > > + args[nr++] = STUBDOM_QMP_PROXY_PATH; > > > + args[nr++] = GCSPRINTF("--state-path=%s", sdss->qmp_proxy_spawn.xspath); > > > + args[nr++] = GCSPRINTF("%u", dm_domid); > > > + args[nr++] = GCSPRINTF("%s/device-model/%u/qmp-vchan", dom_path, guest_domid); > > > > Thinking of OpenXT"s qmp-helper, this path isn't useful. But it is > > for vchan-socket-proxy, so qmp-helper could just change to ignore it. > > For vchan we could use also a port number (and then it will encode it > into a xenstore path). This is in fact how we use libvchan in Qubes. I > opted for explicit path only because of lack of idea for some meaningful > port number ;) But I'm open for suggestions. > I guess that would be useful for Argo version then. The argo version hard codes the port number, so it's not a command line argument. The port number would need to get passed to the stubdom or it would need to be standardized. I think the arguments for vchan-socket-proxy make sense. Since it's the one that's submitted upstream, it makes sense to use them. Put another way, do we want this to support alternate implementations for a qmp proxy? Should the arguments be generic for that case? Since the existing arguments have enough information for either proxy, I think it's fine to leave it as is. While you could have a wrapper generate all the above from just the domid & stub_domid, that's kinda hacky. Thanks, Jason _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-24 20:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-24 19:58 [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain Rich Persaud 2020-01-24 20:22 ` Marek Marczykowski-Górecki -- strict thread matches above, loose matches on Subject: below -- 2020-01-15 2:39 [Xen-devel] [PATCH v4 00/16] Add support for qemu-xen runnning in a Linux-based stubdomain Marek Marczykowski-Górecki 2020-01-15 2:39 ` [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain Marek Marczykowski-Górecki 2020-01-21 20:17 ` Jason Andryuk 2020-01-21 23:46 ` Marek Marczykowski-Górecki 2020-01-24 14:05 ` Jason Andryuk
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).