Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid

From: Ian Jackson <ian.jackson@citrix.com>
To: Paul Durrant <pdurrant@amazon.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	Jason Andryuk <jandryuk@gmail.com>,
	George Dunlap <George.Dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Anthony Perard <anthony.perard@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v5 5/7] libxl: allow creation of domains with a specified or random domid
Date: Mon, 17 Feb 2020 17:51:31 +0000	[thread overview]
Message-ID: <24138.53923.699587.944099@mariner.uk.xensource.com> (raw)
In-Reply-To: <20200131150149.2008-6-pdurrant@amazon.com>

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel