| Message ID | 164200570276.24755.1849386285622380597.stgit@work (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libs/light: fix a race when domain are destroied and created concurrently | expand |
On Wed, Jan 12, 2022 at 05:41:42PM +0100, Dario Faggioli wrote: > If we are in libvxl_list_vcpu() and we are returning NULL, let's avoid extra 'v' ^ here. > touching the output parameter *nr_vcpus_out (which should contain the > number of vcpus in the list). Ideally, the caller initialized it to 0, > which is therefore consistent with us returning NULL (or, as an alternative, > we can explicitly set it to 0 if we're returning null... But just not > touching it seems the best behavior). > > In fact, the current behavior is especially problematic if, for > instance, a domain is destroyed after we have done some steps of the > for() loop. In which case, calls like xc_vcpu_getinfo() or > xc_vcpu_getaffinity() will start to fail, and we return back to the > caller inconsistent information, such as a NULL list of vcpus, but a > modified and not 0 any longer, number of vcpus in the list. > > Signed-off-by: Dario Faggioli <dfaggioli@suse.com> > Tested-by: James Fehlig <jfehlig@suse.com> > --- > diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c > index 544a9bf59d..aabc264e16 100644 > --- a/tools/libs/light/libxl_domain.c > +++ b/tools/libs/light/libxl_domain.c > @@ -1705,6 +1706,7 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid, > ptr->vcpu_time = vcpuinfo.cpu_time; > } > GC_FREE; > + *nr_vcpus_out = nr_vcpus; Can you swap those two lines, to keep GC_FREE just before return? > return ret; > > err: > diff --git a/tools/libs/light/libxl_numa.c b/tools/libs/light/libxl_numa.c > index 3679028c79..b04e3917a0 100644 > --- a/tools/libs/light/libxl_numa.c > +++ b/tools/libs/light/libxl_numa.c > @@ -219,8 +219,10 @@ static int nr_vcpus_on_nodes(libxl__gc *gc, libxl_cputopology *tinfo, > goto next; > > vinfo = libxl_list_vcpu(CTX, dinfo[i].domid, &nr_dom_vcpus, &nr_cpus); > - if (vinfo == NULL) > + if (vinfo == NULL) { > + assert(nr_dom_vcpus == 0); I don't think this assert is necessary. > goto next; > + } Otherwise, this patch looks fine. Thanks,
diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c index 544a9bf59d..aabc264e16 100644 --- a/tools/libs/light/libxl_domain.c +++ b/tools/libs/light/libxl_domain.c @@ -1661,6 +1661,7 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid, libxl_vcpuinfo *ptr, *ret; xc_domaininfo_t domaininfo; xc_vcpuinfo_t vcpuinfo; + int nr_vcpus; if (xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo) != 1) { LOGED(ERROR, domid, "Getting infolist"); @@ -1677,27 +1678,27 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid, ret = ptr = libxl__calloc(NOGC, domaininfo.max_vcpu_id + 1, sizeof(libxl_vcpuinfo)); - for (*nr_vcpus_out = 0; - *nr_vcpus_out <= domaininfo.max_vcpu_id; - ++*nr_vcpus_out, ++ptr) { + for (nr_vcpus = 0; + nr_vcpus <= domaininfo.max_vcpu_id; + ++nr_vcpus, ++ptr) { libxl_bitmap_init(&ptr->cpumap); if (libxl_cpu_bitmap_alloc(ctx, &ptr->cpumap, 0)) goto err; libxl_bitmap_init(&ptr->cpumap_soft); if (libxl_cpu_bitmap_alloc(ctx, &ptr->cpumap_soft, 0)) goto err; - if (xc_vcpu_getinfo(ctx->xch, domid, *nr_vcpus_out, &vcpuinfo) == -1) { + if (xc_vcpu_getinfo(ctx->xch, domid, nr_vcpus, &vcpuinfo) == -1) { LOGED(ERROR, domid, "Getting vcpu info"); goto err; } - if (xc_vcpu_getaffinity(ctx->xch, domid, *nr_vcpus_out, + if (xc_vcpu_getaffinity(ctx->xch, domid, nr_vcpus, ptr->cpumap.map, ptr->cpumap_soft.map, XEN_VCPUAFFINITY_SOFT|XEN_VCPUAFFINITY_HARD) == -1) { LOGED(ERROR, domid, "Getting vcpu affinity"); goto err; } - ptr->vcpuid = *nr_vcpus_out; + ptr->vcpuid = nr_vcpus; ptr->cpu = vcpuinfo.cpu; ptr->online = !!vcpuinfo.online; ptr->blocked = !!vcpuinfo.blocked; @@ -1705,6 +1706,7 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid, ptr->vcpu_time = vcpuinfo.cpu_time; } GC_FREE; + *nr_vcpus_out = nr_vcpus; return ret; err: diff --git a/tools/libs/light/libxl_numa.c b/tools/libs/light/libxl_numa.c index 3679028c79..b04e3917a0 100644 --- a/tools/libs/light/libxl_numa.c +++ b/tools/libs/light/libxl_numa.c @@ -219,8 +219,10 @@ static int nr_vcpus_on_nodes(libxl__gc *gc, libxl_cputopology *tinfo, goto next; vinfo = libxl_list_vcpu(CTX, dinfo[i].domid, &nr_dom_vcpus, &nr_cpus); - if (vinfo == NULL) + if (vinfo == NULL) { + assert(nr_dom_vcpus == 0); goto next; + } /* Retrieve the domain's node-affinity map */ libxl_domain_get_nodeaffinity(CTX, dinfo[i].domid, &dom_nodemap);