diff mbox series

[05/16] xen: defer call to xen_restrict until just before os_setup_post

Message ID 1524156319-11465-6-git-send-email-ian.jackson@eu.citrix.com
State New
Headers show
Series xen: xen-domid-restrict improvements | expand

Commit Message

Ian Jackson April 19, 2018, 4:45 p.m. UTC 
We need to restrict *all* the control fds that qemu opens.  Looking in
/proc/PID/fd shows there are many; their allocation seems scattered
throughout Xen support code in qemu.

We must postpone the restrict call until roughly the same time as qemu
changes its uid, chroots (if applicable), and so on.

There doesn't seem to be an appropriate hook already.  The RunState
change hook fires at different times depending on exactly what mode
qemu is operating in.

And it appears that no-one but the Xen code wants a hook at this phase
of execution.  So, introduce a bare call to a new function
xen_setup_post, just before os_setup_post.  Also provide the
appropriate stub for when Xen compilation is disabled.

We do the restriction before rather than after os_setup_post, because
xen_restrict may need to open /dev/null, and os_setup_post might have
called chroot.

Currently this does not work with migration, because when running as
the Xen device model qemu needs to signal to the toolstack that it is
ready.  It currently does this using xenstore, and for incoming
migration (but not for ordinary startup) that happens after
os_setup_post.

It is correct that this happens late: we want the incoming migration
stream to be processed by a restricted qemu.  The fix for this will be
to do the startup notification a different way, without using
xenstore.  (QMP is probably a reasonable choice.)

So for now this restriction feature cannot be used in conjunction with
migration.  (Note that this is not a regression in this patch, because
previously the -xen-restrict-domid call was, in fact, simply
ineffective!)  We will revisit this in the Xen 4.11 release cycle.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
CC: Richard Henderson <rth@twiddle.net> (maintainer:X86)
CC: Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
CC: Michael S. Tsirkin <mst@redhat.com> (supporter:PC)
---
v7: Use new AccelClass setup_post hook, rather than ad-hoc call
    in vl.c.
v5: Discuss problems with migration startup notification
    in the commit message.
v3: Do xen_setup_post just before, not just after, os_setup_post,
    to improve interaction with chroot.  Thanks to Ross Lagerwall.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/i386/xen/xen-hvm.c |  8 --------
 hw/xen/xen-common.c   | 14 ++++++++++++++
 stubs/xen-hvm.c       |  1 +
 3 files changed, 15 insertions(+), 8 deletions(-)

Comments

Anthony PERARD April 23, 2018, 2:28 p.m. UTC | #1 
On Thu, Apr 19, 2018 at 05:45:08PM +0100, Ian Jackson wrote:
> We need to restrict *all* the control fds that qemu opens.  Looking in
> /proc/PID/fd shows there are many; their allocation seems scattered
> throughout Xen support code in qemu.
> 
> We must postpone the restrict call until roughly the same time as qemu
> changes its uid, chroots (if applicable), and so on.
> 
> There doesn't seem to be an appropriate hook already.  The RunState
> change hook fires at different times depending on exactly what mode
> qemu is operating in.
> 
> And it appears that no-one but the Xen code wants a hook at this phase
> of execution.  So, introduce a bare call to a new function
> xen_setup_post, just before os_setup_post.  Also provide the
> appropriate stub for when Xen compilation is disabled.
> 
> We do the restriction before rather than after os_setup_post, because
> xen_restrict may need to open /dev/null, and os_setup_post might have
> called chroot.
> 
> Currently this does not work with migration, because when running as
> the Xen device model qemu needs to signal to the toolstack that it is
> ready.  It currently does this using xenstore, and for incoming
> migration (but not for ordinary startup) that happens after
> os_setup_post.
> 
> It is correct that this happens late: we want the incoming migration
> stream to be processed by a restricted qemu.  The fix for this will be
> to do the startup notification a different way, without using
> xenstore.  (QMP is probably a reasonable choice.)
> 
> So for now this restriction feature cannot be used in conjunction with
> migration.  (Note that this is not a regression in this patch, because
> previously the -xen-restrict-domid call was, in fact, simply
> ineffective!)  We will revisit this in the Xen 4.11 release cycle.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Anthony PERARD April 23, 2018, 3:58 p.m. UTC | #2 
On Thu, Apr 19, 2018 at 05:45:08PM +0100, Ian Jackson wrote:
> diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
> index 0067bcc..7787ea2 100644
> --- a/stubs/xen-hvm.c
> +++ b/stubs/xen-hvm.c
> @@ -13,6 +13,7 @@
>  #include "hw/xen/xen.h"
>  #include "exec/memory.h"
>  #include "qapi/qapi-commands-misc.h"
> +#include "sysemu/sysemu.h"

I think this include is not needed anymore, and can go away from the
patch series.

>  
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
>  {
Ian Jackson April 24, 2018, 3:08 p.m. UTC | #3 
Anthony PERARD writes ("Re: [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post"):
> I think this include is not needed anymore, and can go away from the
> patch series.

Yes.

Thanks,
Ian.
diff mbox series

Patch

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index f24b7d4..9c3b6b3 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1254,14 +1254,6 @@  void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
         goto err;
     }
 
-    if (xen_domid_restrict) {
-        rc = xen_restrict(xen_domid);
-        if (rc < 0) {
-            error_report("failed to restrict: error %d", errno);
-            goto err;
-        }
-    }
-
     xen_create_ioreq_server(xen_domid, &state->ioservid);
 
     state->exit.notify = xen_exit_notifier;
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 83099dd..454777c 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -117,6 +117,19 @@  static void xen_change_state_handler(void *opaque, int running,
     }
 }
 
+static void xen_setup_post(MachineState *ms, AccelState *accel)
+{
+    int rc;
+
+    if (xen_domid_restrict) {
+        rc = xen_restrict(xen_domid);
+        if (rc < 0) {
+            perror("xen: failed to restrict");
+            exit(1);
+        }
+    }
+}
+
 static int xen_init(MachineState *ms)
 {
     xen_xc = xc_interface_open(0, 0, 0);
@@ -165,6 +178,7 @@  static void xen_accel_class_init(ObjectClass *oc, void *data)
     AccelClass *ac = ACCEL_CLASS(oc);
     ac->name = "Xen";
     ac->init_machine = xen_init;
+    ac->setup_post = xen_setup_post;
     ac->allowed = &xen_allowed;
     ac->global_props = xen_compat_props;
 }
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 0067bcc..7787ea2 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -13,6 +13,7 @@ 
 #include "hw/xen/xen.h"
 #include "exec/memory.h"
 #include "qapi/qapi-commands-misc.h"
+#include "sysemu/sysemu.h"
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
 {