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

[Joe Hershberger]

Hi Simon,

On Wed, Jul 22, 2015 at 9:31 AM, Simon Glass <sjg at chromium.org> wrote:
> +Masahiro and Albert
>
> Hi Joe,
>
> On 15 July 2015 at 11:41, Joe Hershberger <joe.hershberger at gmail.com> wrote:
>> Hi Simon,
>>
>> On Thu, Jul 9, 2015 at 9:15 AM, Simon Glass <sjg at chromium.org> wrote:
>>> 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. 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:
>>>
>>> Invalid access to device priv: dev=eth at 10002000, expecting
>>>    'struct eth_sandbox_priv', requested 'struct eth_pdata'
>>
>> This is interesting, but seems like something that could just be
>> handled with good naming of structs. Type safety is great, but this is
>> fairly heavy-handed way of doing it and only helps after hitting that
>> code path in testing. Is there not a way to make the compiler help
>> out? Maybe each uclass / driver should have wrapper functions that add
>> type. Then the various uses all get type safety from the compiler and
>> the driver only has to verify that the wrappers are correct. E.g.:
>>
>> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
>> index 4e083d3..b01ae0c 100644
>> --- a/drivers/net/sandbox.c
>> +++ b/drivers/net/sandbox.c
>> @@ -33,6 +33,11 @@ struct eth_sandbox_priv {
>>  static bool disabled[8] = {false};
>>  static bool skip_timeout;
>>
>> +static struct eth_sandbox_priv *eth_sandbox_get_priv(struct udevice *dev)
>> +{
>> +       return dev_get_priv(dev);
>> +}
>> +
>>  /*
>>   * sandbox_eth_disable_response()
>>   *
>> @@ -56,7 +61,7 @@ void sandbox_eth_skip_timeout(void)
>>
>>  static int sb_eth_start(struct udevice *dev)
>>  {
>> -       struct eth_sandbox_priv *priv = dev_get_priv(dev);
>> +       struct eth_sandbox_priv *priv = sandbox_eth_get_priv(dev);
>>
>>         debug("eth_sandbox: Start\n");
>>
>
> Yes we could do this, and then the onus is on whoever writes these
> functions to get them right. But this would avoid getting it right in
> 9 out of 10 places and then messing up in one.
>
> Unfortunately it requires writing these functions for every driver.
> But that might be preferable to adding to the declaration overhead at
> every site the data is declared. The functions  could be static inline
> and placed at the top of each driver. Essentially we would be
> mandating a certain code style for drivers.

That's what I was thinking. It also seems less magic, which I generally prefer.

>>> 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.
>>
>> It seems like it should be the more generic approach.
>
> Which approach is more generic?

I was thinking the dlmalloc prepending a token would be a fairly
generic approach. I assume it would only be enabled in a debug build.

>>> 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>
>>
>> Cheers,
>> -Joe
>
> Regards,
> Simon

Cheers,
-Joe