| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20110224100204.GC15118@pulham.picochip.com>
Date: Thu, 24 Feb 2011 10:02:04 +0000
From: Jamie Iles <jamie@...ieiles.com>
To: Wim Van Sebroeck <wim@...ana.be>
Cc: LKML <linux-kernel@...r.kernel.org>,
Linux Watchdog Mailing List <linux-watchdog@...r.kernel.org>,
Alan Cox <alan@...rguk.ukuu.org.uk>
Subject: Re: [RFC] [PATCH 6/10] Generic Watchdog Timer Driver
On Wed, Feb 23, 2011 at 09:43:51PM +0100, Wim Van Sebroeck wrote:
> commit afe3de93859443b575407f39a2e655956c02e088
> Author: Wim Van Sebroeck <wim@...ana.be>
> Date: Sun Jul 18 10:39:00 2010 +0000
>
> watchdog: WatchDog Timer Driver Core - Part 6
>
> Since we don't want that a driver module can be removed
> while the watchdog timer is still active, we lock the
> driver module (by incremeting the reference counter).
> After a clean close of the watchdog driver we unlock the
> driver module.
> If /dev/watchdog is closed uncleanly (and the watchdog is
> still active) then we do not unlock the driver module,
> but keep it, so that next time /dev/watchdog get's opened
> we can continue triggering the watchdog.
Ahh, I was looking for too much in the earlier patches! Apologies.
>
> Signed-off-by: Alan Cox <alan@...rguk.ukuu.org.uk>
> Signed-off-by: Wim Van Sebroeck <wim@...ana.be>
>
> diff --git a/Documentation/watchdog/src/watchdog-with-timer-example.c b/Documentation/watchdog/src/watchdog-with-timer-example.c
> index f1d4f217..604fd91 100644
> --- a/Documentation/watchdog/src/watchdog-with-timer-example.c
> +++ b/Documentation/watchdog/src/watchdog-with-timer-example.c
> @@ -127,6 +127,7 @@ static const struct watchdog_info wdt_info = {
> };
>
> static const struct watchdog_ops wdt_ops = {
> + .owner = THIS_MODULE,
> .start = wdt_start,
> .stop = wdt_stop,
> .ping = wdt_ping,
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index f15e8d4..472bfea 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -63,6 +63,7 @@ It contains following fields:
> The list of watchdog operations is defined as:
>
> struct watchdog_ops {
> + struct module *owner;
> /* mandatory operations */
> int (*start)(struct watchdog_device *);
> int (*stop)(struct watchdog_device *);
> @@ -72,6 +73,10 @@ struct watchdog_ops {
> int (*set_timeout)(struct watchdog_device *, int);
> };
>
> +It is important that you first define the module owner of the watchdog timer
> +driver's operations. This module owner will be used to lock the module when
> +the watchdog is active. (You don't want to unload the module when you're
> +watchdog timer device is still active).
> Some operations are mandatory and some are optional. The mandatory operations
> are:
> * start: this is a pointer to the routine that starts the watchdog timer
> @@ -121,3 +126,8 @@ bit-operations. The status bit's that are defined are:
> * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
> was opened via /dev/watchdog.
> (This bit should only be used by the WatchDog Timer Driver Core).
> +* WDOG_ORPHAN: when /dev/watchdog was closed, but the watchdog is still active
> + then we don't unload the module. This bit is set when this situation occurs.
> + When re-opening /dev/watchdog before a reboot occurs, you can then still use
> + and ping the watchdog timer device.
> + (This bit should only be used by the WatchDog Timer Driver Core).
I don't fully understand why the module refcounting is done in the open
and release though. If you moved it into the registration and
unregistration then doesn't that remove the need for WDOG_ORPHAN?
> diff --git a/drivers/watchdog/core/watchdog_core.c b/drivers/watchdog/core/watchdog_core.c
> index 52bc520..d1a824e 100644
> --- a/drivers/watchdog/core/watchdog_core.c
> +++ b/drivers/watchdog/core/watchdog_core.c
> @@ -60,6 +60,10 @@ int register_watchdogdevice(struct watchdog_device *wdd)
> if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
> return -ENODATA;
>
> + /* Make sure that the owner of the watchdog operations exists */
> + if (wdd->ops->owner == NULL)
> + return -ENODATA;
Won't this be effectively NULL if the module is builtin? It looks like
if it is builtin then THIS_MODULE would be defined as (struct module
*)0.
> +
> /* Make sure that the mandatory operations are supported */
> if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
> return -ENODATA;
> diff --git a/drivers/watchdog/core/watchdog_dev.c b/drivers/watchdog/core/watchdog_dev.c
> index b80c6e6..d3dfac2 100644
> --- a/drivers/watchdog/core/watchdog_dev.c
> +++ b/drivers/watchdog/core/watchdog_dev.c
> @@ -259,7 +259,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>
> static int watchdog_open(struct inode *inode, struct file *file)
> {
> - int err;
> + int err = -EBUSY;
>
> trace("%p, %p", inode, file);
>
> @@ -267,14 +267,26 @@ static int watchdog_open(struct inode *inode, struct file *file)
> if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
> return -EBUSY;
Couldn't wdd be potentially invalid if the module had been unloaded?
>
> + /* if the /dev/watchdog device is open, we don't want the module
> + * to be unloaded. */
> + if (!try_module_get(wdd->ops->owner))
> + goto out;
Same here.
> +
> /* start the watchdog */
> err = watchdog_start(wdd);
> if (err < 0)
> - goto out;
> + goto out_mod;
> +
> + /* We leaked a reference to lock the module in on close
> + * now we can reclaim it as we re-opened before triggering */
> + if (test_and_clear_bit(WDOG_ORPHAN, &wdd->status))
> + module_put(wdd->ops->owner);
>
> /* dev/watchdog is a virtual (and thus non-seekable) filesystem */
> return nonseekable_open(inode, file);
>
> +out_mod:
> + module_put(wdd->ops->owner);
> out:
> clear_bit(WDOG_DEV_OPEN, &wdd->status);
> return err;
> @@ -296,8 +308,13 @@ static int watchdog_release(struct inode *inode, struct file *file)
>
> /* stop the watchdog */
> err = watchdog_stop(wdd);
> - if (err < 0) {
> +
> + /* If the watchdog stopped correctly we let the module unload again */
> + if (err == 0)
> + module_put(wdd->ops->owner);
> + else { /* If not we deliberately leak a module reference */
> printk(KERN_CRIT "%s: not stopping watchdog!\n", wdd->name);
> + set_bit(WDOG_ORPHAN, &wdd->status);
> watchdog_ping(wdd);
> }
>
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index e35f51f..ba08f38 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -64,6 +64,7 @@ struct watchdog_device;
>
> /* The watchdog-devices operations */
> struct watchdog_ops {
> + struct module *owner;
> /* mandatory operations */
> int (*start)(struct watchdog_device *);
> int (*stop)(struct watchdog_device *);
> @@ -84,6 +85,8 @@ struct watchdog_device {
> #define WDOG_ACTIVE 0 /* is the watchdog running/active */
> #define WDOG_DEV_OPEN 1 /* is the watchdog opened via
> * /dev/watchdog */
> +#define WDOG_ORPHAN 2 /* is the device module still loaded
> + * after closing /dev/watchdog */
> };
>
> /* drivers/watchdog/core/watchdog_core.c */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists