| Message ID | 20200813125451.19118-1-madhuparnabhowmik10@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | drivers: watchdog: pc87413_wdt: Fix Race condition bug | expand |
On Thu, Aug 13, 2020 at 06:24:51PM +0530, madhuparnabhowmik10@gmail.com wrote: > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> > > After misc_register the open() callback can be called. > However the base address (swc_base_addr) is set after misc_register() > in init. > As a result, if open callback is called before pc87413_get_swc_base_addr() > then in the following call chain: pc87413_open() -> pc87413_refresh() -> > pc87413_swc_bank3() : The value of swc_base_addr will be -1. > Therefore, do misc_register() after pc87413_get_swc_base_addr(). > > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> Another candidate for removal. The entire driver is at the very least questionable: It hard enables the watchdog after registering it, making it mandatory to open it within one minute or the system will reboot. Also, the driver doesn't even check if the hardware even exists; it just assumes that it does. And then it reconfigures that potentially non-existing hardware to use a specific chip pin as wdt output, as if that would be useful if that pin is not wired up. Worst case that driver may actually break something if it is loaded on an arbitrary system. I really don't see the point of trying to patch it up unless there are users left who can confirm that it even works on existing hardware, and then I'd prefer to have it fixed for good and not just patched up. Thanks, Guenter > --- > drivers/watchdog/pc87413_wdt.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c > index 73fbfc99083b..ad8b8af2bdc0 100644 > --- a/drivers/watchdog/pc87413_wdt.c > +++ b/drivers/watchdog/pc87413_wdt.c > @@ -512,6 +512,10 @@ static int __init pc87413_init(void) > if (ret != 0) > pr_err("cannot register reboot notifier (err=%d)\n", ret); > > + pc87413_select_wdt_out(); > + pc87413_enable_swc(); > + pc87413_get_swc_base_addr(); > + > ret = misc_register(&pc87413_miscdev); > if (ret != 0) { > pr_err("cannot register miscdev on minor=%d (err=%d)\n", > @@ -520,10 +524,6 @@ static int __init pc87413_init(void) > } > pr_info("initialized. timeout=%d min\n", timeout); > > - pc87413_select_wdt_out(); > - pc87413_enable_swc(); > - pc87413_get_swc_base_addr(); > - > if (!request_region(swc_base_addr, 0x20, MODNAME)) { > pr_err("cannot request SWC region at 0x%x\n", swc_base_addr); > ret = -EBUSY;
On Fri, Aug 14, 2020 at 08:07:40AM -0700, Guenter Roeck wrote: > On Thu, Aug 13, 2020 at 06:24:51PM +0530, madhuparnabhowmik10@gmail.com wrote: > > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> > > > > After misc_register the open() callback can be called. > > However the base address (swc_base_addr) is set after misc_register() > > in init. > > As a result, if open callback is called before pc87413_get_swc_base_addr() > > then in the following call chain: pc87413_open() -> pc87413_refresh() -> > > pc87413_swc_bank3() : The value of swc_base_addr will be -1. > > Therefore, do misc_register() after pc87413_get_swc_base_addr(). > > > > Found by Linux Driver Verification project (linuxtesting.org). > > > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> > > Another candidate for removal. The entire driver is at the very least > questionable: It hard enables the watchdog after registering it, making it > mandatory to open it within one minute or the system will reboot. Also, > the driver doesn't even check if the hardware even exists; it just assumes > that it does. And then it reconfigures that potentially non-existing > hardware to use a specific chip pin as wdt output, as if that would be > useful if that pin is not wired up. Worst case that driver may actually > break something if it is loaded on an arbitrary system. > > I really don't see the point of trying to patch it up unless there are users > left who can confirm that it even works on existing hardware, and then I'd > prefer to have it fixed for good and not just patched up. > Sure makes sense. Thank you for reviewing. Regards, Madhuparna > Thanks, > Guenter > > > --- > > drivers/watchdog/pc87413_wdt.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c > > index 73fbfc99083b..ad8b8af2bdc0 100644 > > --- a/drivers/watchdog/pc87413_wdt.c > > +++ b/drivers/watchdog/pc87413_wdt.c > > @@ -512,6 +512,10 @@ static int __init pc87413_init(void) > > if (ret != 0) > > pr_err("cannot register reboot notifier (err=%d)\n", ret); > > > > + pc87413_select_wdt_out(); > > + pc87413_enable_swc(); > > + pc87413_get_swc_base_addr(); > > + > > ret = misc_register(&pc87413_miscdev); > > if (ret != 0) { > > pr_err("cannot register miscdev on minor=%d (err=%d)\n", > > @@ -520,10 +524,6 @@ static int __init pc87413_init(void) > > } > > pr_info("initialized. timeout=%d min\n", timeout); > > > > - pc87413_select_wdt_out(); > > - pc87413_enable_swc(); > > - pc87413_get_swc_base_addr(); > > - > > if (!request_region(swc_base_addr, 0x20, MODNAME)) { > > pr_err("cannot request SWC region at 0x%x\n", swc_base_addr); > > ret = -EBUSY;
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c index 73fbfc99083b..ad8b8af2bdc0 100644 --- a/drivers/watchdog/pc87413_wdt.c +++ b/drivers/watchdog/pc87413_wdt.c @@ -512,6 +512,10 @@ static int __init pc87413_init(void) if (ret != 0) pr_err("cannot register reboot notifier (err=%d)\n", ret); + pc87413_select_wdt_out(); + pc87413_enable_swc(); + pc87413_get_swc_base_addr(); + ret = misc_register(&pc87413_miscdev); if (ret != 0) { pr_err("cannot register miscdev on minor=%d (err=%d)\n", @@ -520,10 +524,6 @@ static int __init pc87413_init(void) } pr_info("initialized. timeout=%d min\n", timeout); - pc87413_select_wdt_out(); - pc87413_enable_swc(); - pc87413_get_swc_base_addr(); - if (!request_region(swc_base_addr, 0x20, MODNAME)) { pr_err("cannot request SWC region at 0x%x\n", swc_base_addr); ret = -EBUSY;