On Fri, Nov 06, 2020 at 07:37:08AM +0000, wangwensheng (C) wrote:
> 在 2020/11/5 22:26, Guenter Roeck 写道:
> > On Thu, Nov 05, 2020 at 12:38:47PM +0000, Wang Wensheng wrote:
> >> A reboot notifier, which stops the WDT by calling the stop hook without
> >> any check, would be registered when we set WDOG_STOP_ON_REBOOT flag.
> >>
> >> Howerer we allow the WDT driver to omit the stop hook since commit
> >> "d0684c8a93549" ("watchdog: Make stop function optional") and provide
> >> a module parameter for user that controls the WDOG_STOP_ON_REBOOT flag
> >> in commit 9232c80659e94 ("watchdog: Add stop_on_reboot parameter to
> >> control reboot policy"). Together that commits make user potential to
> >> insert a watchdog driver that don't provide a stop hook but with the
> >> stop_on_reboot parameter set, then dereferencing of null pointer occurs
> >> on system reboot.
> >>
> >> Check the stop hook before registering the reboot notifier to fix the
> >> issue.
> >>
> >> Fixes: d0684c8a9354 ("watchdog: Make stop function optional")
> >> Signed-off-by: Wang Wensheng <wangwensheng4@xxxxxxxxxx>
> >> ---
> >> drivers/watchdog/watchdog_core.c | 9 ++++++++-
> >> 1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> >> index 423844757812..945ab38b14b8 100644
> >> --- a/drivers/watchdog/watchdog_core.c
> >> +++ b/drivers/watchdog/watchdog_core.c
> >> @@ -267,8 +267,15 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
> >> }
> >>
> >> if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
> >> - wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
> >> + if (!wdd->ops->stop) {
> >> + pr_err("watchdog%d: Cannot support stop_on_reboot\n",
> >> + wdd->id);
> >> + watchdog_dev_unregister(wdd);
> >> + ida_simple_remove(&watchdog_ida, id);
> >> + return -EINVAL;
> >> + }
> >
> > The problem with this is that setting the "stop_on_reboot" module parameter
> > would now prevent the watchdog from being loaded, which isn't really
> > desirable and might go unnoticed. I think the initial check should be
> > above, with the "Mandatory operations" check, and
> > if (stop_on_reboot != -1) {
> > should be extended to
> > if (stop_on_reboot != -1 && wdd->ops->stop) {
> >
> > or possibly more fancy:
> >
> > if (stop_on_reboot != -1) {
> > if (stop_on_reboot) {
> > if (!wdd->ops->stop)
> > pr_warn("watchdog%d: stop_on_reboot not supported\n", wdd->id);
> > else
> > set_bit(WDOG_STOP_ON_REBOOT, &wdd->status);
> > } else {
> > clear_bit(WDOG_STOP_ON_REBOOT, &wdd->status);
> > }
> > }
> >
> > Thanks,
> > Guenter
>
> Now the divergence is that should we stop the registering process and
> return error when the STOP_ON_REBOOT flag is set but the driver doesn't
> support it. The flag is set in two scenes.
> Firstly,the driver that should provide the stop hook may set the flag
> staticlly, and it is a bug of the driver if it set the flag but without
> a stop hook. Then giving an error shall be more striking.
> Secondly, the user can change the flag using module parameter. Is it
> reasonable to just ignore the STOP_ON_REBOOT flag and give a warning
> when the user truely want it? And under this circumstance a warning is
> easier to get unnoticed than an error.
> I prefer to stop the registering process and return an error in those
> two scenes.
>
I disagree. A bad module parameter should not result in module load
failures.
Guenter
> Thanks
> >
> >>
> >> + wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
> >> ret = register_reboot_notifier(&wdd->reboot_nb);
> >> if (ret) {
> >> pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
> >> --
> >> 2.25.0
> >>
> >
>
