[U-Boot] [PATCH v5 2/2] dlmalloc: fix malloc range at end of ram

[Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>]

Tom,

Am 26.04.2019 um 13:00 schrieb Marek Vasut:
> On 4/26/19 12:19 PM, Simon Goldschmidt wrote:
>> On Fri, Apr 26, 2019 at 11:56 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>
>>> On 4/26/19 11:36 AM, Simon Goldschmidt wrote:
>>>> On Fri, Apr 26, 2019 at 11:32 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>
>>>>> On 4/26/19 8:19 AM, Simon Goldschmidt wrote:
>>>>>> Marek Vasut <marek.vasut@gmail.com> schrieb am Fr., 26. Apr. 2019, 00:22:
>>>>>>
>>>>>>> On 4/25/19 9:22 PM, Simon Goldschmidt wrote:
>>>>>>>> If the malloc range passed to mem_malloc_init() is at the end of address
>>>>>>>> range and 'start + size' overflows to 0, following allocations fail as
>>>>>>>> mem_malloc_end is zero (which looks like uninitialized).
>>>>>>>>
>>>>>>>> Fix this by subtracting 1 of 'start + size' overflows to zero.
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

Since there's no way this fits without breaking smartweb, I'd rather 
drop this for now in order to get 1/2 accepted.

Regards,
Simon

>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v5:
>>>>>>>> - this patch was 1/2 in v4 but is now 2/2 as the 2nd patch of v4 has
>>>>>>>>    already been accepted
>>>>>>>> - rearrange the code to make it only 8 bytes plus in code size for arm
>>>>>>>>    (which fixes smartweb SPL overflowing)
>>>>>>>>
>>>>>>>>   common/dlmalloc.c | 6 +++++-
>>>>>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
>>>>>>>> index 6f12a18d54..38859ecbd4 100644
>>>>>>>> --- a/common/dlmalloc.c
>>>>>>>> +++ b/common/dlmalloc.c
>>>>>>>> @@ -601,8 +601,12 @@ void *sbrk(ptrdiff_t increment)
>>>>>>>>   void mem_malloc_init(ulong start, ulong size)
>>>>>>>>   {
>>>>>>>>        mem_malloc_start = start;
>>>>>>>> -     mem_malloc_end = start + size;
>>>>>>>>        mem_malloc_brk = start;
>>>>>>>> +     mem_malloc_end = start + size;
>>>>>>>> +     if (size > mem_malloc_end) {
>>>>>>>> +             /* overflow: malloc area is at end of address range */
>>>>>>>> +             mem_malloc_end--;
>>>>>>>
>>>>>>> Does this mean a memory wrap-around happened ?
>>>>>>> I don't think decrementing malloc area size by 1 is a proper solution.
>>>>>>> You can have it overflow by 2 and decrementing by 1 won't help.
>>>>>>>
>>>>>>
>>>>>> No, not a real overflow. Instead, as I tried to described in the commit
>>>>>> message, mem_malloc_end gets 0 if the range is at the end of addr range,
>>>>>> e.g. malloc start is 0xffff0000 and malloc size is 0x10000. Subtracting 1
>>>>>> will be enough here. It reduces the available mall of aize, but I don't
>>>>>> think that should be a problem.
>>>>>
>>>>> That's a wrap-around . What happens with your example if malloc_size is
>>>>> 0x10001 ? Hint: It fails ...
>>>>
>>>> Yes it fails. But in contrast, that's an invalid configuration, while
>>>> my patch makes
>>>> a valid configuration work. I don't know if we want to fix all invalid
>>>> configurations.
>>>
>>> Yes ? Should be easy, just clamp() size to (size, (BIT(32) - 1) -
>>> mem_malloc_start) or similar for 64bit systems.
>>
>> I'm not convinced we should. This range is normally generated using
>> something like:
>> SIZE=2048
>> START=RAM_END - SIZE
> 
> Normally ... on SoCFPGA . Other ARM32 platforms can have OCRAM mapped
> somewhere in the middle of the address space. Take R-Car Gen2, which has
> it at 0xe6300000 + 64k or something like that.
> 
> And, to make things worse, you cannot detect these overflows at compile
> time, since the DRAM layout, which is passed to malloc init can come
> from DT.
> 
> Thus, you might want to sanitize the input, properly.
> 
>> I don't want to be overprotective here. I don't think there's much point
>> in fixing the out-of-ram-range check if it produces an overflow but not
>> fix it if it's in the middle of an address space.
>>
>> Again, this patch simply fixes the case for something like this:
>> RAM_SIZE=0x10000
>> RAM_START=0xFFFF0000
>> so RAM_END=0
>>
>> We can use clamp as you suggested, but what would it be good for
>> if it only fixes an out-of-range heap if an overflow occurs?
> 
> It's better than nothing. Further refinements welcome.
> 
>>>> You could as well enter a range without RAM, that would fail as well.
>>>
>>> That info is available in gd , but I wonder whether this is the right
>>> place to check for it.
>>
>> Indeed, that would seem misplaced here.
>>
>> Regards,
>> Simon
>>
>>>
>>>> A different approach to fix my valid end-of-ram configuration would be to set
>>>> the end to "start + size - 1" and to change all the checks using it. But that
>>>> would probably lead to more code size problems in various SPL...
>>>>
>>>> Regards,
>>>> Simon
>>>>
>>>>>
>>>>>> I got this when experimenting with full heap in socfpga. Due to other
>>>>>> patches not being accepted, this is not an issue currebtly, but can easily
>>>>>> become one on the future.
>>>>>>
>>>>>> Regrds,
>>>>>> Simon
>>>>>>
>>>>>>
>>>>>>>> +     }
>>>>>>>>
>>>>>>>>        debug("using memory %#lx-%#lx for malloc()\n", mem_malloc_start,
>>>>>>>>              mem_malloc_end);
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Best regards,
>>>>>>> Marek Vasut
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best regards,
>>>>> Marek Vasut
>>>
>>>
>>> --
>>> Best regards,
>>> Marek Vasut
> 
>