[U-Boot] [PATCH] RFC: dm: Add pointer checking for allocated data

[Simon Glass]

+Joe and Stephen who might have comments

On 14 July 2015 at 09:47, Simon Glass <sjg at chromium.org> wrote:
> Hi Masahiro,
>
> On 13 July 2015 at 22:36, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>> Hi Simon,
>>
>>
>>
>>
>> 2015-07-09 23:15 GMT+09:00 Simon Glass <sjg at chromium.org>:
>>> With driver model drivers can have things stored in several places. There is
>>> driver-private data, then the uclass can attach things to a device. If the
>>> device is on a bus then its bus may attach parent data to the device too.
>>>
>>> At present everything is done through void pointers.
>>
>> This is also true for driver private data in Linux.
>>
>
> Yes.
>
>>
>>
>>> It would be nice to
>>> have a way to check that the correct C struct is used in each case.
>>>
>>> Here is a proposed implementation of a way of checking structures in driver
>>> model. It relies on turning the existing dev_get_priv() function into a
>>> macro which (if checking is enabled) checks that the structure names match.
>>> Each xxx_auto_alloc_size turns into a structure containing a string (the
>>> structure name) and the size.
>>>
>>> The dev_get_priv() macro has an extra parameter which is the structure being
>>> accessed:
>>>
>>>         struct eth_pdata *priv = dev_get_priv(dev, struct eth_pdata);
>>>
>>> and you get an error like this when things are wrong:
>>
>> In which scenario do things go wrong?
>> It is still unclear to me why we need this sanity check.
>
> Here struct eth_pdata is the structure for the platform data. So you should use:
>
>        struct eth_pdata *plat = dev_get_platdata(dev, struct eth_pdata);
>
> That is the error - we are using the platdata structure to access
> priv. At present there is no way to detect these errors except by
> inspection (or perhaps crash / strange behaviour).
>
>>
>>
>>
>>> Invalid access to device priv: dev=eth at 10002000, expecting
>>>    'struct eth_sandbox_priv', requested 'struct eth_pdata'
>>>
>>> A new Kconfg option is added to turn this on, since it bloats the code a
>>> little.
>>>
>>> The next step would be to extend it to all pointers in the device and
>>> uclass. This is mostly a mechanical code change.
>>>
>>> Finally we should have a way of checking that the device pointer itself is
>>> valid. For example if someone passes an invalid address like 0x12345 as the
>>> 'struct udevice' then at present we will dutifully look for the devices'
>>> driver and perform an invalid memory access.
>>>
>>> If we want to check for memory corruption one way would be to add a magic
>>> numnber before each allocated memory area. Perhaps this is more the role
>>> of dlmalloc(), but if required it could be attached to Masahiro's devres
>>> feature, which already prepends some data to every allocated area.
>>>
>>> Comments welcome. I'd like to figure this out soon as it involves trivial
>>> but invasive patches to change each driver.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>
>>
>>
>>
>>
>>
>>
>>
>>> diff --git a/arch/x86/cpu/mp_init.c b/arch/x86/cpu/mp_init.c
>>> index e686b28..e835b43 100644
>>> --- a/arch/x86/cpu/mp_init.c
>>> +++ b/arch/x86/cpu/mp_init.c
>>> @@ -238,7 +238,7 @@ static int load_sipi_vector(atomic_t **ap_countp)
>>>
>>>         params->stack_size = CONFIG_AP_STACK_SIZE;
>>>         size = params->stack_size * CONFIG_MAX_CPUS;
>>> -       stack = memalign(size, 4096);
>>> +       stack = memalign(4096, size);
>>>         if (!stack)
>>>                 return -ENOMEM;
>>>         params->stack_top = (u32)(stack + size);
>>
>>
>> This change seems unrelated to the subject of this patch.
>>
>
> Yes, I'll respin it properly when we work out what to do.
>
> Regards,
> Simon