Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain

Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain

[Rich Persaud]

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

Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain

[Marek Marczykowski-Górecki]

[-- 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

Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain

[Jason Andryuk]

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

Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain

[Marek Marczykowski-Górecki]

[-- 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

Re: [Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain

[Jason Andryuk]

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

[Xen-devel] [PATCH v4 12/16] libxl: use vchan for QMP access with Linux stubdomain

[Marek Marczykowski-Górecki]

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