of_match_device could return NULL, and so cause a NULL pointer
dereference later.
Signed-off-by: LABBE Corentin <[email protected]>
---
drivers/mtd/nand/mxc_nand.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 136e73a..9e42431 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -1464,8 +1464,7 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
{
struct device_node *np = host->dev->of_node;
struct mxc_nand_platform_data *pdata = &host->pdata;
- const struct of_device_id *of_id =
- of_match_device(mxcnd_dt_ids, host->dev);
+ const struct of_device_id *of_id;
int buswidth;
if (!np)
@@ -1482,6 +1481,9 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
pdata->width = buswidth / 8;
+ of_id = of_match_device(mxcnd_dt_ids, host->dev);
+ if (!of_id)
+ return -ENODEV;
host->devtype_data = of_id->data;
return 0;
--
2.4.10
Hi,
On Thu, Nov 12, 2015 at 8:46 AM, LABBE Corentin
<[email protected]> wrote:
> of_match_device could return NULL, and so cause a NULL pointer
> dereference later.
Did you actually run into this? It seems to me that this driver is
only probed if and only if we have a match and that therefore
of_match_device will always return a valid pointer (it is using the
same match table). Am I missing something?
> Signed-off-by: LABBE Corentin <[email protected]>
> ---
> drivers/mtd/nand/mxc_nand.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 136e73a..9e42431 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -1464,8 +1464,7 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
> {
> struct device_node *np = host->dev->of_node;
> struct mxc_nand_platform_data *pdata = &host->pdata;
> - const struct of_device_id *of_id =
> - of_match_device(mxcnd_dt_ids, host->dev);
> + const struct of_device_id *of_id;
> int buswidth;
>
> if (!np)
> @@ -1482,6 +1481,9 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
>
> pdata->width = buswidth / 8;
>
> + of_id = of_match_device(mxcnd_dt_ids, host->dev);
> + if (!of_id)
> + return -ENODEV;
> host->devtype_data = of_id->data;
>
> return 0;
> --
> 2.4.10
>
Thanks,
Frans
Hello Corentin,
On Thu, Nov 12, 2015 at 08:46:55AM +0100, LABBE Corentin wrote:
> of_match_device could return NULL, and so cause a NULL pointer
> dereference later.
>
> Signed-off-by: LABBE Corentin <[email protected]>
> ---
> drivers/mtd/nand/mxc_nand.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 136e73a..9e42431 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -1464,8 +1464,7 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
> {
> struct device_node *np = host->dev->of_node;
> struct mxc_nand_platform_data *pdata = &host->pdata;
> - const struct of_device_id *of_id =
> - of_match_device(mxcnd_dt_ids, host->dev);
> + const struct of_device_id *of_id;
> int buswidth;
>
> if (!np)
> @@ -1482,6 +1481,9 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
>
> pdata->width = buswidth / 8;
>
> + of_id = of_match_device(mxcnd_dt_ids, host->dev);
> + if (!of_id)
> + return -ENODEV;
You should return 1 here instead of -ENODEV. Also this check should
better be done instead of
if (!np)
return 1;
at the start of the function. I really wonder there is no helper
function like:
#define of_sensible_name(dev) of_match_device(dev->driver->of_match_table, dev)
Best regards
Uwe
> host->devtype_data = of_id->data;
>
> return 0;
> --
> 2.4.10
>
>
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Nov 12, 2015 at 09:03:11AM +0100, Frans Klaver wrote:
> Hi,
>
> On Thu, Nov 12, 2015 at 8:46 AM, LABBE Corentin
> <[email protected]> wrote:
> > of_match_device could return NULL, and so cause a NULL pointer
> > dereference later.
>
> Did you actually run into this? It seems to me that this driver is
> only probed if and only if we have a match and that therefore
> of_match_device will always return a valid pointer (it is using the
> same match table). Am I missing something?
Yes, you're missing something. The driver would probe for a dt snippet
like:
mxc_nand {
compatible = "foobar";
}
In this case dev->of_node is non-NULL but of_match_device(mxcnd_dt_ids,
dev) is.
(I didn't actually test this, so there is a chance I'm wrong here. And
if not I wonder if it is sensible at all to match the device name on
driver name for of-created platform devices.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Nov 12, 2015 at 9:26 AM, Uwe Kleine-König
<[email protected]> wrote:
> On Thu, Nov 12, 2015 at 09:03:11AM +0100, Frans Klaver wrote:
>> Hi,
>>
>> On Thu, Nov 12, 2015 at 8:46 AM, LABBE Corentin
>> <[email protected]> wrote:
>> > of_match_device could return NULL, and so cause a NULL pointer
>> > dereference later.
>>
>> Did you actually run into this? It seems to me that this driver is
>> only probed if and only if we have a match and that therefore
>> of_match_device will always return a valid pointer (it is using the
>> same match table). Am I missing something?
>
> Yes, you're missing something. The driver would probe for a dt snippet
> like:
>
> mxc_nand {
> compatible = "foobar";
> }
>
> In this case dev->of_node is non-NULL but of_match_device(mxcnd_dt_ids,
> dev) is.
>
> (I didn't actually test this, so there is a chance I'm wrong here. And
> if not I wonder if it is sensible at all to match the device name on
> driver name for of-created platform devices.)
Yea, looks like you're right. platform devices check a number of
things to determine a match, among which is driver name if all else
fails (platform.c, platform_match()).
Thanks,
Frans
CC += [email protected], gregkh
On Thu, Nov 12, 2015 at 09:36:55AM +0100, Frans Klaver wrote:
> On Thu, Nov 12, 2015 at 9:26 AM, Uwe Kleine-K?nig
> <[email protected]> wrote:
> > On Thu, Nov 12, 2015 at 09:03:11AM +0100, Frans Klaver wrote:
> >> Hi,
> >>
> >> On Thu, Nov 12, 2015 at 8:46 AM, LABBE Corentin
> >> <[email protected]> wrote:
> >> > of_match_device could return NULL, and so cause a NULL pointer
> >> > dereference later.
> >>
> >> Did you actually run into this? It seems to me that this driver is
> >> only probed if and only if we have a match and that therefore
> >> of_match_device will always return a valid pointer (it is using the
> >> same match table). Am I missing something?
> >
> > Yes, you're missing something. The driver would probe for a dt snippet
> > like:
> >
> > mxc_nand {
> > compatible = "foobar";
> > }
> >
> > In this case dev->of_node is non-NULL but of_match_device(mxcnd_dt_ids,
> > dev) is.
> >
> > (I didn't actually test this, so there is a chance I'm wrong here. And
> > if not I wonder if it is sensible at all to match the device name on
> > driver name for of-created platform devices.)
>
> Yea, looks like you're right. platform devices check a number of
> things to determine a match, among which is driver name if all else
> fails (platform.c, platform_match()).
Maybe something like this would help to reduce surprises:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f80aaaf9f610..a9fc22c86552 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -840,8 +840,8 @@ static int platform_match(struct device *dev, struct device_driver *drv)
return !strcmp(pdev->driver_override, drv->name);
/* Attempt an OF style match first */
- if (of_driver_match_device(dev, drv))
- return 1;
+ if (pdev->dev.of_node)
+ return of_driver_match_device(dev, drv);
/* Then try ACPI style match */
if (acpi_driver_match_device(dev, drv))
Maybe something similar for acpi devices is desirable, too?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Nov 12, 2015 at 9:53 AM, Uwe Kleine-König
<[email protected]> wrote:
> CC += [email protected], gregkh
You added linux@pengutronix instead of devicetree.
>
> On Thu, Nov 12, 2015 at 09:36:55AM +0100, Frans Klaver wrote:
>> On Thu, Nov 12, 2015 at 9:26 AM, Uwe Kleine-König
>> <[email protected]> wrote:
>> > On Thu, Nov 12, 2015 at 09:03:11AM +0100, Frans Klaver wrote:
>> >> Hi,
>> >>
>> >> On Thu, Nov 12, 2015 at 8:46 AM, LABBE Corentin
>> >> <[email protected]> wrote:
>> >> > of_match_device could return NULL, and so cause a NULL pointer
>> >> > dereference later.
>> >>
>> >> Did you actually run into this? It seems to me that this driver is
>> >> only probed if and only if we have a match and that therefore
>> >> of_match_device will always return a valid pointer (it is using the
>> >> same match table). Am I missing something?
>> >
>> > Yes, you're missing something. The driver would probe for a dt snippet
>> > like:
>> >
>> > mxc_nand {
>> > compatible = "foobar";
>> > }
>> >
>> > In this case dev->of_node is non-NULL but of_match_device(mxcnd_dt_ids,
>> > dev) is.
>> >
>> > (I didn't actually test this, so there is a chance I'm wrong here. And
>> > if not I wonder if it is sensible at all to match the device name on
>> > driver name for of-created platform devices.)
>>
>> Yea, looks like you're right. platform devices check a number of
>> things to determine a match, among which is driver name if all else
>> fails (platform.c, platform_match()).
>
> Maybe something like this would help to reduce surprises:
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index f80aaaf9f610..a9fc22c86552 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -840,8 +840,8 @@ static int platform_match(struct device *dev, struct device_driver *drv)
> return !strcmp(pdev->driver_override, drv->name);
>
> /* Attempt an OF style match first */
> - if (of_driver_match_device(dev, drv))
> - return 1;
> + if (pdev->dev.of_node)
> + return of_driver_match_device(dev, drv);
>
> /* Then try ACPI style match */
> if (acpi_driver_match_device(dev, drv))
That looks sensible, yea. There is a chance that misbehaving DT nodes
will fail after this change, of course.
Thanks,
Frans
On Thu, Nov 12, 2015 at 09:57:07AM +0100, Frans Klaver wrote:
> On Thu, Nov 12, 2015 at 9:53 AM, Uwe Kleine-K?nig
> <[email protected]> wrote:
> > CC += [email protected], gregkh
>
> You added linux@pengutronix instead of devicetree.
Well I substituted Sascha by [email protected] on purpose, but
considered that too unimportant for the outer world :-) But I really
forgot [email protected]. Added now.
> > On Thu, Nov 12, 2015 at 09:36:55AM +0100, Frans Klaver wrote:
> >> On Thu, Nov 12, 2015 at 9:26 AM, Uwe Kleine-K?nig
> >> <[email protected]> wrote:
> >> > On Thu, Nov 12, 2015 at 09:03:11AM +0100, Frans Klaver wrote:
> >> >> Hi,
> >> >>
> >> >> On Thu, Nov 12, 2015 at 8:46 AM, LABBE Corentin
> >> >> <[email protected]> wrote:
> >> >> > of_match_device could return NULL, and so cause a NULL pointer
> >> >> > dereference later.
> >> >>
> >> >> Did you actually run into this? It seems to me that this driver is
> >> >> only probed if and only if we have a match and that therefore
> >> >> of_match_device will always return a valid pointer (it is using the
> >> >> same match table). Am I missing something?
> >> >
> >> > Yes, you're missing something. The driver would probe for a dt snippet
> >> > like:
> >> >
> >> > mxc_nand {
> >> > compatible = "foobar";
> >> > }
> >> >
> >> > In this case dev->of_node is non-NULL but of_match_device(mxcnd_dt_ids,
> >> > dev) is.
> >> >
> >> > (I didn't actually test this, so there is a chance I'm wrong here. And
> >> > if not I wonder if it is sensible at all to match the device name on
> >> > driver name for of-created platform devices.)
> >>
> >> Yea, looks like you're right. platform devices check a number of
> >> things to determine a match, among which is driver name if all else
> >> fails (platform.c, platform_match()).
> >
> > Maybe something like this would help to reduce surprises:
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index f80aaaf9f610..a9fc22c86552 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -840,8 +840,8 @@ static int platform_match(struct device *dev, struct device_driver *drv)
> > return !strcmp(pdev->driver_override, drv->name);
> >
> > /* Attempt an OF style match first */
> > - if (of_driver_match_device(dev, drv))
> > - return 1;
> > + if (pdev->dev.of_node)
> > + return of_driver_match_device(dev, drv);
> >
> > /* Then try ACPI style match */
> > if (acpi_driver_match_device(dev, drv))
>
> That looks sensible, yea. There is a chance that misbehaving DT nodes
> will fail after this change, of course.
Which is ok if this behaviour is considered a misbehave :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Nov 12, 2015 at 09:19:09AM +0100, Uwe Kleine-K?nig wrote:
> Hello Corentin,
>
> On Thu, Nov 12, 2015 at 08:46:55AM +0100, LABBE Corentin wrote:
> > of_match_device could return NULL, and so cause a NULL pointer
> > dereference later.
> >
> > Signed-off-by: LABBE Corentin <[email protected]>
> > ---
> > drivers/mtd/nand/mxc_nand.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > index 136e73a..9e42431 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -1464,8 +1464,7 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
> > {
> > struct device_node *np = host->dev->of_node;
> > struct mxc_nand_platform_data *pdata = &host->pdata;
> > - const struct of_device_id *of_id =
> > - of_match_device(mxcnd_dt_ids, host->dev);
> > + const struct of_device_id *of_id;
> > int buswidth;
> >
> > if (!np)
> > @@ -1482,6 +1481,9 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
> >
> > pdata->width = buswidth / 8;
> >
> > + of_id = of_match_device(mxcnd_dt_ids, host->dev);
> > + if (!of_id)
> > + return -ENODEV;
>
> You should return 1 here instead of -ENODEV. Also this check should
> better be done instead of
>
> if (!np)
> return 1;
>
> at the start of the function.
Are you sure for the "1" value ? It seems a very bad error value.
And I found plenty of case where if (!np) return -Esomething and no example of return 1
Regards
Hello,
On Thu, Nov 12, 2015 at 11:03:28AM +0100, LABBE Corentin wrote:
> On Thu, Nov 12, 2015 at 09:19:09AM +0100, Uwe Kleine-K?nig wrote:
> > Hello Corentin,
> >
> > On Thu, Nov 12, 2015 at 08:46:55AM +0100, LABBE Corentin wrote:
> > > of_match_device could return NULL, and so cause a NULL pointer
> > > dereference later.
> > >
> > > Signed-off-by: LABBE Corentin <[email protected]>
> > > ---
> > > drivers/mtd/nand/mxc_nand.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > > index 136e73a..9e42431 100644
> > > --- a/drivers/mtd/nand/mxc_nand.c
> > > +++ b/drivers/mtd/nand/mxc_nand.c
> > > @@ -1464,8 +1464,7 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
> > > {
> > > struct device_node *np = host->dev->of_node;
> > > struct mxc_nand_platform_data *pdata = &host->pdata;
> > > - const struct of_device_id *of_id =
> > > - of_match_device(mxcnd_dt_ids, host->dev);
> > > + const struct of_device_id *of_id;
> > > int buswidth;
> > >
> > > if (!np)
> > > @@ -1482,6 +1481,9 @@ static int __init mxcnd_probe_dt(struct mxc_nand_host *host)
> > >
> > > pdata->width = buswidth / 8;
> > >
> > > + of_id = of_match_device(mxcnd_dt_ids, host->dev);
> > > + if (!of_id)
> > > + return -ENODEV;
> >
> > You should return 1 here instead of -ENODEV. Also this check should
> > better be done instead of
> >
> > if (!np)
> > return 1;
> >
> > at the start of the function.
>
> Are you sure for the "1" value ? It seems a very bad error value.
> And I found plenty of case where if (!np) return -Esomething and no example of return 1
This is not an error value. The semantic is:
0 everything ok, initialized from dt
1 device was not probed by dt -> fall back to board-file stuff
<0 error that should abort probing
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Nov 12, 2015 at 09:19:09AM +0100, Uwe Kleine-K?nig wrote:
> I really wonder there is no helper
> function like:
>
> #define of_sensible_name(dev) of_match_device(dev->driver->of_match_table, dev)
How about of_device_get_match_data()?
It's not exactly what you asked for, but it gets what you need here.
Brian
Le 16/11/2015 19:33, Brian Norris a ?crit :
> On Thu, Nov 12, 2015 at 09:19:09AM +0100, Uwe Kleine-K?nig wrote:
>> I really wonder there is no helper
>> function like:
>>
>> #define of_sensible_name(dev) of_match_device(dev->driver->of_match_table, dev)
>
> How about of_device_get_match_data()?
>
> It's not exactly what you asked for, but it gets what you need here.
>
> Brian
>
I have got the same comment from another thread.
I will use it. Thanks
Regards