| Message ID | 20200116093602.4203-5-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xl/libxl: domid allocation/preservation changes | expand |
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
> -----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
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
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>
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.
> -----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.
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.
> -----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
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.
> -----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
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;
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(-)