* [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
@ 2020-04-02 13:52 ` Colin King
0 siblings, 0 replies; 16+ messages in thread
From: Colin King @ 2020-04-02 13:52 UTC (permalink / raw)
To: Alessandro Zummo, Alexandre Belloni, Guenter Roeck, Chris Packham,
linux-rtc
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Currently a failed memory allocation will lead to a null pointer
dereference on point wdt. Fix this by checking for a failed allocation
and adding error return handling to function ds1307_wdt_register.
Also move the error exit label "exit" to allow a return statement to
be removed.
Addresses-Coverity: ("Dereference null return")
Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
V2: move error exit label and remove a return statement, thanks to
Walter Harms for spotting this clean up.
---
drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index fad042118862..c058b02efb4d 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
};
-static void ds1307_wdt_register(struct ds1307 *ds1307)
+static int ds1307_wdt_register(struct ds1307 *ds1307)
{
struct watchdog_device *wdt;
if (ds1307->type != ds_1388)
- return;
+ return 0;
wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
+ if (!wdt)
+ return -ENOMEM;
wdt->info = &ds1388_wdt_info;
wdt->ops = &ds1388_wdt_ops;
@@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
watchdog_init_timeout(wdt, 0, ds1307->dev);
watchdog_set_drvdata(wdt, ds1307);
devm_watchdog_register_device(ds1307->dev, wdt);
+
+ return 0;
}
#else
-static void ds1307_wdt_register(struct ds1307 *ds1307)
+static int ds1307_wdt_register(struct ds1307 *ds1307)
{
+ return 0;
}
#endif /* CONFIG_WATCHDOG_CORE */
@@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
ds1307_hwmon_register(ds1307);
ds1307_clks_register(ds1307);
- ds1307_wdt_register(ds1307);
-
- return 0;
-
+ err = ds1307_wdt_register(ds1307);
exit:
return err;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 13:52 ` Colin King
@ 2020-04-02 14:39 ` Guenter Roeck
-1 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2020-04-02 14:39 UTC (permalink / raw)
To: Colin King, Alessandro Zummo, Alexandre Belloni, Chris Packham,
linux-rtc
Cc: kernel-janitors, linux-kernel
On 4/2/20 6:52 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently a failed memory allocation will lead to a null pointer
> dereference on point wdt. Fix this by checking for a failed allocation
> and adding error return handling to function ds1307_wdt_register.
> Also move the error exit label "exit" to allow a return statement to
> be removed.
>
> Addresses-Coverity: ("Dereference null return")
> Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> V2: move error exit label and remove a return statement, thanks to
> Walter Harms for spotting this clean up.
> ---
> drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index fad042118862..c058b02efb4d 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
>
> };
>
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
What exactly is the point of returning an error just to ignore it ?
Guenter
> {
> struct watchdog_device *wdt;
>
> if (ds1307->type != ds_1388)
> - return;
> + return 0;
>
> wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
>
> wdt->info = &ds1388_wdt_info;
> wdt->ops = &ds1388_wdt_ops;
> @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
> watchdog_init_timeout(wdt, 0, ds1307->dev);
> watchdog_set_drvdata(wdt, ds1307);
> devm_watchdog_register_device(ds1307->dev, wdt);
> +
> + return 0;
> }
> #else
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
> {
> + return 0;
> }
> #endif /* CONFIG_WATCHDOG_CORE */
>
> @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
>
> ds1307_hwmon_register(ds1307);
> ds1307_clks_register(ds1307);
> - ds1307_wdt_register(ds1307);
> -
> - return 0;
> -
> + err = ds1307_wdt_register(ds1307);
> exit:
> return err;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
@ 2020-04-02 14:39 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2020-04-02 14:39 UTC (permalink / raw)
To: Colin King, Alessandro Zummo, Alexandre Belloni, Chris Packham,
linux-rtc
Cc: kernel-janitors, linux-kernel
On 4/2/20 6:52 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently a failed memory allocation will lead to a null pointer
> dereference on point wdt. Fix this by checking for a failed allocation
> and adding error return handling to function ds1307_wdt_register.
> Also move the error exit label "exit" to allow a return statement to
> be removed.
>
> Addresses-Coverity: ("Dereference null return")
> Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> V2: move error exit label and remove a return statement, thanks to
> Walter Harms for spotting this clean up.
> ---
> drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index fad042118862..c058b02efb4d 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
>
> };
>
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
What exactly is the point of returning an error just to ignore it ?
Guenter
> {
> struct watchdog_device *wdt;
>
> if (ds1307->type != ds_1388)
> - return;
> + return 0;
>
> wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
>
> wdt->info = &ds1388_wdt_info;
> wdt->ops = &ds1388_wdt_ops;
> @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
> watchdog_init_timeout(wdt, 0, ds1307->dev);
> watchdog_set_drvdata(wdt, ds1307);
> devm_watchdog_register_device(ds1307->dev, wdt);
> +
> + return 0;
> }
> #else
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
> {
> + return 0;
> }
> #endif /* CONFIG_WATCHDOG_CORE */
>
> @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
>
> ds1307_hwmon_register(ds1307);
> ds1307_clks_register(ds1307);
> - ds1307_wdt_register(ds1307);
> -
> - return 0;
> -
> + err = ds1307_wdt_register(ds1307);
> exit:
> return err;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 13:52 ` Colin King
@ 2020-04-02 14:44 ` Guenter Roeck
-1 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2020-04-02 14:44 UTC (permalink / raw)
To: Colin King, Alessandro Zummo, Alexandre Belloni, Chris Packham,
linux-rtc
Cc: kernel-janitors, linux-kernel
On 4/2/20 6:52 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently a failed memory allocation will lead to a null pointer
> dereference on point wdt. Fix this by checking for a failed allocation
> and adding error return handling to function ds1307_wdt_register.
> Also move the error exit label "exit" to allow a return statement to
> be removed.
>
> Addresses-Coverity: ("Dereference null return")
> Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> V2: move error exit label and remove a return statement, thanks to
> Walter Harms for spotting this clean up.
> ---
> drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index fad042118862..c058b02efb4d 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
>
> };
>
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
> {
> struct watchdog_device *wdt;
>
> if (ds1307->type != ds_1388)
> - return;
> + return 0;
>
> wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
>
> wdt->info = &ds1388_wdt_info;
> wdt->ops = &ds1388_wdt_ops;
> @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
> watchdog_init_timeout(wdt, 0, ds1307->dev);
> watchdog_set_drvdata(wdt, ds1307);
> devm_watchdog_register_device(ds1307->dev, wdt);
> +
> + return 0;
> }
> #else
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
> {
> + return 0;
> }
> #endif /* CONFIG_WATCHDOG_CORE */
>
> @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
>
> ds1307_hwmon_register(ds1307);
> ds1307_clks_register(ds1307);
> - ds1307_wdt_register(ds1307);
> -
> - return 0;
> -
> + err = ds1307_wdt_register(ds1307);
Ah, sorry, missed this one. The original idea was to ignore errors on purpose.
Same as with hwmon. If you want to change this, fine with me. Note though
that rtc_nvmem_register() now leaks a sysfs file if I understand the code
correctly.
Guenter
> exit:
> return err;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
@ 2020-04-02 14:44 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2020-04-02 14:44 UTC (permalink / raw)
To: Colin King, Alessandro Zummo, Alexandre Belloni, Chris Packham,
linux-rtc
Cc: kernel-janitors, linux-kernel
On 4/2/20 6:52 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently a failed memory allocation will lead to a null pointer
> dereference on point wdt. Fix this by checking for a failed allocation
> and adding error return handling to function ds1307_wdt_register.
> Also move the error exit label "exit" to allow a return statement to
> be removed.
>
> Addresses-Coverity: ("Dereference null return")
> Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> V2: move error exit label and remove a return statement, thanks to
> Walter Harms for spotting this clean up.
> ---
> drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index fad042118862..c058b02efb4d 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
>
> };
>
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
> {
> struct watchdog_device *wdt;
>
> if (ds1307->type != ds_1388)
> - return;
> + return 0;
>
> wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
>
> wdt->info = &ds1388_wdt_info;
> wdt->ops = &ds1388_wdt_ops;
> @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
> watchdog_init_timeout(wdt, 0, ds1307->dev);
> watchdog_set_drvdata(wdt, ds1307);
> devm_watchdog_register_device(ds1307->dev, wdt);
> +
> + return 0;
> }
> #else
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
> {
> + return 0;
> }
> #endif /* CONFIG_WATCHDOG_CORE */
>
> @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
>
> ds1307_hwmon_register(ds1307);
> ds1307_clks_register(ds1307);
> - ds1307_wdt_register(ds1307);
> -
> - return 0;
> -
> + err = ds1307_wdt_register(ds1307);
Ah, sorry, missed this one. The original idea was to ignore errors on purpose.
Same as with hwmon. If you want to change this, fine with me. Note though
that rtc_nvmem_register() now leaks a sysfs file if I understand the code
correctly.
Guenter
> exit:
> return err;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 14:44 ` Guenter Roeck
@ 2020-04-02 14:53 ` Alexandre Belloni
-1 siblings, 0 replies; 16+ messages in thread
From: Alexandre Belloni @ 2020-04-02 14:53 UTC (permalink / raw)
To: Guenter Roeck
Cc: Colin King, Alessandro Zummo, Chris Packham, linux-rtc,
kernel-janitors, linux-kernel
On 02/04/2020 07:44:44-0700, Guenter Roeck wrote:
> On 4/2/20 6:52 AM, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > Currently a failed memory allocation will lead to a null pointer
> > dereference on point wdt. Fix this by checking for a failed allocation
> > and adding error return handling to function ds1307_wdt_register.
> > Also move the error exit label "exit" to allow a return statement to
> > be removed.
> >
> > Addresses-Coverity: ("Dereference null return")
> > Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > V2: move error exit label and remove a return statement, thanks to
> > Walter Harms for spotting this clean up.
> > ---
> > drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> > index fad042118862..c058b02efb4d 100644
> > --- a/drivers/rtc/rtc-ds1307.c
> > +++ b/drivers/rtc/rtc-ds1307.c
> > @@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
> >
> > };
> >
> > -static void ds1307_wdt_register(struct ds1307 *ds1307)
> > +static int ds1307_wdt_register(struct ds1307 *ds1307)
> > {
> > struct watchdog_device *wdt;
> >
> > if (ds1307->type != ds_1388)
> > - return;
> > + return 0;
> >
> > wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> > + if (!wdt)
> > + return -ENOMEM;
> >
> > wdt->info = &ds1388_wdt_info;
> > wdt->ops = &ds1388_wdt_ops;
> > @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
> > watchdog_init_timeout(wdt, 0, ds1307->dev);
> > watchdog_set_drvdata(wdt, ds1307);
> > devm_watchdog_register_device(ds1307->dev, wdt);
> > +
> > + return 0;
> > }
> > #else
> > -static void ds1307_wdt_register(struct ds1307 *ds1307)
> > +static int ds1307_wdt_register(struct ds1307 *ds1307)
> > {
> > + return 0;
> > }
> > #endif /* CONFIG_WATCHDOG_CORE */
> >
> > @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
> >
> > ds1307_hwmon_register(ds1307);
> > ds1307_clks_register(ds1307);
> > - ds1307_wdt_register(ds1307);
> > -
> > - return 0;
> > -
> > + err = ds1307_wdt_register(ds1307);
>
> Ah, sorry, missed this one. The original idea was to ignore errors on purpose.
> Same as with hwmon. If you want to change this, fine with me. Note though
> that rtc_nvmem_register() now leaks a sysfs file if I understand the code
> correctly.
rtc_nvmem_unregister(rtc) should be called properly by
devm_rtc_release_device when the rtc_device is destroyed so I don't
think it leaks.
As stated, I also prefer keeping the watchdog optional and ignore the
error.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
@ 2020-04-02 14:53 ` Alexandre Belloni
0 siblings, 0 replies; 16+ messages in thread
From: Alexandre Belloni @ 2020-04-02 14:53 UTC (permalink / raw)
To: Guenter Roeck
Cc: Colin King, Alessandro Zummo, Chris Packham, linux-rtc,
kernel-janitors, linux-kernel
On 02/04/2020 07:44:44-0700, Guenter Roeck wrote:
> On 4/2/20 6:52 AM, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > Currently a failed memory allocation will lead to a null pointer
> > dereference on point wdt. Fix this by checking for a failed allocation
> > and adding error return handling to function ds1307_wdt_register.
> > Also move the error exit label "exit" to allow a return statement to
> > be removed.
> >
> > Addresses-Coverity: ("Dereference null return")
> > Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > V2: move error exit label and remove a return statement, thanks to
> > Walter Harms for spotting this clean up.
> > ---
> > drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> > index fad042118862..c058b02efb4d 100644
> > --- a/drivers/rtc/rtc-ds1307.c
> > +++ b/drivers/rtc/rtc-ds1307.c
> > @@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
> >
> > };
> >
> > -static void ds1307_wdt_register(struct ds1307 *ds1307)
> > +static int ds1307_wdt_register(struct ds1307 *ds1307)
> > {
> > struct watchdog_device *wdt;
> >
> > if (ds1307->type != ds_1388)
> > - return;
> > + return 0;
> >
> > wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> > + if (!wdt)
> > + return -ENOMEM;
> >
> > wdt->info = &ds1388_wdt_info;
> > wdt->ops = &ds1388_wdt_ops;
> > @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
> > watchdog_init_timeout(wdt, 0, ds1307->dev);
> > watchdog_set_drvdata(wdt, ds1307);
> > devm_watchdog_register_device(ds1307->dev, wdt);
> > +
> > + return 0;
> > }
> > #else
> > -static void ds1307_wdt_register(struct ds1307 *ds1307)
> > +static int ds1307_wdt_register(struct ds1307 *ds1307)
> > {
> > + return 0;
> > }
> > #endif /* CONFIG_WATCHDOG_CORE */
> >
> > @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
> >
> > ds1307_hwmon_register(ds1307);
> > ds1307_clks_register(ds1307);
> > - ds1307_wdt_register(ds1307);
> > -
> > - return 0;
> > -
> > + err = ds1307_wdt_register(ds1307);
>
> Ah, sorry, missed this one. The original idea was to ignore errors on purpose.
> Same as with hwmon. If you want to change this, fine with me. Note though
> that rtc_nvmem_register() now leaks a sysfs file if I understand the code
> correctly.
rtc_nvmem_unregister(rtc) should be called properly by
devm_rtc_release_device when the rtc_device is destroyed so I don't
think it leaks.
As stated, I also prefer keeping the watchdog optional and ignore the
error.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 14:53 ` Alexandre Belloni
@ 2020-04-03 8:45 ` Dan Carpenter
-1 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2020-04-03 8:45 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Guenter Roeck, Colin King, Alessandro Zummo, Chris Packham,
linux-rtc, kernel-janitors, linux-kernel
On Thu, Apr 02, 2020 at 04:53:12PM +0200, Alexandre Belloni wrote:
>
> As stated, I also prefer keeping the watchdog optional and ignore the
> error.
Hopefully you aren't running out of memory on start up. In current
kernels small memory allocations like this never fail so it doesn't
really affect runtime. It only silences the NULL dereference static
checker warning.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
@ 2020-04-03 8:45 ` Dan Carpenter
0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2020-04-03 8:45 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Guenter Roeck, Colin King, Alessandro Zummo, Chris Packham,
linux-rtc, kernel-janitors, linux-kernel
On Thu, Apr 02, 2020 at 04:53:12PM +0200, Alexandre Belloni wrote:
>
> As stated, I also prefer keeping the watchdog optional and ignore the
> error.
Hopefully you aren't running out of memory on start up. In current
kernels small memory allocations like this never fail so it doesn't
really affect runtime. It only silences the NULL dereference static
checker warning.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-03 8:45 ` Dan Carpenter
@ 2020-04-03 9:22 ` Alexandre Belloni
-1 siblings, 0 replies; 16+ messages in thread
From: Alexandre Belloni @ 2020-04-03 9:22 UTC (permalink / raw)
To: Dan Carpenter
Cc: Guenter Roeck, Colin King, Alessandro Zummo, Chris Packham,
linux-rtc, kernel-janitors, linux-kernel
On 03/04/2020 11:45:04+0300, Dan Carpenter wrote:
> On Thu, Apr 02, 2020 at 04:53:12PM +0200, Alexandre Belloni wrote:
> >
> > As stated, I also prefer keeping the watchdog optional and ignore the
> > error.
>
> Hopefully you aren't running out of memory on start up. In current
> kernels small memory allocations like this never fail so it doesn't
> really affect runtime. It only silences the NULL dereference static
> checker warning.
>
Yes, so the
if (!wdt)
return;
would be enough instead of introducing unnecessary error handling.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
@ 2020-04-03 9:22 ` Alexandre Belloni
0 siblings, 0 replies; 16+ messages in thread
From: Alexandre Belloni @ 2020-04-03 9:22 UTC (permalink / raw)
To: Dan Carpenter
Cc: Guenter Roeck, Colin King, Alessandro Zummo, Chris Packham,
linux-rtc, kernel-janitors, linux-kernel
On 03/04/2020 11:45:04+0300, Dan Carpenter wrote:
> On Thu, Apr 02, 2020 at 04:53:12PM +0200, Alexandre Belloni wrote:
> >
> > As stated, I also prefer keeping the watchdog optional and ignore the
> > error.
>
> Hopefully you aren't running out of memory on start up. In current
> kernels small memory allocations like this never fail so it doesn't
> really affect runtime. It only silences the NULL dereference static
> checker warning.
>
Yes, so the
if (!wdt)
return;
would be enough instead of introducing unnecessary error handling.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-03 9:22 ` Alexandre Belloni
@ 2020-04-03 10:01 ` Colin Ian King
-1 siblings, 0 replies; 16+ messages in thread
From: Colin Ian King @ 2020-04-03 10:01 UTC (permalink / raw)
To: Alexandre Belloni, Dan Carpenter
Cc: Guenter Roeck, Alessandro Zummo, Chris Packham, linux-rtc,
kernel-janitors, linux-kernel
On 03/04/2020 10:22, Alexandre Belloni wrote:
> On 03/04/2020 11:45:04+0300, Dan Carpenter wrote:
>> On Thu, Apr 02, 2020 at 04:53:12PM +0200, Alexandre Belloni wrote:
>>>
>>> As stated, I also prefer keeping the watchdog optional and ignore the
>>> error.
>>
>> Hopefully you aren't running out of memory on start up. In current
>> kernels small memory allocations like this never fail so it doesn't
>> really affect runtime. It only silences the NULL dereference static
>> checker warning.
>>
>
> Yes, so the
>
> if (!wdt)
> return;
>
> would be enough instead of introducing unnecessary error handling.
>
OK, I'll send a V3 later today.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
@ 2020-04-03 10:01 ` Colin Ian King
0 siblings, 0 replies; 16+ messages in thread
From: Colin Ian King @ 2020-04-03 10:01 UTC (permalink / raw)
To: Alexandre Belloni, Dan Carpenter
Cc: Guenter Roeck, Alessandro Zummo, Chris Packham, linux-rtc,
kernel-janitors, linux-kernel
On 03/04/2020 10:22, Alexandre Belloni wrote:
> On 03/04/2020 11:45:04+0300, Dan Carpenter wrote:
>> On Thu, Apr 02, 2020 at 04:53:12PM +0200, Alexandre Belloni wrote:
>>>
>>> As stated, I also prefer keeping the watchdog optional and ignore the
>>> error.
>>
>> Hopefully you aren't running out of memory on start up. In current
>> kernels small memory allocations like this never fail so it doesn't
>> really affect runtime. It only silences the NULL dereference static
>> checker warning.
>>
>
> Yes, so the
>
> if (!wdt)
> return;
>
> would be enough instead of introducing unnecessary error handling.
>
OK, I'll send a V3 later today.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 13:52 ` Colin King
@ 2020-04-02 19:51 ` Chris Packham
-1 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2020-04-02 19:51 UTC (permalink / raw)
To: linux@roeck-us.net, colin.king@canonical.com,
a.zummo@towertech.it, linux-rtc@vger.kernel.org,
alexandre.belloni@bootlin.com
Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
T24gVGh1LCAyMDIwLTA0LTAyIGF0IDE0OjUyICswMTAwLCBDb2xpbiBLaW5nIHdyb3RlOg0KPiBG
cm9tOiBDb2xpbiBJYW4gS2luZyA8Y29saW4ua2luZ0BjYW5vbmljYWwuY29tPg0KPiANCj4gQ3Vy
cmVudGx5IGEgZmFpbGVkIG1lbW9yeSBhbGxvY2F0aW9uIHdpbGwgbGVhZCB0byBhIG51bGwgcG9p
bnRlcg0KPiBkZXJlZmVyZW5jZSBvbiBwb2ludCB3ZHQuICBGaXggdGhpcyBieSBjaGVja2luZyBm
b3IgYSBmYWlsZWQNCj4gYWxsb2NhdGlvbg0KPiBhbmQgYWRkaW5nIGVycm9yIHJldHVybiBoYW5k
bGluZyB0byBmdW5jdGlvbiBkczEzMDdfd2R0X3JlZ2lzdGVyLg0KPiBBbHNvIG1vdmUgdGhlIGVy
cm9yIGV4aXQgbGFiZWwgImV4aXQiIHRvIGFsbG93IGEgcmV0dXJuIHN0YXRlbWVudCB0bw0KPiBi
ZSByZW1vdmVkLg0KPiANCj4gQWRkcmVzc2VzLUNvdmVyaXR5OiAoIkRlcmVmZXJlbmNlIG51bGwg
cmV0dXJuIikNCj4gRml4ZXM6IGZkOTBkNDhkYjAzNyAoInJ0YzogZHMxMzA3OiBhZGQgc3VwcG9y
dCBmb3Igd2F0Y2hkb2cgdGltZXIgb24NCj4gZHMxMzg4IikNCj4gU2lnbmVkLW9mZi1ieTogQ29s
aW4gSWFuIEtpbmcgPGNvbGluLmtpbmdAY2Fub25pY2FsLmNvbT4NCj4gLS0tDQo+IFYyOiBtb3Zl
IGVycm9yIGV4aXQgbGFiZWwgYW5kIHJlbW92ZSBhIHJldHVybiBzdGF0ZW1lbnQsIHRoYW5rcyB0
byANCj4gICAgIFdhbHRlciBIYXJtcyBmb3Igc3BvdHRpbmcgdGhpcyBjbGVhbiB1cC4NCj4gLS0t
DQo+ICBkcml2ZXJzL3J0Yy9ydGMtZHMxMzA3LmMgfCAxNiArKysrKysrKystLS0tLS0tDQo+ICAx
IGZpbGUgY2hhbmdlZCwgOSBpbnNlcnRpb25zKCspLCA3IGRlbGV0aW9ucygtKQ0KPiANCj4gZGlm
ZiAtLWdpdCBhL2RyaXZlcnMvcnRjL3J0Yy1kczEzMDcuYyBiL2RyaXZlcnMvcnRjL3J0Yy1kczEz
MDcuYw0KPiBpbmRleCBmYWQwNDIxMTg4NjIuLmMwNThiMDJlZmI0ZCAxMDA2NDQNCj4gLS0tIGEv
ZHJpdmVycy9ydGMvcnRjLWRzMTMwNy5jDQo+ICsrKyBiL2RyaXZlcnMvcnRjL3J0Yy1kczEzMDcu
Yw0KPiBAQCAtMTY2NSwxNCArMTY2NSwxNiBAQCBzdGF0aWMgY29uc3Qgc3RydWN0IHdhdGNoZG9n
X29wcw0KPiBkczEzODhfd2R0X29wcyA9IHsNCj4gIA0KPiAgfTsNCj4gIA0KPiAtc3RhdGljIHZv
aWQgZHMxMzA3X3dkdF9yZWdpc3RlcihzdHJ1Y3QgZHMxMzA3ICpkczEzMDcpDQo+ICtzdGF0aWMg
aW50IGRzMTMwN193ZHRfcmVnaXN0ZXIoc3RydWN0IGRzMTMwNyAqZHMxMzA3KQ0KPiAgew0KPiAg
CXN0cnVjdCB3YXRjaGRvZ19kZXZpY2UJKndkdDsNCj4gIA0KPiAgCWlmIChkczEzMDctPnR5cGUg
IT0gZHNfMTM4OCkNCj4gLQkJcmV0dXJuOw0KPiArCQlyZXR1cm4gMDsNCj4gIA0KPiAgCXdkdCA9
IGRldm1fa3phbGxvYyhkczEzMDctPmRldiwgc2l6ZW9mKCp3ZHQpLCBHRlBfS0VSTkVMKTsNCj4g
KwlpZiAoIXdkdCkNCj4gKwkJcmV0dXJuIC1FTk9NRU07DQoNCk15IG9yaWdpbmFsIGludGVudGlv
biB3YXMgdGhhdCB0aGUgd2R0IHN1cHBvcnQgd2FzIG9wdGlvbmFsLiBJJ2QNCnN1Z2dlc3QganVz
dA0KDQorCWlmICghd2R0KQ0KKwkJcmV0dXJuOw0KDQpXaGljaCBzaG91bGQga2VlcCBDb3Zlcml0
eSBoYXBweS4NCg0KPiAgCXdkdC0+aW5mbyA9ICZkczEzODhfd2R0X2luZm87DQo+ICAJd2R0LT5v
cHMgPSAmZHMxMzg4X3dkdF9vcHM7DQo+IEBAIC0xNjgzLDEwICsxNjg1LDEzIEBAIHN0YXRpYyB2
b2lkIGRzMTMwN193ZHRfcmVnaXN0ZXIoc3RydWN0IGRzMTMwNw0KPiAqZHMxMzA3KQ0KPiAgCXdh
dGNoZG9nX2luaXRfdGltZW91dCh3ZHQsIDAsIGRzMTMwNy0+ZGV2KTsNCj4gIAl3YXRjaGRvZ19z
ZXRfZHJ2ZGF0YSh3ZHQsIGRzMTMwNyk7DQo+ICAJZGV2bV93YXRjaGRvZ19yZWdpc3Rlcl9kZXZp
Y2UoZHMxMzA3LT5kZXYsIHdkdCk7DQo+ICsNCj4gKwlyZXR1cm4gMDsNCj4gIH0NCj4gICNlbHNl
DQo+IC1zdGF0aWMgdm9pZCBkczEzMDdfd2R0X3JlZ2lzdGVyKHN0cnVjdCBkczEzMDcgKmRzMTMw
NykNCj4gK3N0YXRpYyBpbnQgZHMxMzA3X3dkdF9yZWdpc3RlcihzdHJ1Y3QgZHMxMzA3ICpkczEz
MDcpDQo+ICB7DQo+ICsJcmV0dXJuIDA7DQo+ICB9DQo+ICAjZW5kaWYgLyogQ09ORklHX1dBVENI
RE9HX0NPUkUgKi8NCj4gIA0KPiBAQCAtMTk3OSwxMCArMTk4NCw3IEBAIHN0YXRpYyBpbnQgZHMx
MzA3X3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50DQo+ICpjbGllbnQsDQo+ICANCj4gIAlkczEzMDdf
aHdtb25fcmVnaXN0ZXIoZHMxMzA3KTsNCj4gIAlkczEzMDdfY2xrc19yZWdpc3RlcihkczEzMDcp
Ow0KPiAtCWRzMTMwN193ZHRfcmVnaXN0ZXIoZHMxMzA3KTsNCj4gLQ0KPiAtCXJldHVybiAwOw0K
PiAtDQo+ICsJZXJyID0gZHMxMzA3X3dkdF9yZWdpc3RlcihkczEzMDcpOw0KPiAgZXhpdDoNCj4g
IAlyZXR1cm4gZXJyOw0KPiAgfQ0K
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
@ 2020-04-02 19:51 ` Chris Packham
0 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2020-04-02 19:51 UTC (permalink / raw)
To: linux@roeck-us.net, colin.king@canonical.com,
a.zummo@towertech.it, linux-rtc@vger.kernel.org,
alexandre.belloni@bootlin.com
Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
On Thu, 2020-04-02 at 14:52 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently a failed memory allocation will lead to a null pointer
> dereference on point wdt. Fix this by checking for a failed
> allocation
> and adding error return handling to function ds1307_wdt_register.
> Also move the error exit label "exit" to allow a return statement to
> be removed.
>
> Addresses-Coverity: ("Dereference null return")
> Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on
> ds1388")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> V2: move error exit label and remove a return statement, thanks to
> Walter Harms for spotting this clean up.
> ---
> drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index fad042118862..c058b02efb4d 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -1665,14 +1665,16 @@ static const struct watchdog_ops
> ds1388_wdt_ops = {
>
> };
>
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
> {
> struct watchdog_device *wdt;
>
> if (ds1307->type != ds_1388)
> - return;
> + return 0;
>
> wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
My original intention was that the wdt support was optional. I'd
suggest just
+ if (!wdt)
+ return;
Which should keep Coverity happy.
> wdt->info = &ds1388_wdt_info;
> wdt->ops = &ds1388_wdt_ops;
> @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307
> *ds1307)
> watchdog_init_timeout(wdt, 0, ds1307->dev);
> watchdog_set_drvdata(wdt, ds1307);
> devm_watchdog_register_device(ds1307->dev, wdt);
> +
> + return 0;
> }
> #else
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
> {
> + return 0;
> }
> #endif /* CONFIG_WATCHDOG_CORE */
>
> @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client
> *client,
>
> ds1307_hwmon_register(ds1307);
> ds1307_clks_register(ds1307);
> - ds1307_wdt_register(ds1307);
> -
> - return 0;
> -
> + err = ds1307_wdt_register(ds1307);
> exit:
> return err;
> }
^ permalink raw reply [flat|nested] 16+ messages in thread