When Domain-0 autoballooning is enabled, it's possible that memory may
need to be ballooned down in Domain-0 to accommodate the needs of another
virtual machine. libxlDomainFreeMemory handles this task, but due to a
logic bug is underflowing the variable containing Domain-0 new
target memory. The resulting huge numbers are filtered by
libxlSetMemoryTargetWrapper and memory is not changed.

Under the covers, libxlDomainFreeMemory uses Xen's libxl_set_memory_target
API, which includes a 'relative' parameter for specifying how to set the
target. If true, the target is an increment/decrement value over the
current memory, otherwise target is taken as an absolute value.
libxlDomainFreeMemory sets 'relative' to true, but never allows for
negative values by declaring the target memory variable as an unsigned.
Fix by declaring the variable as signed, which also requried adjusting
libxlSetMemoryTargetWrapper.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_api_wrapper.h | 16 ++++++----------
 src/libxl/libxl_domain.c      |  2 +-
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/src/libxl/libxl_api_wrapper.h b/src/libxl/libxl_api_wrapper.h
index a9627f7983..c9582294cc 100644
--- a/src/libxl/libxl_api_wrapper.h
+++ b/src/libxl/libxl_api_wrapper.h
@@ -193,24 +193,20 @@ libxlSendTriggerWrapper(libxl_ctx *ctx,
 static inline int
 libxlSetMemoryTargetWrapper(libxl_ctx *ctx,
                         uint32_t domid,
-                        uint64_t target_memkb,
+                        int64_t target_memkb,
                         int relative,
                         int enforce)
 {
     int ret = -1;
 
-    /* Technically this guard could be LIBXL_HAVE_MEMKB_64BITS */
-#if LIBXL_API_VERSION < 0x040800
-    if (target_memkb < UINT_MAX) {
-        uint32_t val32 = target_memkb;
+#ifdef LIBXL_HAVE_MEMKB_64BITS
+    ret = libxl_set_memory_target(ctx, domid, target_memkb, relative, enforce);
+#else
+    if (target_memkb < INT_MAX) {
+        int32_t val32 = target_memkb;
 
         ret = libxl_set_memory_target(ctx, domid, val32, relative, enforce);
     }
-#else
-    if (target_memkb < LLONG_MAX) {
-        int64_t val64 = target_memkb;
-        ret = libxl_set_memory_target(ctx, domid, val64, relative, enforce);
-    }
 #endif
 
     return ret;
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 6c167df63e..0c4beffd6a 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -926,7 +926,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config)
 {
     uint64_t needed_mem;
     uint64_t free_mem;
-    uint64_t target_mem;
+    int64_t target_mem;
     int tries = 3;
     int wait_secs = 10;
 
-- 
2.42.0

On 9/18/23 19:16, Jim Fehlig wrote:
> When Domain-0 autoballooning is enabled, it's possible that memory may
> need to be ballooned down in Domain-0 to accommodate the needs of another
> virtual machine. libxlDomainFreeMemory handles this task, but due to a
> logic bug is underflowing the variable containing Domain-0 new
> target memory. The resulting huge numbers are filtered by
> libxlSetMemoryTargetWrapper and memory is not changed.
> 
> Under the covers, libxlDomainFreeMemory uses Xen's libxl_set_memory_target
> API, which includes a 'relative' parameter for specifying how to set the
> target. If true, the target is an increment/decrement value over the
> current memory, otherwise target is taken as an absolute value.
> libxlDomainFreeMemory sets 'relative' to true, but never allows for
> negative values by declaring the target memory variable as an unsigned.
> Fix by declaring the variable as signed, which also requried adjusting
> libxlSetMemoryTargetWrapper.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_api_wrapper.h | 16 ++++++----------
>  src/libxl/libxl_domain.c      |  2 +-
>  2 files changed, 7 insertions(+), 11 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

On 9/19/23 06:50, Michal Prívozník wrote:
> On 9/18/23 19:16, Jim Fehlig wrote:
>> When Domain-0 autoballooning is enabled, it's possible that memory may
>> need to be ballooned down in Domain-0 to accommodate the needs of another
>> virtual machine. libxlDomainFreeMemory handles this task, but due to a
>> logic bug is underflowing the variable containing Domain-0 new
>> target memory. The resulting huge numbers are filtered by
>> libxlSetMemoryTargetWrapper and memory is not changed.
>>
>> Under the covers, libxlDomainFreeMemory uses Xen's libxl_set_memory_target
>> API, which includes a 'relative' parameter for specifying how to set the
>> target. If true, the target is an increment/decrement value over the
>> current memory, otherwise target is taken as an absolute value.
>> libxlDomainFreeMemory sets 'relative' to true, but never allows for
>> negative values by declaring the target memory variable as an unsigned.
>> Fix by declaring the variable as signed, which also requried adjusting
>> libxlSetMemoryTargetWrapper.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>   src/libxl/libxl_api_wrapper.h | 16 ++++++----------
>>   src/libxl/libxl_domain.c      |  2 +-
>>   2 files changed, 7 insertions(+), 11 deletions(-)
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Thanks! Before pushing I also ensured the autoballoon logic is working as 
expected. I don't have any automated tests for that since autoballoon has not 
been enabled in SUSE distros for years.

Regards,
Jim