2013-04-06 04:24:38

by Guenter Roeck

[permalink] [raw]
Subject: [PATCH] watchdog: Fix race condition in registration code

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 <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
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


2013-04-06 08:19:42

by Arkadiusz Miskiewicz

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

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 <[email protected]>

Please use
Reported-by: Arkadiusz Miśkiewicz <[email protected]>

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 <[email protected]>
> ---
> 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 <[email protected]>

>
> 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