[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: David Hildenbrand
Subject: Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly
Date: Thu, 27 Sep 2018 13:58:45 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

On 27/09/2018 13:53, Igor Mammedov wrote:
> On Thu, 27 Sep 2018 10:58:47 +0200
> David Hildenbrand <address@hidden> wrote:
>> On 27/09/2018 10:47, Igor Mammedov wrote:
>>> On Thu, 27 Sep 2018 10:13:07 +0200
>>> David Hildenbrand <address@hidden> wrote:
>>>> 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;  
>>> that should never happen, nor valid in any conditions
>>> just add assert so we would catch error if some patch would introduce it
>>>> 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.  
>>> assert(address_space_end > address_space_start)
>>> would be much better for unrealistic values without messing up with
>>> meaning of variables.
>>>> 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)  
>>> start,end pair is a range, there shouldn't be any other interpretations to
>>> this variables.
>> Please see include/qemu/range.h:struct Range;
>> So I am using _the definition_.
>>>>> 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.  
>>> Looks like I have to insist on dropping this hunk.  
>> You're going from "I'd" to "I have to insist". Please make up your mind.
> my suggestions heavily influenced by the fact that the code should
> be easy (for me and hopefully for others) to maintain in future,
> when I have to review related code later down the road when original
> author is not reachable/care anymore. (i.e. by pure selfishness)
> So far I'm not convinced that this change is for a better hence we stuck
> without consensus.
>> I have a R-b from David. But I am willing to change it if you can give
>> me a good reason why we should add asserts instead of fixing the code
>> (== making it work in all possibly valid scenarios, especially end of
>> address space).
> if it's about valid cases then it's worth fixing,
> but how many of the cases targeted here are in need of fixing in practice?
> I'd keep it simple so it would be readable and do the job
> but not clutter it with not necessary logic.
> If we =actually= need to handle every possible permutation, it might
> be better to reuse range_foo() helpers you've mentioned, instead of
> open coding conditions over again. But only if we really need it or
> if it makes current code simpler to read.

We would probably have to add some new range_* helpers for that,
something I like to avoid now. So to make some progress, I will keep the
changes in this file minimal for now (e.g. specifying a device with a
huge size is IMHO possible, so we should fix that).

Maybe we can instead focus our energy on the more important parts
(namely Patch 18, 23, and 24) - I would really like to hear your opinion
about these. Thanks!



David / dhildenb

reply via email to

[Prev in Thread] Current Thread [Next in Thread]