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

[Joe Hershberger]

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");


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

> 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