| Message ID | 20200131150149.2008-6-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xl/libxl: domid allocation/preservation changes | expand |
On Fri, Jan 31, 2020 at 10:02 AM Paul Durrant <pdurrant@amazon.com> wrote: > > This patch adds a 'domid' field to libxl_domain_create_info and then > modifies libxl__domain_make() to have Xen use that value if it is valid. > 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 choose a random domid value > for Xen to use. > > If Xen determines that a domid specified to or chosen by > libxl__domain_make() co-incides with an existing domain then the create > operation will fail. In this case, if RANDOM_DOMID was specified to > libxl__domain_make() then a new random value will be chosen and the create > operation will be re-tried, otherwise libxl__domain_make() will fail. > > After Xen has successfully created a new domain, libxl__domain_make() will > check whether its domid matches any recently used domid values. If it does > then the domain will be destroyed. If the domid used in creation was > specified to libxl__domain_make() then it will fail at this point, > otherwise the create operation will be re-tried with either a new random > or Xen-selected domid value. > > 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, with one suggestion below. Reviewed-by: Jason Andryuk <jandryuk@gmail.com> <snip> > + > + /* Try to destroy the domain again as we can't use it */ > + ret = xc_domain_destroy(ctx->xch, *domid); > + if (ret < 0) { > + LOGED(ERROR, *domid, "domain destroy fail"); Maybe "destroy recently used domain id failed"? > + *domid = INVALID_DOMID; > + rc = ERROR_FAIL; > + goto out; > + } > } > > rc = libxl__arch_domain_save_config(gc, d_config, state, &create);
> -----Original Message----- > From: jandryuk@gmail.com <jandryuk@gmail.com> > Sent: 31 January 2020 17:23 > To: Durrant, Paul <pdurrant@amazon.co.uk> > Cc: xen-devel <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>; Jan Beulich > <jbeulich@suse.com>; Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org> > Subject: Re: [PATCH v5 5/7] libxl: allow creation of domains with a > specified or random domid > > On Fri, Jan 31, 2020 at 10:02 AM Paul Durrant <pdurrant@amazon.com> wrote: > > > > This patch adds a 'domid' field to libxl_domain_create_info and then > > modifies libxl__domain_make() to have Xen use that value if it is valid. > > 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 choose a random domid value > > for Xen to use. > > > > If Xen determines that a domid specified to or chosen by > > libxl__domain_make() co-incides with an existing domain then the create > > operation will fail. In this case, if RANDOM_DOMID was specified to > > libxl__domain_make() then a new random value will be chosen and the > create > > operation will be re-tried, otherwise libxl__domain_make() will fail. > > > > After Xen has successfully created a new domain, libxl__domain_make() > will > > check whether its domid matches any recently used domid values. If it > does > > then the domain will be destroyed. If the domid used in creation was > > specified to libxl__domain_make() then it will fail at this point, > > otherwise the create operation will be re-tried with either a new random > > or Xen-selected domid value. > > > > 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, with one suggestion below. > > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> > Thanks. > <snip> > > > + > > + /* Try to destroy the domain again as we can't use it */ > > + ret = xc_domain_destroy(ctx->xch, *domid); > > + if (ret < 0) { > > + LOGED(ERROR, *domid, "domain destroy fail"); > > Maybe "destroy recently used domain id failed"? > Probably doesn't actually matter. A destroy failure during domain creation would be an unusual circumstance, but if I have to post a v6 I'll change the message while I'm at it. Paul > > + *domid = INVALID_DOMID; > > + rc = ERROR_FAIL; > > + goto out; > > + } > > } > > > > rc = libxl__arch_domain_save_config(gc, d_config, state, > &create);
Paul Durrant writes ("[PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid"):
> This patch adds a 'domid' field to libxl_domain_create_info and then
> modifies libxl__domain_make() to have Xen use that value if it is valid.
> 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 choose a random domid value
> for Xen to use.
>
> If Xen determines that a domid specified to or chosen by
> libxl__domain_make() co-incides with an existing domain then the create
> operation will fail. In this case, if RANDOM_DOMID was specified to
> libxl__domain_make() then a new random value will be chosen and the create
> operation will be re-tried, otherwise libxl__domain_make() will fail.
>
> After Xen has successfully created a new domain, libxl__domain_make() will
> check whether its domid matches any recently used domid values. If it does
> then the domain will be destroyed. If the domid used in creation was
> specified to libxl__domain_make() then it will fail at this point,
> otherwise the create operation will be re-tried with either a new random
> or Xen-selected domid value.
>
> 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: Jason Andryuk <jandryuk@gmail.com>
>
> v5:
> - Flattened nested loops
>
> v4:
> - Not added Jason's R-b because of substantial change
> - Check for recent domid *after* creation
> - Re-worked commit comment
>
> 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 | 67 ++++++++++++++++++++++++++++++++----
> tools/libxl/libxl_internal.c | 2 +-
> tools/libxl/libxl_types.idl | 1 +
> xen/include/public/xen.h | 3 ++
> 5 files changed, 74 insertions(+), 8 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 1d235ecb1c..31c6f4b11a 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 3a7364e2ac..7fd4d713e7 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -555,8 +555,6 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> libxl_domain_create_info *info = &d_config->c_info;
> libxl_domain_build_info *b_info = &d_config->b_info;
>
> - assert(soft_reset || *domid == INVALID_DOMID);
> -
> uuid_string = libxl__uuid2string(gc, info->uuid);
> if (!uuid_string) {
> rc = ERROR_NOMEM;
> @@ -600,11 +598,66 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> goto out;
> }
>
> - ret = xc_domain_create(ctx->xch, domid, &create);
> - if (ret < 0) {
> - LOGED(ERROR, *domid, "domain creation fail");
> - rc = ERROR_FAIL;
> - goto out;
> + for (;;) {
> + bool recent;
> +
> + if (info->domid == RANDOM_DOMID) {
> + uint16_t v;
> +
> + ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
> + if (ret < 0)
> + break;
> +
> + v &= DOMID_MASK;
> + if (!libxl_domid_valid_guest(v))
> + continue;
> +
> + *domid = v;
> + } else
> + *domid = info->domid;
Style: { } on all or none of the same `if' series. (CODING_STYLE)
> + /* The domid is not recent, so we're done */
> + if (!recent)
> + break;
> +
> + /*
> + * If the domid was specified then there's no point in
> + * trying again.
> + */
> + if (libxl_domid_valid_guest(info->domid)) {
> + LOGED(ERROR, *domid, "domain id recently used");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + /* Try to destroy the domain again as we can't use it */
> + ret = xc_domain_destroy(ctx->xch, *domid);
> + if (ret < 0) {
> + LOGED(ERROR, *domid, "domain destroy fail");
> + *domid = INVALID_DOMID;
> + rc = ERROR_FAIL;
> + goto out;
> + }
These two seem to be in the wrong order. Also if
libxl__is_domid_recent fails, you leak the domain.
This is sort of a result of you not treating `domid' as a `local
[variable] referring to resources which might need cleaning up'.
According to a strict reading of CODING_STYLE you should initialise it
to -1 and the xc_domain_destroy out should be in the out block, but
that would duplicate the call to destroy.
I don't mind exactly how you fix this, but please make sure not to
leak the newly-created domain even in the error cases.
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index bbd4c6cba9..d93a75533f 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",
This wants to be a separate patch.
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index d2198dffad..75b1619d0d 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 xen_mk_uint(0x7FFF)
This needs a hypervisor maintainer ack.
Please split it into its own patch, with a rationale, etc.
Thanks,
ian.
> -----Original Message----- > From: Ian Jackson <ian.jackson@citrix.com> > Sent: 17 February 2020 17:52 > 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>; > Jason Andryuk <jandryuk@gmail.com> > Subject: Re: [PATCH v5 5/7] libxl: allow creation of domains with a > specified or random domid > > Paul Durrant writes ("[PATCH v5 5/7] libxl: allow creation of domains with > a specified or random domid"): > > This patch adds a 'domid' field to libxl_domain_create_info and then > > modifies libxl__domain_make() to have Xen use that value if it is valid. > > 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 choose a random domid value > > for Xen to use. > > > > If Xen determines that a domid specified to or chosen by > > libxl__domain_make() co-incides with an existing domain then the create > > operation will fail. In this case, if RANDOM_DOMID was specified to > > libxl__domain_make() then a new random value will be chosen and the > create > > operation will be re-tried, otherwise libxl__domain_make() will fail. > > > > After Xen has successfully created a new domain, libxl__domain_make() > will > > check whether its domid matches any recently used domid values. If it > does > > then the domain will be destroyed. If the domid used in creation was > > specified to libxl__domain_make() then it will fail at this point, > > otherwise the create operation will be re-tried with either a new random > > or Xen-selected domid value. > > > > 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: Jason Andryuk <jandryuk@gmail.com> > > > > v5: > > - Flattened nested loops > > > > v4: > > - Not added Jason's R-b because of substantial change > > - Check for recent domid *after* creation > > - Re-worked commit comment > > > > 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 | 67 ++++++++++++++++++++++++++++++++---- > > tools/libxl/libxl_internal.c | 2 +- > > tools/libxl/libxl_types.idl | 1 + > > xen/include/public/xen.h | 3 ++ > > 5 files changed, 74 insertions(+), 8 deletions(-) > > > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > > index 1d235ecb1c..31c6f4b11a 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 3a7364e2ac..7fd4d713e7 100644 > > --- a/tools/libxl/libxl_create.c > > +++ b/tools/libxl/libxl_create.c > > @@ -555,8 +555,6 @@ int libxl__domain_make(libxl__gc *gc, > libxl_domain_config *d_config, > > libxl_domain_create_info *info = &d_config->c_info; > > libxl_domain_build_info *b_info = &d_config->b_info; > > > > - assert(soft_reset || *domid == INVALID_DOMID); > > - > > uuid_string = libxl__uuid2string(gc, info->uuid); > > if (!uuid_string) { > > rc = ERROR_NOMEM; > > @@ -600,11 +598,66 @@ int libxl__domain_make(libxl__gc *gc, > libxl_domain_config *d_config, > > goto out; > > } > > > > - ret = xc_domain_create(ctx->xch, domid, &create); > > - if (ret < 0) { > > - LOGED(ERROR, *domid, "domain creation fail"); > > - rc = ERROR_FAIL; > > - goto out; > > + for (;;) { > > + bool recent; > > + > > + if (info->domid == RANDOM_DOMID) { > > + uint16_t v; > > + > > + ret = libxl__random_bytes(gc, (void *)&v, sizeof(v)); > > + if (ret < 0) > > + break; > > + > > + v &= DOMID_MASK; > > + if (!libxl_domid_valid_guest(v)) > > + continue; > > + > > + *domid = v; > > + } else > > + *domid = info->domid; > > Style: { } on all or none of the same `if' series. (CODING_STYLE) OK. Too used to Xen style. > > > + /* The domid is not recent, so we're done */ > > + if (!recent) > > + break; > > + > > + /* > > + * If the domid was specified then there's no point in > > + * trying again. > > + */ > > + if (libxl_domid_valid_guest(info->domid)) { > > + LOGED(ERROR, *domid, "domain id recently used"); > > + rc = ERROR_FAIL; > > + goto out; > > + } > > + > > + /* Try to destroy the domain again as we can't use it */ > > + ret = xc_domain_destroy(ctx->xch, *domid); > > + if (ret < 0) { > > + LOGED(ERROR, *domid, "domain destroy fail"); > > + *domid = INVALID_DOMID; > > + rc = ERROR_FAIL; > > + goto out; > > + } > > These two seem to be in the wrong order. Also if > libxl__is_domid_recent fails, you leak the domain. > No, the domain will not be leaked. The existing failure handling in libxl will clean up if *domid != INVALID_DOMID. > This is sort of a result of you not treating `domid' as a `local > [variable] referring to resources which might need cleaning up'. > According to a strict reading of CODING_STYLE you should initialise it > to -1 and the xc_domain_destroy out should be in the out block, but > that would duplicate the call to destroy. > > I don't mind exactly how you fix this, but please make sure not to > leak the newly-created domain even in the error cases. > I think the error handling is good (but obscured by the way libxl works), and there is no way to avoid leaking the domain if xc_domain_destroy() fails. > > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c > > index bbd4c6cba9..d93a75533f 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", > > This wants to be a separate patch. > Ok. > > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h > > index d2198dffad..75b1619d0d 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 xen_mk_uint(0x7FFF) > > This needs a hypervisor maintainer ack. > > Please split it into its own patch, with a rationale, etc. > Ok, but it has no rationale without the rest of this patch; I can only assert that it 'will be needed by a subsequent patch'. Paul > Thanks, > ian.
Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid"):
> No, the domain will not be leaked. The existing failure handling in libxl will clean up if *domid != INVALID_DOMID.
Sorry if I was confused; I will read this again.
> > > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > > index d2198dffad..75b1619d0d 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 xen_mk_uint(0x7FFF)
> >
> > This needs a hypervisor maintainer ack.
> >
> > Please split it into its own patch, with a rationale, etc.
>
> Ok, but it has no rationale without the rest of this patch; I can only assert that it 'will be needed by a subsequent patch'.
What do the Xen maintainers, and my tools co-maintainers think ?
I was hoping for a rationale along the lines of:
This is useful for programs which need to [explanation],
including for example, libxl, which is going to want to
randomly generate domids.
Maybe it needs some explanation of why this belongs in the Xen public
headers rather than in some header available to libxc, libxl and other
tools stuff ?
Ian.
> -----Original Message----- > From: Ian Jackson <ian.jackson@citrix.com> > Sent: 18 February 2020 11:17 > 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>; > Jason Andryuk <jandryuk@gmail.com> > Subject: RE: [PATCH v5 5/7] libxl: allow creation of domains with a > specified or random domid > > Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains > with a specified or random domid"): > > No, the domain will not be leaked. The existing failure handling in > libxl will clean up if *domid != INVALID_DOMID. > > Sorry if I was confused; I will read this again. > It is hard to follow the error paths. Early on in development I ended up with domains getting destroyed when I didn't want them to be (when xc_domain_create() failed due to a duplicate domid). > > > > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h > > > > index d2198dffad..75b1619d0d 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 xen_mk_uint(0x7FFF) > > > > > > This needs a hypervisor maintainer ack. > > > > > > Please split it into its own patch, with a rationale, etc. > > > > Ok, but it has no rationale without the rest of this patch; I can only > assert that it 'will be needed by a subsequent patch'. > > What do the Xen maintainers, and my tools co-maintainers think ? > > I was hoping for a rationale along the lines of: > > This is useful for programs which need to [explanation], > including for example, libxl, which is going to want to > randomly generate domids. > > Maybe it needs some explanation of why this belongs in the Xen public > headers rather than in some header available to libxc, libxl and other > tools stuff ? Ok, I'll try words along those lines. Paul > > Ian.
Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid"):
> Ian Jackson <ian.jackson@citrix.com>
> > Sorry if I was confused; I will read this again.
>
> It is hard to follow the error paths. Early on in development I ended up with domains getting destroyed when I didn't want them to be (when xc_domain_create() failed due to a duplicate domid).
Having read the patch again, I suggest the following discipline (which
is along the lines contemplated by CODYING_STYLE):
The local variable `domid' contains only a domid we are trying to
create and does not constitute a "local [variable] referring to
resources which might need cleaning up" (in the words of
CODING_STYLE). Therefore it should never be passed to destroy.
Maybe it should be called `prospective_domid'.
The variable *domid _is_ a "local [variable] referring to resources
which might need cleaning up". Therefore it must only ever contain a
domain which actually exists. It should be set from prospective_domid
when xc_domain_create succeeds, and cleared (set back to INVALID) when
xc_domain_destroy succeeds in our retry loop.
That way any `goto out' anywhere will clear up a domain iff there is
one to clear up.
There is a hunk in this patch which I think is incompatible with this
discipline:
- assert(soft_reset || *domid == INVALID_DOMID);
-
I don't understand what this hunk is for. If we adopt the discipline
I suggest, can it go away ?
Ian.
> -----Original Message----- > From: Ian Jackson <ian.jackson@citrix.com> > Sent: 18 February 2020 11:48 > 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>; > Jason Andryuk <jandryuk@gmail.com> > Subject: RE: [PATCH v5 5/7] libxl: allow creation of domains with a > specified or random domid > > Durrant, Paul writes ("RE: [PATCH v5 5/7] libxl: allow creation of domains > with a specified or random domid"): > > Ian Jackson <ian.jackson@citrix.com> > > > Sorry if I was confused; I will read this again. > > > > It is hard to follow the error paths. Early on in development I ended up > with domains getting destroyed when I didn't want them to be (when > xc_domain_create() failed due to a duplicate domid). > > Having read the patch again, I suggest the following discipline (which > is along the lines contemplated by CODYING_STYLE): > > The local variable `domid' contains only a domid we are trying to > create and does not constitute a "local [variable] referring to > resources which might need cleaning up" (in the words of > CODING_STYLE). Therefore it should never be passed to destroy. > Maybe it should be called `prospective_domid'. > > The variable *domid _is_ a "local [variable] referring to resources > which might need cleaning up". Therefore it must only ever contain a > domain which actually exists. It should be set from prospective_domid > when xc_domain_create succeeds, and cleared (set back to INVALID) when > xc_domain_destroy succeeds in our retry loop. > > That way any `goto out' anywhere will clear up a domain iff there is > one to clear up. > > There is a hunk in this patch which I think is incompatible with this > discipline: > > - assert(soft_reset || *domid == INVALID_DOMID); > - > > I don't understand what this hunk is for. If we adopt the discipline > I suggest, can it go away ? Ok, I'll give that a try. It's possible things are sufficiently complex that a sub-function may be appropriate, which should also achieve the localization. Paul > > Ian.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 1d235ecb1c..31c6f4b11a 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 3a7364e2ac..7fd4d713e7 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -555,8 +555,6 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, libxl_domain_create_info *info = &d_config->c_info; libxl_domain_build_info *b_info = &d_config->b_info; - assert(soft_reset || *domid == INVALID_DOMID); - uuid_string = libxl__uuid2string(gc, info->uuid); if (!uuid_string) { rc = ERROR_NOMEM; @@ -600,11 +598,66 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, goto out; } - ret = xc_domain_create(ctx->xch, domid, &create); - if (ret < 0) { - LOGED(ERROR, *domid, "domain creation fail"); - rc = ERROR_FAIL; - goto out; + for (;;) { + bool recent; + + if (info->domid == RANDOM_DOMID) { + uint16_t v; + + ret = libxl__random_bytes(gc, (void *)&v, sizeof(v)); + if (ret < 0) + break; + + v &= DOMID_MASK; + if (!libxl_domid_valid_guest(v)) + continue; + + *domid = v; + } else + *domid = info->domid; + + ret = xc_domain_create(ctx->xch, domid, &create); + if (ret < 0) { + /* + * If we generated a random domid and creation failed + * because that domid already exists then simply try + * again. + */ + if (errno == EEXIST && info->domid == RANDOM_DOMID) + continue; + + LOGED(ERROR, *domid, "domain creation fail"); + *domid = INVALID_DOMID; + rc = ERROR_FAIL; + goto out; + } + + rc = libxl__is_domid_recent(gc, *domid, &recent); + if (rc) + goto out; + + /* The domid is not recent, so we're done */ + if (!recent) + break; + + /* + * If the domid was specified then there's no point in + * trying again. + */ + if (libxl_domid_valid_guest(info->domid)) { + LOGED(ERROR, *domid, "domain id recently used"); + rc = ERROR_FAIL; + goto out; + } + + /* Try to destroy the domain again as we can't use it */ + ret = xc_domain_destroy(ctx->xch, *domid); + if (ret < 0) { + LOGED(ERROR, *domid, "domain destroy fail"); + *domid = INVALID_DOMID; + rc = ERROR_FAIL; + goto out; + } } rc = libxl__arch_domain_save_config(gc, d_config, state, &create); diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c index bbd4c6cba9..d93a75533f 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..75b1619d0d 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 xen_mk_uint(0x7FFF) + #ifndef __ASSEMBLY__ typedef uint16_t domid_t;
This patch adds a 'domid' field to libxl_domain_create_info and then modifies libxl__domain_make() to have Xen use that value if it is valid. 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 choose a random domid value for Xen to use. If Xen determines that a domid specified to or chosen by libxl__domain_make() co-incides with an existing domain then the create operation will fail. In this case, if RANDOM_DOMID was specified to libxl__domain_make() then a new random value will be chosen and the create operation will be re-tried, otherwise libxl__domain_make() will fail. After Xen has successfully created a new domain, libxl__domain_make() will check whether its domid matches any recently used domid values. If it does then the domain will be destroyed. If the domid used in creation was specified to libxl__domain_make() then it will fail at this point, otherwise the create operation will be re-tried with either a new random or Xen-selected domid value. 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: Jason Andryuk <jandryuk@gmail.com> v5: - Flattened nested loops v4: - Not added Jason's R-b because of substantial change - Check for recent domid *after* creation - Re-worked commit comment 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 | 67 ++++++++++++++++++++++++++++++++---- tools/libxl/libxl_internal.c | 2 +- tools/libxl/libxl_types.idl | 1 + xen/include/public/xen.h | 3 ++ 5 files changed, 74 insertions(+), 8 deletions(-)