All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] libs/light: fix a race when domain are destroied and created concurrently
@ 2022-01-12 16:41 Dario Faggioli
  2022-01-12 16:41 ` [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus Dario Faggioli
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Dario Faggioli @ 2022-01-12 16:41 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Anthony PERARD, Juergen Gross, James Fehlig,
	Anthony PERARD, Wei Liu, Juergen Gross, James Fehlig

It is possible to encounter a segfault in libxl during concurrent domain
create and destroy operations.

This is because Placement of existing domains on the host's CPUs is examined
when creating a new domain, but the existing logic does not tolerate well a
domain disappearing during the examination.

The race is quite difficult to trigger. Whan that happens, this is an
example of a backtrace:

#0  libxl_bitmap_dispose (map=map@entry=0x50) at libxl_utils.c:626
#1  0x00007fe72c993a32 in libxl_vcpuinfo_dispose (p=p@entry=0x38) at _libxl_types.c:692
#2  0x00007fe72c94e3c4 in libxl_vcpuinfo_list_free (list=0x0, nr=<optimized out>) at libxl_utils.c:1059
#3  0x00007fe72c9528bf in nr_vcpus_on_nodes (vcpus_on_node=0x7fe71000eb60, suitable_cpumap=0x7fe721df0d38, tinfo_elements=48, tinfo=0x7fe7101b3900, gc=0x7fe7101bbfa0) at libxl_numa.c:258
#4  libxl__get_numa_candidate (gc=gc@entry=0x7fe7100033a0, min_free_memkb=4233216, min_cpus=4, min_nodes=min_nodes@entry=0, max_nodes=max_nodes@entry=0, suitable_cpumap=suitable_cpumap@entry=0x7fe721df0d38, numa_cmpf=0x7fe72c940110 <numa_cmpf>, cndt_out=0x7fe721df0cf0, cndt_found=0x7fe721df0cb4) at libxl_numa.c:394
#5  0x00007fe72c94152b in numa_place_domain (d_config=0x7fe721df11b0, domid=975, gc=0x7fe7100033a0) at libxl_dom.c:209
#6  libxl__build_pre (gc=gc@entry=0x7fe7100033a0, domid=domid@entry=975, d_config=d_config@entry=0x7fe721df11b0, state=state@entry=0x7fe710077700) at libxl_dom.c:436
#7  0x00007fe72c92c4a5 in libxl__domain_build (gc=0x7fe7100033a0, d_config=d_config@entry=0x7fe721df11b0, domid=975, state=0x7fe710077700) at libxl_create.c:444
#8  0x00007fe72c92de8b in domcreate_bootloader_done (egc=0x7fe721df0f60, bl=0x7fe7100778c0, rc=<optimized out>) at libxl_create.c:1222
#9  0x00007fe72c980425 in libxl__bootloader_run (egc=egc@entry=0x7fe721df0f60, bl=bl@entry=0x7fe7100778c0) at libxl_bootloader.c:403
#10 0x00007fe72c92f281 in initiate_domain_create (egc=egc@entry=0x7fe721df0f60, dcs=dcs@entry=0x7fe7100771b0) at libxl_create.c:1159
#11 0x00007fe72c92f456 in do_domain_create (ctx=ctx@entry=0x7fe71001c840, d_config=d_config@entry=0x7fe721df11b0, domid=domid@entry=0x7fe721df10a8, restore_fd=restore_fd@entry=-1, send_back_fd=send_back_fd@entry=-1, params=params@entry=0x0, ao_how=0x0, aop_console_how=0x7fe721df10f0) at libxl_create.c:1856
#12 0x00007fe72c92f776 in libxl_domain_create_new (ctx=0x7fe71001c840, d_config=d_config@entry=0x7fe721df11b0, domid=domid@entry=0x7fe721df10a8, ao_how=ao_how@entry=0x0, aop_console_how=aop_console_how@entry=0x7fe721df10f0) at libxl_create.c:2075

Luckily, it is easy to close the race, by just making sure that
libvxl_list_vcpu() returns 0 as the number of vCPUs of the domain, when
it also returns NULL as the list of them.

Regards
---
Dario Faggioli (2):
      tools/libs/light: numa placement: don't try to free a NULL list of vcpus
      tools/libs/light: don't touch nr_vcpus_out if listing vcpus and returning NULL

 tools/libs/light/libxl_domain.c | 14 ++++++++------
 tools/libs/light/libxl_numa.c   |  4 +++-
 2 files changed, 11 insertions(+), 7 deletions(-)
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus
  2022-01-12 16:41 [PATCH 0/2] libs/light: fix a race when domain are destroied and created concurrently Dario Faggioli
@ 2022-01-12 16:41 ` Dario Faggioli
  2022-01-13 12:05   ` Anthony PERARD
  2022-01-12 16:41 ` [PATCH 2/2] tools/libs/light: don't touch nr_vcpus_out if listing vcpus and returning NULL Dario Faggioli
  2022-01-13 11:46 ` [PATCH 0/2] libs/light: fix a race when domain are destroied and created concurrently Dario Faggioli
  2 siblings, 1 reply; 9+ messages in thread
From: Dario Faggioli @ 2022-01-12 16:41 UTC (permalink / raw)
  To: xen-devel; +Cc: James Fehlig, Wei Liu, Anthony PERARD, Juergen Gross

If libxl_vcpu_list() returned NULL, we should not call
libxl_numainfo_list_free() as:
1) it'll fail trying to (double) free() *list
2) there should be nothing to free anyway

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
Tested-by: James Fehlig <jfehlig@suse.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
---
 tools/libs/light/libxl_numa.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_numa.c b/tools/libs/light/libxl_numa.c
index a8a75f89e9..3679028c79 100644
--- a/tools/libs/light/libxl_numa.c
+++ b/tools/libs/light/libxl_numa.c
@@ -253,9 +253,9 @@ static int nr_vcpus_on_nodes(libxl__gc *gc, libxl_cputopology *tinfo,
             }
         }
 
+        libxl_vcpuinfo_list_free(vinfo, nr_dom_vcpus);
  next:
         libxl_cpupoolinfo_dispose(&cpupool_info);
-        libxl_vcpuinfo_list_free(vinfo, nr_dom_vcpus);
     }
 
     libxl_bitmap_dispose(&dom_nodemap);




^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] tools/libs/light: don't touch nr_vcpus_out if listing vcpus and returning NULL
  2022-01-12 16:41 [PATCH 0/2] libs/light: fix a race when domain are destroied and created concurrently Dario Faggioli
  2022-01-12 16:41 ` [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus Dario Faggioli
@ 2022-01-12 16:41 ` Dario Faggioli
  2022-01-13 12:38   ` Anthony PERARD
  2022-01-13 11:46 ` [PATCH 0/2] libs/light: fix a race when domain are destroied and created concurrently Dario Faggioli
  2 siblings, 1 reply; 9+ messages in thread
From: Dario Faggioli @ 2022-01-12 16:41 UTC (permalink / raw)
  To: xen-devel; +Cc: James Fehlig, Wei Liu, Anthony PERARD, Juergen Gross

If we are in libvxl_list_vcpu() and we are returning NULL, let's avoid
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>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
---
 tools/libs/light/libxl_domain.c |   14 ++++++++------
 tools/libs/light/libxl_numa.c   |    4 +++-
 2 files changed, 11 insertions(+), 7 deletions(-)

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);




^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] libs/light: fix a race when domain are destroied and created concurrently
  2022-01-12 16:41 [PATCH 0/2] libs/light: fix a race when domain are destroied and created concurrently Dario Faggioli
  2022-01-12 16:41 ` [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus Dario Faggioli
  2022-01-12 16:41 ` [PATCH 2/2] tools/libs/light: don't touch nr_vcpus_out if listing vcpus and returning NULL Dario Faggioli
@ 2022-01-13 11:46 ` Dario Faggioli
  2 siblings, 0 replies; 9+ messages in thread
From: Dario Faggioli @ 2022-01-13 11:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Jim Fehlig, wl, anthony.perard

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On Wed, 2022-01-12 at 17:41 +0100, Dario Faggioli wrote:
> It is possible to encounter a segfault in libxl during concurrent
> domain
> create and destroy operations.
> 
> This is because Placement of existing domains on the host's CPUs is
> examined
> when creating a new domain, but the existing logic does not tolerate
> well a
> domain disappearing during the examination.
> 
Oh, one more thing. This has been first encountered and reproduced on
our SLE15.2, which ships Xen 4.13 so, if the fixes are accepted
upstream, I think they should also be backported (to all supported
branches, I would say).

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus
  2022-01-12 16:41 ` [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus Dario Faggioli
@ 2022-01-13 12:05   ` Anthony PERARD
  2022-01-14 23:22     ` Dario Faggioli
  0 siblings, 1 reply; 9+ messages in thread
From: Anthony PERARD @ 2022-01-13 12:05 UTC (permalink / raw)
  To: Dario Faggioli; +Cc: xen-devel, James Fehlig, Wei Liu, Juergen Gross

On Wed, Jan 12, 2022 at 05:41:36PM +0100, Dario Faggioli wrote:
> If libxl_vcpu_list() returned NULL, we should not call
> libxl_numainfo_list_free() as:

You mean libxl_vcpuinfo_list_free() ?

> 1) it'll fail trying to (double) free() *list

This isn't really an issue. free(NULL) is legit, can be call as many
time as you want.

> 2) there should be nothing to free anyway

The issue here is that it doesn't appear to be true. Even if "info" is
NULL, "nr" have an other value than 0, so libxl_vcpuinfo_list_free()
will try to access random addresses.

> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> Tested-by: James Fehlig <jfehlig@suse.com>

Can I suggest to make libxl_vcpuinfo_list_free() work a bit better in
case it's "nr" parameter is wrong? It will do nothing if "list" is NULL.
Even if that seems wrong, and the caller should use the correct value.

Also I think it is better to keep the free in the exit path at the end
of the loop.

Thanks,

-- 
Anthony PERARD


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] tools/libs/light: don't touch nr_vcpus_out if listing vcpus and returning NULL
  2022-01-12 16:41 ` [PATCH 2/2] tools/libs/light: don't touch nr_vcpus_out if listing vcpus and returning NULL Dario Faggioli
@ 2022-01-13 12:38   ` Anthony PERARD
  0 siblings, 0 replies; 9+ messages in thread
From: Anthony PERARD @ 2022-01-13 12:38 UTC (permalink / raw)
  To: Dario Faggioli; +Cc: xen-devel, James Fehlig, Wei Liu, Juergen Gross

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,

-- 
Anthony PERARD


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus
  2022-01-13 12:05   ` Anthony PERARD
@ 2022-01-14 23:22     ` Dario Faggioli
  2022-01-17 15:56       ` Anthony PERARD
  0 siblings, 1 reply; 9+ messages in thread
From: Dario Faggioli @ 2022-01-14 23:22 UTC (permalink / raw)
  To: anthony.perard; +Cc: Juergen Gross, Jim Fehlig, wl, xen-devel

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

On Thu, 2022-01-13 at 12:05 +0000, Anthony PERARD wrote:
> On Wed, Jan 12, 2022 at 05:41:36PM +0100, Dario Faggioli wrote:
> 
> > 2) there should be nothing to free anyway
> 
> The issue here is that it doesn't appear to be true. Even if "info"
> is
> NULL, "nr" have an other value than 0, so libxl_vcpuinfo_list_free()
> will try to access random addresses.
> 
My point here was that if vinfo is NULL (because libxl_list_vcpu()
returned NULL), there aren't any list element or any list to free, so
we can avoid calling libxl_vcpuinfo_list_free().

Then, sure, if we call it with a NULL vinfo and a non-zero
nr_dom_vcpus, things explode, but this was being addressed in patch 2.

This first one was really only about not trying to free an empty list.
And not to workaround the fact that it currently can make things
explode, just because it feels pointless.

> > Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> > Tested-by: James Fehlig <jfehlig@suse.com>
> 
> Can I suggest to make libxl_vcpuinfo_list_free() work a bit better in
> case it's "nr" parameter is wrong? It will do nothing if "list" is
> NULL.
>
I thought about that, and we certainly can do it.

However, I think it's unrelated to this patch so, if I do it, I'll do
it in its own one.

Also, if we go that way, I guess we want to change
libxl_cputopology_list_free(), libxl_pcitopology_list_free(),
libxl_numainfo_list_free(), libxl_dominfo_list_free(),
libxl_vminfo_list_free() and libxl_cpupoolinfo_list_free(), don't we?

> Also I think it is better to keep the free in the exit path at the
> end
> of the loop.
> 
Can I ask why as, as I was trying to say above, if we are sent directly
to next by one of the goto, we do know that vinfo is NULL and
libxl_vcpuinfo_list_free() will be basically a NOP, however it is
implemented.

Of course, you're the maintainer of this code, so I'll do like that if
you ask... I'm just curious. :-)

Actually, if you really think that the call to
libxl_vcpuinfo_list_free() should stay where it is, I can just drop
this patch and go on with patch 2 only, which is enough for fixing the
crash.

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus
  2022-01-14 23:22     ` Dario Faggioli
@ 2022-01-17 15:56       ` Anthony PERARD
  2022-01-19  9:02         ` Dario Faggioli
  0 siblings, 1 reply; 9+ messages in thread
From: Anthony PERARD @ 2022-01-17 15:56 UTC (permalink / raw)
  To: Dario Faggioli; +Cc: Juergen Gross, Jim Fehlig, wl, xen-devel

On Fri, Jan 14, 2022 at 11:22:00PM +0000, Dario Faggioli wrote:
> On Thu, 2022-01-13 at 12:05 +0000, Anthony PERARD wrote:
> > On Wed, Jan 12, 2022 at 05:41:36PM +0100, Dario Faggioli wrote:
> > 
> > > 2) there should be nothing to free anyway
> > 
> > The issue here is that it doesn't appear to be true. Even if "info"
> > is
> > NULL, "nr" have an other value than 0, so libxl_vcpuinfo_list_free()
> > will try to access random addresses.
> > 
> My point here was that if vinfo is NULL (because libxl_list_vcpu()
> returned NULL), there aren't any list element or any list to free, so
> we can avoid calling libxl_vcpuinfo_list_free().
> 
> Then, sure, if we call it with a NULL vinfo and a non-zero
> nr_dom_vcpus, things explode, but this was being addressed in patch 2.
> 
> This first one was really only about not trying to free an empty list.
> And not to workaround the fact that it currently can make things
> explode, just because it feels pointless.
> 
> > > Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> > > Tested-by: James Fehlig <jfehlig@suse.com>
> > 
> > Can I suggest to make libxl_vcpuinfo_list_free() work a bit better in
> > case it's "nr" parameter is wrong? It will do nothing if "list" is
> > NULL.
> >
> I thought about that, and we certainly can do it.
> 
> However, I think it's unrelated to this patch so, if I do it, I'll do
> it in its own one.
> 
> Also, if we go that way, I guess we want to change
> libxl_cputopology_list_free(), libxl_pcitopology_list_free(),
> libxl_numainfo_list_free(), libxl_dominfo_list_free(),
> libxl_vminfo_list_free() and libxl_cpupoolinfo_list_free(), don't we?

I actually don't know if that would be useful. Those functions do work
as expected (I think) with the right parameters: they do nothing when
called with (NULL, 0). free(NULL) does nothing.

So checking for NULL before using `nr` would probably just be a
workaround for programming mistake in the function allocating the object
or some missing initialisation in the caller. So I don't think it is
important anymore.

> > Also I think it is better to keep the free in the exit path at the
> > end
> > of the loop.
> > 
> Can I ask why as, as I was trying to say above, if we are sent directly
> to next by one of the goto, we do know that vinfo is NULL and
> libxl_vcpuinfo_list_free() will be basically a NOP, however it is
> implemented.
> 
> Of course, you're the maintainer of this code, so I'll do like that if
> you ask... I'm just curious. :-)

Freeing resources should always been attempted, even if that mean to
check whether there's something to free before calling the associated
free function. Imagine someone adding some code that could fail after
the libxl_list_vcpu(), then when that new code fails it would `goto
next;` and the allocated data from libxl_list_vcpu() would never be
freed.

> Actually, if you really think that the call to
> libxl_vcpuinfo_list_free() should stay where it is, I can just drop
> this patch and go on with patch 2 only, which is enough for fixing the
> crash.

This patch is just a workaround a bug in libxl_list_vcpu(), so I think
it can be dropped.

Cheers,

-- 
Anthony PERARD


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus
  2022-01-17 15:56       ` Anthony PERARD
@ 2022-01-19  9:02         ` Dario Faggioli
  0 siblings, 0 replies; 9+ messages in thread
From: Dario Faggioli @ 2022-01-19  9:02 UTC (permalink / raw)
  To: anthony.perard; +Cc: Juergen Gross, Jim Fehlig, wl, xen-devel

[-- Attachment #1: Type: text/plain, Size: 2913 bytes --]

On Mon, 2022-01-17 at 15:56 +0000, Anthony PERARD wrote:
> On Fri, Jan 14, 2022 at 11:22:00PM +0000, Dario Faggioli wrote:
> > 
> > Also, if we go that way, I guess we want to change
> > libxl_cputopology_list_free(), libxl_pcitopology_list_free(),
> > libxl_numainfo_list_free(), libxl_dominfo_list_free(),
> > libxl_vminfo_list_free() and libxl_cpupoolinfo_list_free(), don't
> > we?
> 
> I actually don't know if that would be useful. Those functions do
> work
> as expected (I think) with the right parameters: they do nothing when
> called with (NULL, 0). free(NULL) does nothing.
> 
Right.

> So checking for NULL before using `nr` would probably just be a
> workaround for programming mistake in the function allocating the
> object
> or some missing initialisation in the caller. So I don't think it is
> important anymore.
> 
Agreed. That's why, as I said, I though about doing something like
that, but ended up deciding not to.

> > > Also I think it is better to keep the free in the exit path at
> > > the
> > > end
> > > of the loop.
> > > 
> > Can I ask why as, as I was trying to say above, if we are sent
> > directly
> > to next by one of the goto, we do know that vinfo is NULL and
> > libxl_vcpuinfo_list_free() will be basically a NOP, however it is
> > implemented.
> > 
> > Of course, you're the maintainer of this code, so I'll do like that
> > if
> > you ask... I'm just curious. :-)
> 
> Freeing resources should always been attempted, even if that mean to
> check whether there's something to free before calling the associated
> free function. Imagine someone adding some code that could fail after
> the libxl_list_vcpu(), then when that new code fails it would `goto
> next;` and the allocated data from libxl_list_vcpu() would never be
> freed.
> 
Yeah, sure, whoever adds code that does a 'goto next' with an allocated
list, should also realize that libxl_vcpuinfo_list_free() needs to be
moved after 'next' itself, as a consequence of the very change being
done, and this seems fair to me.

But, at the end of the day, it's not a big deal at all. Thanks for
satisfying my curiosity and, since you also agree on...

> > Actually, if you really think that the call to
> > libxl_vcpuinfo_list_free() should stay where it is, I can just drop
> > this patch and go on with patch 2 only, which is enough for fixing
> > the
> > crash.
> 
> This patch is just a workaround a bug in libxl_list_vcpu(), so I
> think
> it can be dropped.
> 
...this, I'll just drop this patch and proceed with just what, in this
series, was patch 2.

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-01-19  9:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-12 16:41 [PATCH 0/2] libs/light: fix a race when domain are destroied and created concurrently Dario Faggioli
2022-01-12 16:41 ` [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus Dario Faggioli
2022-01-13 12:05   ` Anthony PERARD
2022-01-14 23:22     ` Dario Faggioli
2022-01-17 15:56       ` Anthony PERARD
2022-01-19  9:02         ` Dario Faggioli
2022-01-12 16:41 ` [PATCH 2/2] tools/libs/light: don't touch nr_vcpus_out if listing vcpus and returning NULL Dario Faggioli
2022-01-13 12:38   ` Anthony PERARD
2022-01-13 11:46 ` [PATCH 0/2] libs/light: fix a race when domain are destroied and created concurrently Dario Faggioli

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.