In a very distant past, we came around machines that has not
continuous node IDs. This made us error out when constructing
capabilities XML. We resolved it by utilizing strange behaviour
of numa_node_to_cpus() in which it returned a mask with all bits
set for a non-existent node. However, this is not the only case
when it returns all ones mask - if the node exists and has enough
CPUs to fill the mask up (e.g. 128 CPUs).

The fix consists of using nodemask_isset(&numa_all_nodes, ..)
prior to calling numa_node_to_cpus() to determine if the node
exists.

Fixes: 628c93574758abb59e71160042524d321a33543f
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1860231
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virnuma.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index eeca438f25..75d5628cff 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -256,31 +256,23 @@ virNumaGetNodeCPUs(int node,
     int mask_n_bytes = max_n_cpus / 8;
     size_t i;
     g_autofree unsigned long *mask = NULL;
-    g_autofree unsigned long *allonesmask = NULL;
     g_autoptr(virBitmap) cpumap = NULL;
 
     *cpus = NULL;
 
+    if (!nodemask_isset(&numa_all_nodes, node)) {
+        VIR_DEBUG("NUMA topology for cell %d is not available, ignoring", node);
+        return -2;
+    }
+
     if (VIR_ALLOC_N(mask, mask_n_bytes / sizeof(*mask)) < 0)
         return -1;
 
-    if (VIR_ALLOC_N(allonesmask, mask_n_bytes / sizeof(*mask)) < 0)
-        return -1;
-
-    memset(allonesmask, 0xff, mask_n_bytes);
-
-    /* The first time this returns -1, ENOENT if node doesn't exist... */
     if (numa_node_to_cpus(node, mask, mask_n_bytes) < 0) {
         VIR_WARN("NUMA topology for cell %d is not available, ignoring", node);
         return -2;
     }
 
-    /* second, third... times it returns an all-1's mask */
-    if (memcmp(mask, allonesmask, mask_n_bytes) == 0) {
-        VIR_DEBUG("NUMA topology for cell %d is invalid, ignoring", node);
-        return -2;
-    }
-
     if (!(cpumap = virBitmapNew(max_n_cpus)))
         return -1;
 
-- 
2.26.2

On Fri, Aug 21, 2020 at 05:12:08PM +0200, Michal Privoznik wrote:
> In a very distant past, we came around machines that has not
> continuous node IDs. This made us error out when constructing
> capabilities XML. We resolved it by utilizing strange behaviour
> of numa_node_to_cpus() in which it returned a mask with all bits
> set for a non-existent node. However, this is not the only case
> when it returns all ones mask - if the node exists and has enough
> CPUs to fill the mask up (e.g. 128 CPUs).
> 
> The fix consists of using nodemask_isset(&numa_all_nodes, ..)
> prior to calling numa_node_to_cpus() to determine if the node
> exists.
> 
> Fixes: 628c93574758abb59e71160042524d321a33543f
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1860231
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/virnuma.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/src/util/virnuma.c b/src/util/virnuma.c
> index eeca438f25..75d5628cff 100644
> --- a/src/util/virnuma.c
> +++ b/src/util/virnuma.c
> @@ -256,31 +256,23 @@ virNumaGetNodeCPUs(int node,
>      int mask_n_bytes = max_n_cpus / 8;
>      size_t i;
>      g_autofree unsigned long *mask = NULL;
> -    g_autofree unsigned long *allonesmask = NULL;
>      g_autoptr(virBitmap) cpumap = NULL;
>  
>      *cpus = NULL;
>  
> +    if (!nodemask_isset(&numa_all_nodes, node)) {
> +        VIR_DEBUG("NUMA topology for cell %d is not available, ignoring", node);
> +        return -2;
> +    }
> +
>      if (VIR_ALLOC_N(mask, mask_n_bytes / sizeof(*mask)) < 0)
>          return -1;
>  
> -    if (VIR_ALLOC_N(allonesmask, mask_n_bytes / sizeof(*mask)) < 0)
> -        return -1;
> -
> -    memset(allonesmask, 0xff, mask_n_bytes);
> -
> -    /* The first time this returns -1, ENOENT if node doesn't exist... */
>      if (numa_node_to_cpus(node, mask, mask_n_bytes) < 0) {
>          VIR_WARN("NUMA topology for cell %d is not available, ignoring", node);
>          return -2;
>      }
>  
> -    /* second, third... times it returns an all-1's mask */
> -    if (memcmp(mask, allonesmask, mask_n_bytes) == 0) {
> -        VIR_DEBUG("NUMA topology for cell %d is invalid, ignoring", node);
> -        return -2;
> -    }
> -
>      if (!(cpumap = virBitmapNew(max_n_cpus)))
>          return -1;

Nice, that's simpler than I expected the fix would be.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|