Re: [Qemu-ppc] [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly

[David Hildenbrand]

On 27/09/2018 10:02, Igor Mammedov wrote:
> On Wed, 26 Sep 2018 11:41:59 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> Make address_space_end point at the real end, instead of end + 1, so we don't
>> have to handle special cases like it being 0. This will allow us to
>> place a memory device at the very end of the guest physical 64bit address
>> space (if ever possible).
> 
> [...]
> 
>> @@ -115,7 +116,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
>> const uint64_t *hint,
>>      }
>>      address_space_start = ms->device_memory->base;
>>      address_space_end = address_space_start +
>> -                        memory_region_size(&ms->device_memory->mr);
>> +                        memory_region_size(&ms->device_memory->mr) - 1;
> I'm terrible at math, lets assume size = 1 so after this
> 'real end' would end up at 'address_space_start', which makes
> it rather confusing beside not matching variable name anymore.

(very simply and unrealistic) counter example as given in the
description. You should get the idea.

address_space_start = 0xffffffffffffffffull;
size = 1;

-> address_space_end = 0;

While this would be perfectly valid, we would have to handle
address_space_end potentially being 0 in the code below, because this
would be a valid overflow. This, I avoid.

And form my POV, the variable name here matches perfectly. It points at
the last address of the address space. (yes people have different
opinions on this)

> 
> I'd drop it and maybe extend the following assert to abort
> on overflow condition.

I'll leave it like this, handling address_space_end = 0 is ugly.

I'll add a comment like

/* address_space_end points at the last valid address */

> 
>>      g_assert(address_space_end >= address_space_start);
>>  
>>      /* address_space_start indicates the maximum alignment we expect */
>> @@ -149,7 +150,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
>> const uint64_t *hint,
>>                         "] before 0x%" PRIx64, new_addr, size,
>>                         address_space_start);
>>              return 0;
>> -        } else if ((new_addr + size) > address_space_end) {
>> +        } else if (new_addr + size - 1 < new_addr ||
>> +                   new_addr + size - 1 > address_space_end) {
>>              error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
>>                         "] beyond 0x%" PRIx64, new_addr, size,
>>                         address_space_end);
>> @@ -182,7 +184,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
>> const uint64_t *hint,
>>          }
>>      }
>>  
>> -    if (new_addr + size > address_space_end) {
>> +    if (new_addr + size - 1 < new_addr || !new_addr ||
>> +        new_addr + size - 1 > address_space_end) {
>>          error_setg(errp, "could not find position in guest address space 
>> for "
>>                     "memory device - memory fragmented due to alignments");
>>          goto out;
> I strongly suggest replace non obvious math with several
> plain <>= conditions even if adds more conditions compared
> to math variant.
> At least reader doesn't have to do calculations manually
> to figure out what's going on
> 

Right, maybe adding a new temporary variable new_region_end will help.

-- 

Thanks,

David / dhildenb