[v3,4/6] libxl: allow creation of domains with a specified or random domid
diff mbox series

Message ID 20200116093602.4203-5-pdurrant@amazon.com
State Superseded
Headers show
Series
  • xl/libxl: domid allocation/preservation changes
Related show

Commit Message

Paul Durrant Jan. 16, 2020, 9:36 a.m. UTC
This patch adds a 'domid' field to libxl_domain_create_info and then
modifies do_domain_create() to use that value if it is valid. Any valid
domid will be checked against the retired domid list before being passed
to libxl__domain_make().
If the domid value is invalid then Xen will choose the domid, as before,
unless the value is the new special RANDOM_DOMID value added to the API.
This value instructs libxl__domain_make() to select a random domid value,
check it for validity, verify it does not match a retired domain, and then
pass it to Xen's XEN_DOMCTL_createdomain operation. If Xen determines that
it co-incides with an existing domain, a new random value will be
selected and the operation will be re-tried.

NOTE: libxl__logv() is also modified to only log valid domid values in
      messages rather than any domid, valid or otherwise, that is not
      INVALID_DOMID.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: jandryuk@gmail.com

v3:
 - Added DOMID_MASK definition used to mask randomized values
 - Use stack variable to avoid assuming endianness

v2:
 - Re-worked to use a value from libxl_domain_create_info
---
 tools/libxl/libxl.h          |  9 +++++++++
 tools/libxl/libxl_create.c   | 36 +++++++++++++++++++++++++++++++++++-
 tools/libxl/libxl_internal.c |  2 +-
 tools/libxl/libxl_types.idl  |  1 +
 xen/include/public/xen.h     |  3 +++
 5 files changed, 49 insertions(+), 2 deletions(-)

Comments

Jan Beulich Jan. 16, 2020, 9:40 a.m. UTC | #1
On 16.01.2020 10:36, Paul Durrant wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>  /* Idle domain. */
>  #define DOMID_IDLE           xen_mk_uint(0x7FFF)
>  
> +/* Mask for valid domain id values */
> +#define DOMID_MASK           0x7FFF

Seeing it used in context, any reason not to use xen_mk_uint()
here as well?

Jan
Durrant, Paul Jan. 16, 2020, 9:46 a.m. UTC | #2
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 16 January 2020 10:40
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Ian Jackson
> <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; Anthony PERARD
> <anthony.perard@citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> George Dunlap <George.Dunlap@eu.citrix.com>; Julien Grall
> <julien@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano
> Stabellini <sstabellini@kernel.org>; jandryuk@gmail.com
> Subject: Re: [PATCH v3 4/6] libxl: allow creation of domains with a
> specified or random domid
> 
> On 16.01.2020 10:36, Paul Durrant wrote:
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> >  /* Idle domain. */
> >  #define DOMID_IDLE           xen_mk_uint(0x7FFF)
> >
> > +/* Mask for valid domain id values */
> > +#define DOMID_MASK           0x7FFF
> 
> Seeing it used in context, any reason not to use xen_mk_uint()
> here as well?
> 

I did wonder but I thought it best not to impose a type on a mask.

  Paul
Jan Beulich Jan. 16, 2020, 9:59 a.m. UTC | #3
On 16.01.2020 10:46, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 16 January 2020 10:40
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: xen-devel@lists.xenproject.org; Ian Jackson
>> <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; Anthony PERARD
>> <anthony.perard@citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
>> George Dunlap <George.Dunlap@eu.citrix.com>; Julien Grall
>> <julien@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; jandryuk@gmail.com
>> Subject: Re: [PATCH v3 4/6] libxl: allow creation of domains with a
>> specified or random domid
>>
>> On 16.01.2020 10:36, Paul Durrant wrote:
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>>>  /* Idle domain. */
>>>  #define DOMID_IDLE           xen_mk_uint(0x7FFF)
>>>
>>> +/* Mask for valid domain id values */
>>> +#define DOMID_MASK           0x7FFF
>>
>> Seeing it used in context, any reason not to use xen_mk_uint()
>> here as well?
>>
> 
> I did wonder but I thought it best not to impose a type on a mask.

I'd be happy to see the other DOMID_* uses dropped (I don't see
whey they had a U suffix originally, which was then converted
to xen_mk_uint()), but I'd prefer the entire set to be consistent.
I can see in general why a mask might better not be explicitly
(or implicitly) unsigned, but here I don't really see plausible
uses of ~ on the mask.

Jan
Jason Andryuk Jan. 16, 2020, 3:53 p.m. UTC | #4
On Thu, Jan 16, 2020 at 4:36 AM Paul Durrant <pdurrant@amazon.com> wrote:
>
> This patch adds a 'domid' field to libxl_domain_create_info and then
> modifies do_domain_create() to use that value if it is valid. Any valid
> domid will be checked against the retired domid list before being passed
> to libxl__domain_make().
> If the domid value is invalid then Xen will choose the domid, as before,
> unless the value is the new special RANDOM_DOMID value added to the API.
> This value instructs libxl__domain_make() to select a random domid value,
> check it for validity, verify it does not match a retired domain, and then
> pass it to Xen's XEN_DOMCTL_createdomain operation. If Xen determines that
> it co-incides with an existing domain, a new random value will be
> selected and the operation will be re-tried.
>
> NOTE: libxl__logv() is also modified to only log valid domid values in
>       messages rather than any domid, valid or otherwise, that is not
>       INVALID_DOMID.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Looks good.
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Ian Jackson Jan. 16, 2020, 6:36 p.m. UTC | #5
Hi.  This broadly contains what I expected, but:

Paul Durrant writes ("[PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid"):

> +        for (;;) {
> +            if (info->domid == RANDOM_DOMID) {
> +                uint16_t v;
> +
> +                /* Randomize lower order bytes */
> +                ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
> +                if (ret < 0)
> +                    break;
> +
> +                v &= DOMID_MASK;
> +                if (!libxl_domid_valid_guest(v) ||
> +                    libxl__is_retired_domid(gc, v))
> +                    continue;
> +
> +                *domid = v;
> +            }
> +
> +            ret = xc_domain_create(ctx->xch, domid, &create);
> +            if (ret == 0 || errno != EEXIST || info->domid != RANDOM_DOMID)
> +                break;
> +        }

I think this has a race.

  Thread A, in domain destroy           Thread B, in code above

                                         choose domid V
                                         check V in recent domid list

     add V to recent domid list
     destroy domain V in Xen

                                         create domain V in Xen
                                         continue constructing V

Thread B improperly constructs a new guest using V, exposing anyone
who was talking about V a moment ago to bugs.  Some code might even
fail to spot the interval where V does not exist and carry on talking
to the new V as if it were the old one...

I think there are only two possible solutions:

  - Check the domain's entry in the recent list *after* creating
    the domain in Xen.  This involves accepting that we will
    reuse the domid but only for a domain we are in the early
    stages of constructing, so hopefully without bad consequence?

  - Take the recent domid lock.

Also, it seems to me that we should check the recent domid list if we
let Xen choose the domid.  Maybe that can be in a subsequent patch...

Thanks,
Ian.
Durrant, Paul Jan. 17, 2020, 9:37 a.m. UTC | #6
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 16 January 2020 19:36
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> jandryuk@gmail.com
> Subject: Re: [PATCH v3 4/6] libxl: allow creation of domains with a
> specified or random domid
> 
> Hi.  This broadly contains what I expected, but:
> 
> Paul Durrant writes ("[PATCH v3 4/6] libxl: allow creation of domains with
> a specified or random domid"):
> 
> > +        for (;;) {
> > +            if (info->domid == RANDOM_DOMID) {
> > +                uint16_t v;
> > +
> > +                /* Randomize lower order bytes */
> > +                ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
> > +                if (ret < 0)
> > +                    break;
> > +
> > +                v &= DOMID_MASK;
> > +                if (!libxl_domid_valid_guest(v) ||
> > +                    libxl__is_retired_domid(gc, v))
> > +                    continue;
> > +
> > +                *domid = v;
> > +            }
> > +
> > +            ret = xc_domain_create(ctx->xch, domid, &create);
> > +            if (ret == 0 || errno != EEXIST || info->domid !=
> RANDOM_DOMID)
> > +                break;
> > +        }
> 
> I think this has a race.
> 
>   Thread A, in domain destroy           Thread B, in code above
> 
>                                          choose domid V
>                                          check V in recent domid list
> 
>      add V to recent domid list
>      destroy domain V in Xen
> 
>                                          create domain V in Xen
>                                          continue constructing V
> 
> Thread B improperly constructs a new guest using V, exposing anyone
> who was talking about V a moment ago to bugs.  Some code might even
> fail to spot the interval where V does not exist and carry on talking
> to the new V as if it were the old one...
> 
> I think there are only two possible solutions:
> 
>   - Check the domain's entry in the recent list *after* creating
>     the domain in Xen.  This involves accepting that we will
>     reuse the domid but only for a domain we are in the early
>     stages of constructing, so hopefully without bad consequence?
> 
>   - Take the recent domid lock.
> 

Or take a global file lock in libxl around domain creation and destruction?

> Also, it seems to me that we should check the recent domid list if we
> let Xen choose the domid.  Maybe that can be in a subsequent patch...
> 

Well, we could solve all this, remove the need for a file and all the associated complexity by simply keeping history inside the hypervisor. I don't know how the Xen maintainers will feel about that though, as Xen itself shouldn't have a problem with eager domid re-use.

  Paul

> Thanks,
> Ian.
Ian Jackson Jan. 17, 2020, 11:35 a.m. UTC | #7
Durrant, Paul writes ("RE: [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid"):
> [Ian:]
> > I think there are only two possible solutions:
> > 
> >   - Check the domain's entry in the recent list *after* creating
> >     the domain in Xen.  This involves accepting that we will
> >     reuse the domid but only for a domain we are in the early
> >     stages of constructing, so hopefully without bad consequence?
> > 
> >   - Take the recent domid lock.
> > 
> 
> Or take a global file lock in libxl around domain creation and destruction?

We want domain construction to be concurrent, when it can be.  So I
think a lock around just xc_domain_create is OK but a lock around the
whole operation is not.

> > Also, it seems to me that we should check the recent domid list if we
> > let Xen choose the domid.  Maybe that can be in a subsequent patch...
> 
> Well, we could solve all this, remove the need for a file and all the associated complexity by simply keeping history inside the hypervisor. I don't know how the Xen maintainers will feel about that though, as Xen itself shouldn't have a problem with eager domid re-use.

I think this doesn't need to be done in the hypervisor so I am
inclined to say it shouldn't be.  Also, there is a lot of policy here...

Ian.
Durrant, Paul Jan. 17, 2020, 12:06 p.m. UTC | #8
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 17 January 2020 12:36
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> jandryuk@gmail.com
> Subject: RE: [PATCH v3 4/6] libxl: allow creation of domains with a
> specified or random domid
> 
> Durrant, Paul writes ("RE: [PATCH v3 4/6] libxl: allow creation of domains
> with a specified or random domid"):
> > [Ian:]
> > > I think there are only two possible solutions:
> > >
> > >   - Check the domain's entry in the recent list *after* creating
> > >     the domain in Xen.  This involves accepting that we will
> > >     reuse the domid but only for a domain we are in the early
> > >     stages of constructing, so hopefully without bad consequence?
> > >
> > >   - Take the recent domid lock.
> > >
> >
> > Or take a global file lock in libxl around domain creation and
> destruction?
> 
> We want domain construction to be concurrent, when it can be.  So I
> think a lock around just xc_domain_create is OK but a lock around the
> whole operation is not.
> 
> > > Also, it seems to me that we should check the recent domid list if we
> > > let Xen choose the domid.  Maybe that can be in a subsequent patch...
> >
> > Well, we could solve all this, remove the need for a file and all the
> associated complexity by simply keeping history inside the hypervisor. I
> don't know how the Xen maintainers will feel about that though, as Xen
> itself shouldn't have a problem with eager domid re-use.
> 
> I think this doesn't need to be done in the hypervisor so I am
> inclined to say it shouldn't be.  Also, there is a lot of policy here...
> 

Ok, to cover all bases then it seems like checking the domid after creation and then destroying if it is too recent is the better option.

  Paul
Ian Jackson Jan. 17, 2020, 3:30 p.m. UTC | #9
Durrant, Paul writes ("RE: [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid"):
> Ok, to cover all bases then it seems like checking the domid after creation and then destroying if it is too recent is the better option.

I think so, yes.  I think the recent timestamp should be updated in
this case.  (Faff!)

Ian.
Durrant, Paul Jan. 20, 2020, 8:18 a.m. UTC | #10
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 17 January 2020 15:31
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> jandryuk@gmail.com
> Subject: RE: [PATCH v3 4/6] libxl: allow creation of domains with a
> specified or random domid
> 
> Durrant, Paul writes ("RE: [PATCH v3 4/6] libxl: allow creation of domains
> with a specified or random domid"):
> > Ok, to cover all bases then it seems like checking the domid after
> creation and then destroying if it is too recent is the better option.
> 
> I think so, yes.  I think the recent timestamp should be updated in
> this case.  (Faff!)
> 

I don't think we need to mess with the time-stamp in this case. The domain will be killed very quickly, before any PV backends are built and IIUC that's what we care about when it comes to re-using domids too quickly.

  Paul

Patch
diff mbox series

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 18c1a2d6bf..7e60ee1c8b 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1268,6 +1268,14 @@  void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
  */
 #define LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG
 
+/*
+ * LIBXL_HAVE_CREATEINFO_DOMID
+ *
+ * libxl_domain_create_new() and libxl_domain_create_restore() will use
+ * a domid specified in libxl_domain_create_info().
+ */
+#define LIBXL_HAVE_CREATEINFO_DOMID
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
@@ -1528,6 +1536,7 @@  int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
 /* domain related functions */
 
 #define INVALID_DOMID ~0
+#define RANDOM_DOMID (INVALID_DOMID - 1)
 
 /* If the result is ERROR_ABORTED, the domain may or may not exist
  * (in a half-created state).  *domid will be valid and will be the
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1835a5502c..a80d4f3755 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -600,9 +600,43 @@  int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
             goto out;
         }
 
-        ret = xc_domain_create(ctx->xch, domid, &create);
+        if (libxl_domid_valid_guest(info->domid)) {
+            *domid = info->domid;
+
+            if (libxl__is_retired_domid(gc, *domid)) {
+                LOGED(ERROR, *domid, "domain id is retired");
+                rc = ERROR_FAIL;
+                goto out;
+            }
+        } else if (info->domid == RANDOM_DOMID) {
+            *domid = 0; /* Zero-out initial value */
+        }
+
+        for (;;) {
+            if (info->domid == RANDOM_DOMID) {
+                uint16_t v;
+
+                /* Randomize lower order bytes */
+                ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
+                if (ret < 0)
+                    break;
+
+                v &= DOMID_MASK;
+                if (!libxl_domid_valid_guest(v) ||
+                    libxl__is_retired_domid(gc, v))
+                    continue;
+
+                *domid = v;
+            }
+
+            ret = xc_domain_create(ctx->xch, domid, &create);
+            if (ret == 0 || errno != EEXIST || info->domid != RANDOM_DOMID)
+                break;
+        }
+
         if (ret < 0) {
             LOGED(ERROR, *domid, "domain creation fail");
+            *domid = INVALID_DOMID;
             rc = ERROR_FAIL;
             goto out;
         }
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index ba5637358e..dc6aaa9c9f 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -234,7 +234,7 @@  void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
     fileline[sizeof(fileline)-1] = 0;
 
     domain[0] = 0;
-    if (domid != INVALID_DOMID)
+    if (libxl_domid_valid_guest(domid))
         snprintf(domain, sizeof(domain), "Domain %"PRIu32":", domid);
  x:
     xtl_log(ctx->lg, msglevel, errnoval, "libxl",
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 7921950f6a..d0d431614f 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -409,6 +409,7 @@  libxl_domain_create_info = Struct("domain_create_info",[
     ("ssidref",      uint32),
     ("ssid_label",   string),
     ("name",         string),
+    ("domid",        libxl_domid),
     ("uuid",         libxl_uuid),
     ("xsdata",       libxl_key_value_list),
     ("platformdata", libxl_key_value_list),
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index d2198dffad..fade089a7b 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -614,6 +614,9 @@  DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
 /* Idle domain. */
 #define DOMID_IDLE           xen_mk_uint(0x7FFF)
 
+/* Mask for valid domain id values */
+#define DOMID_MASK           0x7FFF
+
 #ifndef __ASSEMBLY__
 
 typedef uint16_t domid_t;