fix uint64 underflow

[PATCH] fix uint64 underflow

Posted by Dmitry Frolov 2 years, 4 months ago

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

Re: [PATCH] fix uint64 underflow

Posted by Michal Prívozník 2 years, 4 months ago

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

Re: [PATCH] fix uint64 underflow

Posted by Дмитрий Фролов 2 years, 4 months ago

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
>

Re: [PATCH] fix uint64 underflow

Posted by Jim Fehlig 2 years, 4 months ago

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
>>
>