When automatically adding a NUMA node (qemuDomainDefNumaAutoAdd()) the
memory size of the node is computed as:

  total_memory - sum(memory devices)

And we have a nice helper for that: virDomainDefGetMemoryInitial() so
it looks logical to just call it. Except, this code runs in post parse
callback, i.e. memory sizes were not validated and it may happen that
the sum is greater than the total memory. This would be caught by
virDomainDefPostParseMemory() but that runs only after driver specific
callbacks (i.e. after qemuDomainDefNumaAutoAdd()) and because the
domain config was changed and memory was increased to this huge
number no error is caught.

So let's do what virDomainDefGetMemoryInitial() would do, but
with error checking.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2216236
Fixes: f5d4f5c8ee44e9f1939070afcc5381bdd5545e50
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6eea8a9fa5..fdda001795 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4821,17 +4821,24 @@ qemuDomainDefNumaAutoAdd(virDomainDef *def,
         return 0;
     }
 
-    initialMem = virDomainDefGetMemoryInitial(def);
+    initialMem = virDomainDefGetMemoryTotal(def);
 
     if (!def->numa)
         def->numa = virDomainNumaNew();
 
     virDomainNumaSetNodeCount(def->numa, 1);
-    virDomainNumaSetNodeMemorySize(def->numa, 0, initialMem);
 
     for (i = 0; i < def->nmems; i++) {
         virDomainMemoryDef *mem = def->mems[i];
 
+        if (mem->size > initialMem) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("Total size of memory devices exceeds the total memory size"));
+            return -1;
+        }
+
+        initialMem -= mem->size;
+
         switch (mem->model) {
         case VIR_DOMAIN_MEMORY_MODEL_DIMM:
         case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
@@ -4848,6 +4855,8 @@ qemuDomainDefNumaAutoAdd(virDomainDef *def,
         }
     }
 
+    virDomainNumaSetNodeMemorySize(def->numa, 0, initialMem);
+
     return 0;
 }
 
-- 
2.41.0

On Fri, Jul 21, 2023 at 1:13 PM Michal Privoznik <mprivozn@redhat.com>
wrote:

> When automatically adding a NUMA node (qemuDomainDefNumaAutoAdd()) the
> memory size of the node is computed as:
>
>   total_memory - sum(memory devices)
>
> And we have a nice helper for that: virDomainDefGetMemoryInitial() so
> it looks logical to just call it. Except, this code runs in post parse
> callback, i.e. memory sizes were not validated and it may happen that
> the sum is greater than the total memory. This would be caught by
> virDomainDefPostParseMemory() but that runs only after driver specific
> callbacks (i.e. after qemuDomainDefNumaAutoAdd()) and because the
> domain config was changed and memory was increased to this huge
> number no error is caught.
>
> So let's do what virDomainDefGetMemoryInitial() would do, but
> with error checking.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2216236
> Fixes: f5d4f5c8ee44e9f1939070afcc5381bdd5545e50
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6eea8a9fa5..fdda001795 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4821,17 +4821,24 @@ qemuDomainDefNumaAutoAdd(virDomainDef *def,
>          return 0;
>      }
>
> -    initialMem = virDomainDefGetMemoryInitial(def);
> +    initialMem = virDomainDefGetMemoryTotal(def);
>

I would prefer if this variable was renamed to totalMem / remainingMem /
availableMem.


>
>      if (!def->numa)
>          def->numa = virDomainNumaNew();
>
>      virDomainNumaSetNodeCount(def->numa, 1);
> -    virDomainNumaSetNodeMemorySize(def->numa, 0, initialMem);
>
>      for (i = 0; i < def->nmems; i++) {
>          virDomainMemoryDef *mem = def->mems[i];
>
> +        if (mem->size > initialMem) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Total size of memory devices exceeds the
> total memory size"));
> +            return -1;
> +        }
> +
> +        initialMem -= mem->size;
> +
>          switch (mem->model) {
>          case VIR_DOMAIN_MEMORY_MODEL_DIMM:
>          case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> @@ -4848,6 +4855,8 @@ qemuDomainDefNumaAutoAdd(virDomainDef *def,
>          }
>      }
>
> +    virDomainNumaSetNodeMemorySize(def->numa, 0, initialMem);
> +
>      return 0;
>  }
>
> --
> 2.41.0
>
>
Reviewed-by: Kristina Hanicova <khanicov@redhat.com>
Kristina