[PATCH for-4.5 0/2] Fix `xl vcpu-list` with partially created domains

[PATCH for-4.5 0/2] Fix `xl vcpu-list` with partially created domains

[Andrew Cooper]

This series fixes a bug to do with listing the vcpus of a domain lacking any.
This can occur if there was an error constructing a domain, and it wasn't
successfully cleaned up.

This series fixes two problems.  First that getdomaininfo() in Xen previously
had no way of expressing "this domain has no vcpus", and second that libxl
would blindly allocate memory with an uninitialised value in the case that it
encountered a domain in this state.

This bug was discovered against Xen 4.4, but the bug is present in -unstable
as well.  As such, I flag it for backport ASAP.

Andrew Cooper (2):
  xen/domctl: Fix getdomaininfo() of a domain without vcpus
  tools/[lib]xl: Fix `xl vcpu-list` when domains without vcpus exist

 tools/libxl/libxl.c         |    8 +++++++-
 tools/libxl/xl_cmdimpl.c    |    4 +---
 xen/common/domctl.c         |    1 +
 xen/include/public/domctl.h |    1 +
 4 files changed, 10 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[PATCH for-4.5 1/2] xen/domctl: Fix getdomaininfo() of a domain without vcpus

[Andrew Cooper]

In the case that a domain has not got any vcpus, getdomaininfo() leaves
info->max_vcpu_id uninitialised, which causes a problem for toolstacks.

Unfortunately, setting it to 0 as a default is not appropriate.  A max_vcpu_id
of 0 and nr_online_cpus of 0 is the valid state for a single vcpu domain which
is in the process of being destroyed.

As all components are required to add 1 to max_vcpu_id to get the number of
vcpus, an id of ~0U is not valid to be used.  Explicitly define this as an
invalid max vcpu value, and use it to express "no vcpus" in getdomaininfo()

Reported-by: Euan Harris <euan.harris@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/common/domctl.c         |    1 +
 xen/include/public/domctl.h |    1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index b6f9708..d9c2635 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -156,6 +156,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     struct vcpu_runstate_info runstate;
     
     info->domain = d->domain_id;
+    info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
     info->nr_online_vcpus = 0;
     info->ssidref = 0;
     
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index f519524..58b19e7 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -108,6 +108,7 @@ struct xen_domctl_getdomaininfo {
     uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */
     uint64_aligned_t cpu_time;
     uint32_t nr_online_vcpus;    /* Number of VCPUs currently online. */
+#define XEN_INVALID_MAX_VCPU_ID (~0U) /* Domain has no vcpus? */
     uint32_t max_vcpu_id;        /* Maximum VCPUID in use by this domain. */
     uint32_t ssidref;
     xen_domain_handle_t handle;
-- 
1.7.10.4

Re: [PATCH for-4.5 1/2] xen/domctl: Fix getdomaininfo() of a domain without vcpus

[Jan Beulich]

>>> On 20.10.14 at 18:21, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -156,6 +156,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>      struct vcpu_runstate_info runstate;
>      
>      info->domain = d->domain_id;
> +    info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
>      info->nr_online_vcpus = 0;
>      info->ssidref = 0;

Afaict this will convert the so far random allocation failure in the
tool stack to a guaranteed one. Hence I think that despite them
touching hypervisor and tools, the patches should be folded to
not cause an apparent regression between applying the first
and second to people who are lucky in not hitting the allocation
failure right now.

In any event the hypervisor side is
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH for-4.5 1/2] xen/domctl: Fix getdomaininfo() of a domain without vcpus

[George Dunlap]

On Tue, Oct 21, 2014 at 9:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.10.14 at 18:21, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -156,6 +156,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>>      struct vcpu_runstate_info runstate;
>>
>>      info->domain = d->domain_id;
>> +    info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
>>      info->nr_online_vcpus = 0;
>>      info->ssidref = 0;
>
> Afaict this will convert the so far random allocation failure in the
> tool stack to a guaranteed one. Hence I think that despite them
> touching hypervisor and tools, the patches should be folded to
> not cause an apparent regression between applying the first
> and second to people who are lucky in not hitting the allocation
> failure right now.

+1

 -George

[PATCH for-4.5 2/2] tools/[lib]xl: Fix `xl vcpu-list` when domains without vcpus exist

[Andrew Cooper]

On a system which looks like this:

[root@st04 ~]# xl list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   752     4     r-----   46699.3
(null)                                       1     0     0     --p---       0.0
(null)                                       2     0     0     --p---       0.0
(null)                                       3     0     0     --p---       0.0
badger                                      25     0     1     --p---       0.0

`xl vcpu-list` failes as so:

[root@st04 ~]# xl vcpu-list
Name                                ID  VCPU   CPU State   Time(s) CPU Affinity
Domain-0                             0     0    0   -b-   12171.0  all
Domain-0                             0     1    1   -b-   11779.6  all
Domain-0                             0     2    2   -b-   11599.0  all
Domain-0                             0     3    3   r--   11007.0  all
libxl: critical: libxl__calloc: libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)
: Cannot allocate memory
libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)

The root cause of this is that, in the case that a domain has no vcpus,
libxl_list_vcpu() attempts to calloc() with domaininfo.nr_online_vcpus, which
is uninitialised on the stack.

Check domaininfo.nr_online_vcpus against the new sentinel value
XEN_INVALID_MAX_VCPU_ID (introduced in the previous patch), and return early.
This means that it is now valid for libxl_list_vcpu() to return NULL for a
domain which lacks any vcpus.

As part of this change, remove the pointless call to libxl_get_max_cpus(),
whose returned value is unconditionally clobbered in the for() loop.

Reported-by: Euan Harris <euan.harris@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c      |    8 +++++++-
 tools/libxl/xl_cmdimpl.c |    4 +---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f27b581..d7e2442 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5246,7 +5246,13 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
         GC_FREE;
         return NULL;
     }
-    *nr_cpus_out = libxl_get_max_cpus(ctx);
+
+    if (domaininfo.nr_online_vcpus == XEN_INVALID_MAX_VCPU_ID) {
+        *nr_cpus_out = 0;
+        GC_FREE;
+        return NULL;
+    }
+
     ret = ptr = libxl__calloc(NOGC, domaininfo.max_vcpu_id + 1,
                               sizeof(libxl_vcpuinfo));
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ea43761..0882644 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4813,10 +4813,8 @@ static void print_domain_vcpuinfo(uint32_t domid, uint32_t nr_cpus)
 
     vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu, &nrcpus);
 
-    if (!vcpuinfo) {
-        fprintf(stderr, "libxl_list_vcpu failed.\n");
+    if (!vcpuinfo)
         return;
-    }
 
     for (i = 0; i < nb_vcpu; i++) {
         print_vcpuinfo(domid, &vcpuinfo[i], nr_cpus);
-- 
1.7.10.4

[PATCH for-4.5 v2 2/2] tools/[lib]xl: Fix `xl vcpu-list` when domains without vcpus exist

[Andrew Cooper]

On a system which looks like this:

[root@st04 ~]# xl list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   752     4     r-----   46699.3
(null)                                       1     0     0     --p---       0.0
(null)                                       2     0     0     --p---       0.0
(null)                                       3     0     0     --p---       0.0
badger                                      25     0     1     --p---       0.0

`xl vcpu-list` failes as so:

[root@st04 ~]# xl vcpu-list
Name                                ID  VCPU   CPU State   Time(s) CPU Affinity
Domain-0                             0     0    0   -b-   12171.0  all
Domain-0                             0     1    1   -b-   11779.6  all
Domain-0                             0     2    2   -b-   11599.0  all
Domain-0                             0     3    3   r--   11007.0  all
libxl: critical: libxl__calloc: libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)
: Cannot allocate memory
libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)

The root cause of this is that, in the case that a domain has no vcpus,
libxl_list_vcpu() attempts to calloc() with domaininfo.max_vcpu_id, which
is uninitialised on the stack.

Check domaininfo.max_vcpu_id against the new sentinel value
XEN_INVALID_MAX_VCPU_ID (introduced in the previous patch), and return early.
This means that it is now valid for libxl_list_vcpu() to return NULL for a
domain which lacks any vcpus.

As part of this change, remove the pointless call to libxl_get_max_cpus(),
whose returned value is unconditionally clobbered in the for() loop.

Reported-by: Euan Harris <euan.harris@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

---
v2: s/nr_online_cpus/max_vcpu_id/g - I accidentally sent an unrefreshed patch
---
 tools/libxl/libxl.c      |    8 +++++++-
 tools/libxl/xl_cmdimpl.c |    4 +---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f27b581..b7a9952 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5246,7 +5246,13 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
         GC_FREE;
         return NULL;
     }
-    *nr_cpus_out = libxl_get_max_cpus(ctx);
+
+    if (domaininfo.max_vcpu_id == XEN_INVALID_MAX_VCPU_ID) {
+        *nr_cpus_out = 0;
+        GC_FREE;
+        return NULL;
+    }
+
     ret = ptr = libxl__calloc(NOGC, domaininfo.max_vcpu_id + 1,
                               sizeof(libxl_vcpuinfo));
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ea43761..0882644 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4813,10 +4813,8 @@ static void print_domain_vcpuinfo(uint32_t domid, uint32_t nr_cpus)
 
     vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu, &nrcpus);
 
-    if (!vcpuinfo) {
-        fprintf(stderr, "libxl_list_vcpu failed.\n");
+    if (!vcpuinfo)
         return;
-    }
 
     for (i = 0; i < nb_vcpu; i++) {
         print_vcpuinfo(domid, &vcpuinfo[i], nr_cpus);
-- 
1.7.10.4

Re: [PATCH for-4.5 v2 2/2] tools/[lib]xl: Fix `xl vcpu-list` when domains without vcpus exist

[Wei Liu]

On Mon, Oct 20, 2014 at 05:38:54PM +0100, Andrew Cooper wrote:
> On a system which looks like this:
> 
> [root@st04 ~]# xl list
> Name                                        ID   Mem VCPUs      State   Time(s)
> Domain-0                                     0   752     4     r-----   46699.3
> (null)                                       1     0     0     --p---       0.0
> (null)                                       2     0     0     --p---       0.0
> (null)                                       3     0     0     --p---       0.0
> badger                                      25     0     1     --p---       0.0
> 
> `xl vcpu-list` failes as so:
> 
> [root@st04 ~]# xl vcpu-list
> Name                                ID  VCPU   CPU State   Time(s) CPU Affinity
> Domain-0                             0     0    0   -b-   12171.0  all
> Domain-0                             0     1    1   -b-   11779.6  all
> Domain-0                             0     2    2   -b-   11599.0  all
> Domain-0                             0     3    3   r--   11007.0  all
> libxl: critical: libxl__calloc: libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)
> : Cannot allocate memory
> libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)
> 
> The root cause of this is that, in the case that a domain has no vcpus,
> libxl_list_vcpu() attempts to calloc() with domaininfo.max_vcpu_id, which
> is uninitialised on the stack.
> 
> Check domaininfo.max_vcpu_id against the new sentinel value
> XEN_INVALID_MAX_VCPU_ID (introduced in the previous patch), and return early.
> This means that it is now valid for libxl_list_vcpu() to return NULL for a
> domain which lacks any vcpus.
> 
> As part of this change, remove the pointless call to libxl_get_max_cpus(),
> whose returned value is unconditionally clobbered in the for() loop.
> 
> Reported-by: Euan Harris <euan.harris@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> ---
> v2: s/nr_online_cpus/max_vcpu_id/g - I accidentally sent an unrefreshed patch
> ---
>  tools/libxl/libxl.c      |    8 +++++++-
>  tools/libxl/xl_cmdimpl.c |    4 +---
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index f27b581..b7a9952 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5246,7 +5246,13 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
>          GC_FREE;
>          return NULL;
>      }
> -    *nr_cpus_out = libxl_get_max_cpus(ctx);
> +
> +    if (domaininfo.max_vcpu_id == XEN_INVALID_MAX_VCPU_ID) {
> +        *nr_cpus_out = 0;

Do you not need to set nr_vcpus_out to 0 as well?

Wei.

Re: [PATCH for-4.5 v2 2/2] tools/[lib]xl: Fix `xl vcpu-list` when domains without vcpus exist

[Andrew Cooper]

On 21/10/14 14:06, Wei Liu wrote:
> On Mon, Oct 20, 2014 at 05:38:54PM +0100, Andrew Cooper wrote:
>> On a system which looks like this:
>>
>> [root@st04 ~]# xl list
>> Name                                        ID   Mem VCPUs      State   Time(s)
>> Domain-0                                     0   752     4     r-----   46699.3
>> (null)                                       1     0     0     --p---       0.0
>> (null)                                       2     0     0     --p---       0.0
>> (null)                                       3     0     0     --p---       0.0
>> badger                                      25     0     1     --p---       0.0
>>
>> `xl vcpu-list` failes as so:
>>
>> [root@st04 ~]# xl vcpu-list
>> Name                                ID  VCPU   CPU State   Time(s) CPU Affinity
>> Domain-0                             0     0    0   -b-   12171.0  all
>> Domain-0                             0     1    1   -b-   11779.6  all
>> Domain-0                             0     2    2   -b-   11599.0  all
>> Domain-0                             0     3    3   r--   11007.0  all
>> libxl: critical: libxl__calloc: libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)
>> : Cannot allocate memory
>> libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)
>>
>> The root cause of this is that, in the case that a domain has no vcpus,
>> libxl_list_vcpu() attempts to calloc() with domaininfo.max_vcpu_id, which
>> is uninitialised on the stack.
>>
>> Check domaininfo.max_vcpu_id against the new sentinel value
>> XEN_INVALID_MAX_VCPU_ID (introduced in the previous patch), and return early.
>> This means that it is now valid for libxl_list_vcpu() to return NULL for a
>> domain which lacks any vcpus.
>>
>> As part of this change, remove the pointless call to libxl_get_max_cpus(),
>> whose returned value is unconditionally clobbered in the for() loop.
>>
>> Reported-by: Euan Harris <euan.harris@citrix.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>>
>> ---
>> v2: s/nr_online_cpus/max_vcpu_id/g - I accidentally sent an unrefreshed patch
>> ---
>>  tools/libxl/libxl.c      |    8 +++++++-
>>  tools/libxl/xl_cmdimpl.c |    4 +---
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index f27b581..b7a9952 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -5246,7 +5246,13 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
>>          GC_FREE;
>>          return NULL;
>>      }
>> -    *nr_cpus_out = libxl_get_max_cpus(ctx);
>> +
>> +    if (domaininfo.max_vcpu_id == XEN_INVALID_MAX_VCPU_ID) {
>> +        *nr_cpus_out = 0;
> Do you not need to set nr_vcpus_out to 0 as well?

Hmm - probably not.  NULL should do.

I will respin a v3 with the hypervisor and tool sides merged

~Andrew

Re: [PATCH for-4.5 v2 2/2] tools/[lib]xl: Fix `xl vcpu-list` when domains without vcpus exist

[Wei Liu]

On Tue, Oct 21, 2014 at 02:08:37PM +0100, Andrew Cooper wrote:
[...]
> >>          GC_FREE;
> >>          return NULL;
> >>      }
> >> -    *nr_cpus_out = libxl_get_max_cpus(ctx);
> >> +
> >> +    if (domaininfo.max_vcpu_id == XEN_INVALID_MAX_VCPU_ID) {
> >> +        *nr_cpus_out = 0;
> > Do you not need to set nr_vcpus_out to 0 as well?
> 
> Hmm - probably not.  NULL should do.
> 

Just setting one out parameter not the other looks a bit odd to me.
Nothing fundamental wrong with that though, as you said, it's mostly the
NULL return value indicates failure.

Wei.