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

[Igor Mammedov]

On Thu, 27 Sep 2018 13:58:45 +0200
David Hildenbrand <address@hidden> wrote:

> 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).
It's not a rush time before release where patch fixing something is
good enough reason to convince me even if it's not the way it should be
written.

To minimize changes it's fine to just drop this patch.
If you can reproduce real problem it's fine to fix it in this series
or separately and do simplification later.

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