Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid

[Ian Jackson <ian.jackson@citrix.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.


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