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

[Simon Glass]

+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.

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

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