watchdog: Fix potential kref imbalance when opening watchdog
Guenter Roeck
maintainerPATCH 1/2 watchdog: Fix potential kref imbalance when opening watchdog
If a watchdog driver's open function sets WDOG_HW_RUNNING with the expectation that the watchdog can not be stopped, but then stops the watchdog anyway in its stop function, kref_get() wil not be called in watchdog_open(). If the watchdog then stops on close, WDOG_HW_RUNNING will be cleared and kref_put() will be called, causing a kref imbalance. As result the character device data structure will be released, which in turn will cause the system to crash on the next call to watchdog_open().
--- drivers/watchdog/watchdog_dev.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/watchdog/watchdog_dev.c +4 -2
@@ -768,6 +768,7 @@ static int watchdog_open(struct inode *inode, struct file *file) {struct watchdog_core_data *wd_data;struct watchdog_device *wdd; +bool hw_running;int err;/* Get the corresponding watchdog device */
@@ -787,7 +788,8 @@ static int watchdog_open(struct inode *inode, struct file *file)* If the /dev/watchdog device is open, we don't want the module * to be unloaded. */ -if (!watchdog_hw_running (wdd) && !try_module_get (wdd->ops->owner)) { + hw_running =watchdog_hw_running (wdd); +if (!hw_running && !try_module_get (wdd->ops->owner)) { err = -EBUSY ; goto out_clear; }
@@ -798,7 +800,7 @@ static int watchdog_open(struct inode *inode, struct file *file) file->private_data = wd_data; -if (!watchdog_hw_running (wdd)) +if (!hw_running)kref_get (&wd_data->kref);/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
Signature · 1 line
2.7.4
Guenter Roeck
maintainerPATCH 2/2 watchdog: Fix kref imbalance seen if handle_boot_enabled=0
If handle_boot_enabled is set to 0, the watchdog driver module use counter will not be increased and kref_get() will not be called when registering the watchdog. Subsequently, on open, this does not happen either because the code believes that it was already done because the hardware watchdog is marked as running.
We could introduce a state variable to indicate this state, but let's just increase the module use counter and call kref_get() unconditionally if the hardware watchdog is running when a driver is registering itself to keep the code simple.
--- drivers/watchdog/watchdog_dev.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
drivers/watchdog/watchdog_dev.c +5 -6
@@ -966,14 +966,13 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)* and schedule an immediate ping. */ if (watchdog_hw_running (wdd)) { -if (handle_boot_enabled) { -__module_get (wdd->ops->owner); -kref_get (&wd_data->kref); +__module_get (wdd->ops->owner); +kref_get (&wd_data->kref); +if (handle_boot_enabled)queue_delayed_work (watchdog_wq, &wd_data->work,0 ); - }else { +else pr_info ("watchdog%d running and kernel based pre-userspace handler disabled\n" , - wdd->id); - } + wdd->id); }return 0 ;
Signature · 1 line
2.7.4
Quoted 7 lines · Guenter Roeck
If a watchdog driver's open function sets
WDOG_HW_RUNNINGwith the expectation that the watchdog can not be stopped, but then stops the watchdog anyway in its stop function,kref_get()wil not be called inwatchdog_open(). If the watchdog then stops on close,WDOG_HW_RUNNINGwill be cleared andkref_put()will be called, causing a kref imbalance. As result the character device data structure will be released, which in turn will cause the system to crash on the next call towatchdog_open().
Ach, ok. I see. So the stop patch should remove it from the start.