[PATCH] watchdog: Fix race condition in registration code

[PATCH] watchdog: Fix race condition in registration code

[Guenter Roeck]

A race condition exists when registering the first watchdog device.
Sequence of events:

- watchdog_register_device calls watchdog_dev_register
- watchdog_dev_register creates the watchdog misc device by calling
  misc_register.
  At that time, the matching character device (/dev/watchdog0) does not yet
  exist, and old_wdd is not set either.
- Userspace gets an event and opens /dev/watchdog
- watchdog_open is called and sets sets wdd = old_wdd, which is still NULL,
  and tries to dereference it. This causes the kernel to panic.

Seen with systemd trying to open /dev/watchdog immediately after
it was created.

Reported-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Arkadiusz,

would be great if you can test this in your system.

 drivers/watchdog/watchdog_dev.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 08b48bb..faf4e18 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -523,6 +523,7 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
 	int err, devno;
 
 	if (watchdog->id == 0) {
+		old_wdd = watchdog;
 		watchdog_miscdev.parent = watchdog->parent;
 		err = misc_register(&watchdog_miscdev);
 		if (err != 0) {
@@ -531,9 +532,9 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
 			if (err == -EBUSY)
 				pr_err("%s: a legacy watchdog module is probably present.\n",
 					watchdog->info->identity);
+			old_wdd = NULL;
 			return err;
 		}
-		old_wdd = watchdog;
 	}
 
 	/* Fill in the data structures */
-- 
1.7.9.7

Re: [PATCH] watchdog: Fix race condition in registration code

[Arkadiusz Miskiewicz]

On Saturday 06 of April 2013, Guenter Roeck wrote:
> A race condition exists when registering the first watchdog device.
> Sequence of events:
> 
> - watchdog_register_device calls watchdog_dev_register
> - watchdog_dev_register creates the watchdog misc device by calling
>   misc_register.
>   At that time, the matching character device (/dev/watchdog0) does not yet
>   exist, and old_wdd is not set either.
> - Userspace gets an event and opens /dev/watchdog
> - watchdog_open is called and sets sets wdd = old_wdd, which is still NULL,
>   and tries to dereference it. This causes the kernel to panic.
> 
> Seen with systemd trying to open /dev/watchdog immediately after
> it was created.
> 
> Reported-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>

Please use
Reported-by: Arkadiusz Miśkiewicz <arekm@maven.pl>

I have to use gmail address because maven.pl domain is blocked due to some 
unknown, secret reason and vger.kernel.org postmasters (Dave M etc) are less 
than helpful:

"We are under no obligation to explain why you were banned nor to remove
the ban.

If you don't like this, you can run your own list server and on it determine
your own set of policies."


> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Arkadiusz,
> 
> would be great if you can test this in your system.

Did few reboots without oops but this test isn't reliable. Previously I wasn't 
able to reproduce this on demand. It just happens sometime. If any problem 
popup I'll let you know.

So for now
Tested-by: Arkadiusz Miśkiewicz <arekm@maven.pl>

> 
>  drivers/watchdog/watchdog_dev.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c
> b/drivers/watchdog/watchdog_dev.c index 08b48bb..faf4e18 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -523,6 +523,7 @@ int watchdog_dev_register(struct watchdog_device
> *watchdog) int err, devno;
> 
>  	if (watchdog->id == 0) {
> +		old_wdd = watchdog;
>  		watchdog_miscdev.parent = watchdog->parent;
>  		err = misc_register(&watchdog_miscdev);
>  		if (err != 0) {
> @@ -531,9 +532,9 @@ int watchdog_dev_register(struct watchdog_device
> *watchdog) if (err == -EBUSY)
>  				pr_err("%s: a legacy watchdog module is probably present.\n",
>  					watchdog->info->identity);
> +			old_wdd = NULL;
>  			return err;
>  		}
> -		old_wdd = watchdog;
>  	}
> 
>  	/* Fill in the data structures */


-- 
Arkadiusz Miśkiewicz, arekm / maven.pl