[PATCH] fix uint64 underflow
According to previous statement, 'free_mem' is less than 'needed_mem'. So, the subtraction 'free_mem - needed_mem' is negative, and will raise uint64 underflow. Signed-off-by: Dmitry Frolov <frolov@swemel.ru> --- src/libxl/libxl_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 6c167df63e..36be042971 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -940,7 +940,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config) if (free_mem >= needed_mem) return 0; - target_mem = free_mem - needed_mem; + target_mem = needed_mem - free_mem; if (libxlSetMemoryTargetWrapper(ctx, 0, target_mem, /* relative */ 1, 0) < 0) goto error; -- 2.34.1
On 9/12/23 16:50, Dmitry Frolov wrote:
According to previous statement, 'free_mem' is less than 'needed_mem'. So, the subtraction 'free_mem - needed_mem' is negative, and will raise uint64 underflow.
Signed-off-by: Dmitry Frolov <frolov@swemel.ru> --- src/libxl/libxl_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 6c167df63e..36be042971 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -940,7 +940,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config) if (free_mem >= needed_mem) return 0;
- target_mem = free_mem - needed_mem; + target_mem = needed_mem - free_mem; if (libxlSetMemoryTargetWrapper(ctx, 0, target_mem, /* relative */ 1, 0) < 0) goto error;
Yeah, this fixes the underflow, but I worry about the while semantics a bit. What the code effectively does: libxl_domain_need_memory(&needed_mem); do { libxl_get_free_memory(&free_mem); if (free_mem >= needed_mem) return 0; target_mem = needed_mem - free_mem; libxl_set_memory_target(target_mem); } while(...) Now, if libxl_domain_need_memory() returns how much memory a domain really needs, then why undergo trouble of getting its free memory? Why not pass it to set_memory_target() right away? Or am I misunderstanding something? Michal
Hi, Michal Of course, this changes the semantics. To my opinion, it is obvious from the source code, that we need some additional memory. I really doubt, that we intended to allocate some Exabytes additionally (using dirty underflow hack by the way). Dmitry 18.09.2023 10:59, Michal Prívozník пишет:
On 9/12/23 16:50, Dmitry Frolov wrote:
According to previous statement, 'free_mem' is less than 'needed_mem'. So, the subtraction 'free_mem - needed_mem' is negative, and will raise uint64 underflow.
Signed-off-by: Dmitry Frolov <frolov@swemel.ru> --- src/libxl/libxl_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 6c167df63e..36be042971 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -940,7 +940,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config) if (free_mem >= needed_mem) return 0;
- target_mem = free_mem - needed_mem; + target_mem = needed_mem - free_mem; if (libxlSetMemoryTargetWrapper(ctx, 0, target_mem, /* relative */ 1, 0) < 0) goto error; Yeah, this fixes the underflow, but I worry about the while semantics a bit. What the code effectively does:
libxl_domain_need_memory(&needed_mem);
do { libxl_get_free_memory(&free_mem);
if (free_mem >= needed_mem) return 0;
target_mem = needed_mem - free_mem; libxl_set_memory_target(target_mem); } while(...)
Now, if libxl_domain_need_memory() returns how much memory a domain really needs, then why undergo trouble of getting its free memory? Why not pass it to set_memory_target() right away?
Or am I misunderstanding something?
Michal
On 9/18/23 02:22, Дмитрий Фролов wrote:
Hi, Michal
Of course, this changes the semantics. To my opinion, it is obvious from the source code, that we need some additional memory. I really doubt, that we intended to allocate some Exabytes additionally (using dirty underflow hack by the way).
libxlSetMemoryTargetWrapper would have at least prevented allocating > LLONG_MAX :-). I took a stab at fixing it, along with general brokeness of the dom0 autoballoon logic https://listman.redhat.com/archives/libvir-list/2023-September/242160.html It's compile-tested only ATM. I'll check the functionality later today after repurposing a test system. Regards, Jim
Dmitry
18.09.2023 10:59, Michal Prívozník пишет:
On 9/12/23 16:50, Dmitry Frolov wrote:
According to previous statement, 'free_mem' is less than 'needed_mem'. So, the subtraction 'free_mem - needed_mem' is negative, and will raise uint64 underflow.
Signed-off-by: Dmitry Frolov <frolov@swemel.ru> --- src/libxl/libxl_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 6c167df63e..36be042971 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -940,7 +940,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config) if (free_mem >= needed_mem) return 0; - target_mem = free_mem - needed_mem; + target_mem = needed_mem - free_mem; if (libxlSetMemoryTargetWrapper(ctx, 0, target_mem, /* relative */ 1, 0) < 0) goto error; Yeah, this fixes the underflow, but I worry about the while semantics a bit. What the code effectively does:
libxl_domain_need_memory(&needed_mem);
do { libxl_get_free_memory(&free_mem);
if (free_mem >= needed_mem) return 0;
target_mem = needed_mem - free_mem; libxl_set_memory_target(target_mem); } while(...)
Now, if libxl_domain_need_memory() returns how much memory a domain really needs, then why undergo trouble of getting its free memory? Why not pass it to set_memory_target() right away?
Or am I misunderstanding something?
Michal
participants (4)
-
Dmitry Frolov -
Jim Fehlig -
Michal Prívozník -
Дмитрий Фролов