| 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: <57581FB2.10806@mentor.com>
Date: Wed, 8 Jun 2016 16:37:54 +0300
From: Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
To: Guenter Roeck <linux@...ck-us.net>
CC: Wim Van Sebroeck <wim@...ana.be>, Wolfram Sang <wsa@...-dreams.de>,
Robin Gong <b38343@...escale.com>,
<linux-watchdog@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
Hi Guenter,
On 08.06.2016 00:43, Guenter Roeck wrote:
> On Tue, Jun 07, 2016 at 08:38:45PM +0300, Vladimir Zapolskiy wrote:
>> The change adds a simple watchdog pretimeout framework infrastructure,
>> its purpose is to allow users to select a desired handling of watchdog
>> pretimeout events, which may be generated by some watchdog devices.
>>
>> A user selects a default watchdog pretimeout governor during
>> compilation stage.
>>
>> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
>> attributes in sysfs: pretimeout to display currently set pretimeout
>> value and pretimeout_governor attribute to display the selected
>> watchdog pretimeout governor.
>>
>> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
>> sysfs, and such watchdog devices do not require the framework.
>>
>
> One might read "require" as saying that it is mandatory for drivers
> with pretimeout support. That is not what you mean, presumably ?
>
Well, I expressed a negation, formally it does not contradict to the
fact that a WDIOF_PRETIMEOUT capable device/driver can exist without
this code.
If a board has watchdog devices/drivers without WDIOF_PRETIMEOUT
capability (at the moment only kempld_wdt.c explicitly sets it)
the whole framework is dead code, though "do not require" may be too
strict.
But I agree, there is a room for ambiguity, I'll reword the paragraph.
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
>> ---
>> Changes from v2 to v3:
>> * essentially simplified the implementation due to removal of runtime
>> dynamic selection of watchdog pretimeout governors by a user, this
>> feature is supposed to be added later on
>> * removed support of sleepable watchdog pretimeout governors
>> * moved sysfs device attributes to watchdog_dev.c, this required to
>> add exported watchdog_pretimeout_governor_name() interface
>> * if pretimeout framework is not selected, then pretimeout event
>> ends up in kernel panic -- this behaviour as a default one was asked
>> by Guenter
>>
>> Changes from v1 to v2:
>> * removed framework private bits from struct watchdog_governor,
>> * centralized compile-time selection of a default governor in
>> watchdog_pretimeout.h,
>> * added can_sleep option, now only sleeping governors (e.g. userspace)
>> will be executed in a special workqueue,
>> * changed fallback logic, if a governor in use is removed, now this
>> situation is not possible, because in use governors have non-zero
>> module refcount
>>
>> drivers/watchdog/Kconfig | 8 ++
>> drivers/watchdog/Makefile | 5 +-
>> drivers/watchdog/watchdog_core.c | 9 +++
>> drivers/watchdog/watchdog_dev.c | 16 ++++
>> drivers/watchdog/watchdog_pretimeout.c | 134 +++++++++++++++++++++++++++++++++
>> drivers/watchdog/watchdog_pretimeout.h | 42 +++++++++++
>> include/linux/watchdog.h | 13 ++++
>> 7 files changed, 226 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/watchdog/watchdog_pretimeout.c
>> create mode 100644 drivers/watchdog/watchdog_pretimeout.h
>>
> Please document the new API functions.
>
Ok.
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index b54f26c..354217e 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1800,4 +1800,12 @@ config USBPCWATCHDOG
>>
>> Most people will say N.
>>
>> +comment "Watchdog Pretimeout Governors"
>> +
>> +config WATCHDOG_PRETIMEOUT_GOV
>> + bool "Enable watchdog pretimeout governors"
>> + default n
>
> I don't think 'default n" is needed.
>
No strict objections, but probably 'default n' may save quite many
lines in defconfigs.
>> + help
>> + The option allows to select watchdog pretimeout governors.
>> +
>> endif # WATCHDOG
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index a46e7c1..cca47de 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -3,9 +3,12 @@
>> #
>>
>> # The WatchDog Timer Driver Core.
>> -watchdog-objs += watchdog_core.o watchdog_dev.o
>> obj-$(CONFIG_WATCHDOG_CORE) += watchdog.o
>>
>> +watchdog-objs += watchdog_core.o watchdog_dev.o
>> +
>> +watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV) += watchdog_pretimeout.o
>> +
>> # Only one watchdog can succeed. We probe the ISA/PCI/USB based
>> # watchdog-cards first, then the architecture specific watchdog
>> # drivers and then the architecture independent "softdog" driver.
>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
>> index 7c3ba58..ae6c23a 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -40,6 +40,7 @@
>> #include <linux/of.h> /* For of_get_timeout_sec */
>>
>> #include "watchdog_core.h" /* For watchdog_dev_register/... */
>> +#include "watchdog_pretimeout.h"
>>
>> static DEFINE_IDA(watchdog_ida);
>>
>> @@ -244,6 +245,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>> }
>> }
>>
>> + ret = watchdog_register_pretimeout(wdd);
>> + if (ret) {
>> + watchdog_dev_unregister(wdd);
>> + ida_simple_remove(&watchdog_ida, wdd->id);
>> + return ret;
>> + }
>> +
>> if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
>> wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
>>
>> @@ -251,6 +259,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>> if (ret) {
>> pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
>> wdd->id, ret);
>> + watchdog_unregister_pretimeout(wdd);
>> watchdog_dev_unregister(wdd);
>> ida_simple_remove(&watchdog_ida, wdd->id);
>> return ret;
>
> A bit at loss here. Why is it not necessary to unregister the pretimeout
> from __watchdog_unregister_device() ? Doesn't this result in a stale pointer
> and a memory leak ?
This is a bug you found, thank you.
>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>> index 87bbae7..c0fd743 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -49,6 +49,7 @@
>> #include <linux/uaccess.h> /* For copy_to_user/put_user/... */
>>
>> #include "watchdog_core.h"
>> +#include "watchdog_pretimeout.h"
>>
>> /*
>> * struct watchdog_core_data - watchdog core internal data
>> @@ -452,6 +453,16 @@ static ssize_t pretimeout_show(struct device *dev,
>> }
>> static DEVICE_ATTR_RO(pretimeout);
>>
>> +static ssize_t pretimeout_governor_show(struct device *dev,
>> + struct device_attribute *devattr,
>> + char *buf)
>> +{
>> + struct watchdog_device *wdd = dev_get_drvdata(dev);
>> +
>> + return watchdog_pretimeout_governor_name(wdd, buf);
>> +}
>> +static DEVICE_ATTR_RO(pretimeout_governor);
>> +
>> static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
>> int n)
>> {
>> @@ -466,6 +477,10 @@ static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
>> else if (attr == &dev_attr_pretimeout.attr &&
>> !(wdd->info->options & WDIOF_PRETIMEOUT))
>> mode = 0;
>> + else if (attr == &dev_attr_pretimeout_governor.attr &&
>> + !((wdd->info->options & WDIOF_PRETIMEOUT) &&
>> + IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))
>
> This is confusing. How about the following ?
> &&
> (!(wdd->info->options & WDIOF_PRETIMEOUT) ||
> !IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))
>
> Sure, it is mathematically the same, but it would be much easier
> to understand.
No objections, originally I intended to preserve the preexisting style of
attr == &dev_attr_XXX.attr && !(some expression on wdd)
>> + mode = 0;
>>
>> return mode;
>> }
>> @@ -478,6 +493,7 @@ static struct attribute *wdt_attrs[] = {
>> &dev_attr_status.attr,
>> &dev_attr_nowayout.attr,
>> &dev_attr_pretimeout.attr,
>> + &dev_attr_pretimeout_governor.attr,
>> NULL,
>> };
>>
>> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
>> new file mode 100644
>> index 0000000..ebfc3d6
>> --- /dev/null
>> +++ b/drivers/watchdog/watchdog_pretimeout.c
>> @@ -0,0 +1,134 @@
>> +/*
>> + * Copyright (C) 2015-2016 Mentor Graphics
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/list.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/watchdog.h>
>> +
>> +#include "watchdog_pretimeout.h"
>> +
>> +/* Default watchdog pretimeout governor */
>> +static struct watchdog_governor *default_gov;
>> +
>> +/* The spinlock protects wdd->gov and pretimeout_list */
>> +static DEFINE_SPINLOCK(pretimeout_lock);
>> +
>> +/* List of watchdog devices, which can generate a pretimeout event */
>> +static LIST_HEAD(pretimeout_list);
>> +
>> +struct watchdog_pretimeout {
>> + struct watchdog_device *wdd;
>> + struct list_head entry;
>> +};
>> +
>> +int watchdog_pretimeout_governor_name(struct watchdog_device *wdd, char *buf)
>> +{
>> + int count = 0;
>> +
> Initialization seems unnecessary.
>
Correct, will remove it.
>> + spin_lock_irq(&pretimeout_lock);
>> + if (wdd->gov)
>> + count = sprintf(buf, "%s\n", wdd->gov->name);
>> + else
>> + count = sprintf(buf, "N/A\n");
>
> Why not just return an empty string ?
>
This is a matter of interface, whatever clearer for a user is
preferred. I don't have objections against returning an empty string,
I will change it in v4.
>> + spin_unlock_irq(&pretimeout_lock);
>> +
>> + return count;
>> +}
>> +
>> +void watchdog_notify_pretimeout(struct watchdog_device *wdd)
>> +{
>> + unsigned long flags;
>> +
>> + if (!wdd)
>> + return;
>> +
> Why would this function ever be called with NULL wdd ?
>
No reason for that, I believe Postel's law is not applicable here...
>> + spin_lock_irqsave(&pretimeout_lock, flags);
>> + if (!wdd->gov) {
>> + spin_unlock_irqrestore(&pretimeout_lock, flags);
>> + return;
>> + }
>> +
>> + wdd->gov->pretimeout(wdd);
>> + spin_unlock_irqrestore(&pretimeout_lock, flags);
>> +}
>> +EXPORT_SYMBOL_GPL(watchdog_notify_pretimeout);
>> +
>> +int watchdog_register_governor(struct watchdog_governor *gov)
>> +{
>> + struct watchdog_pretimeout *p;
>> +
>> + if (!gov || !gov->name || !gov->pretimeout ||
>> + strlen(gov->name) >= WATCHDOG_GOV_NAME_MAXLEN)
>> + return -EINVAL;
>
> We don't usually do API parameter validation like this. Callers
> are expected to write correct code.
Same as above, I will remove the checks.
>> +
>> + if (default_gov)
>> + return -EBUSY;
>> +
>
> What does this mean ? That only one governor will be accepted ?
>
> Maybe an API documentation will help, but I don't really understand the logic
> here. If only one governor can be registered anyway, why bother keeping more
> than one around ?
Same as above.
>> + spin_lock_irq(&pretimeout_lock);
>> + list_for_each_entry(p, &pretimeout_list, entry)
>> + if (!p->wdd->gov)
>> + p->wdd->gov = gov;
>> + spin_unlock_irq(&pretimeout_lock);
>> +
>> + default_gov = gov;
>> +
>> + return 0;
>> +}
>> +
>> +void watchdog_unregister_governor(struct watchdog_governor *gov)
>> +{
>> + if (!gov)
>> + return;
>> +
> Under what circumstances is this expected to happen ?
>
Same as above.
>> + if (default_gov == gov)
>> + default_gov = NULL;
>
> Why bother ? Registration code would not accept a second registration anyway.
>
> Individual drivers may (and will) still reference default_gov.
>
> With the governors all being boolean, the remove function is pretty much
> useless anyway.
>
> I understand that you plan to add features later, and maybe there is a plan
> to correctly handle more than one governor. But it doesn't make sense to start
> with a broken framework; whatever is introduced should work by itself and not
> rely on it being fixed later.
Yes, that's perfectly correct, I totally agree.
>> +}
>> +
>> +int watchdog_register_pretimeout(struct watchdog_device *wdd)
>> +{
>> + struct watchdog_pretimeout *p;
>> +
>> + if (!(wdd->info->options & WDIOF_PRETIMEOUT))
>> + return 0;
>> +
>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>> + if (!p)
>> + return -ENOMEM;
>> +
>> + spin_lock_irq(&pretimeout_lock);
>> + list_add(&p->entry, &pretimeout_list);
>> + p->wdd = wdd;
>> + wdd->gov = default_gov;
>
> Unless I am missing something, at least per this patch, there is only
> one governor that is ever accepted, which is the first one registered.
> With that, I don't really see the value of keeping a per-device governor
> list around.
This is not a per-device governor list (which actually was present in v2
and removed in v3), the description of this list is found around its
declaration, this is a list of watchdog devices, which can produce
a pretimeout event. Oh, "pretimeout_list" is a confusing naming...
The list can not be safely removed due to the fact that there is no
synchronization between registration of governors and watchdog devices,
at boot time a governor driver may be registered after completed
registration of some watchdog drivers, still these early watchdogs should
get a link to the governor, when it is ready, the list intends to
accumulate all of these registered WDIOF_PRETIMEOUT capable watchdogs.
> After all my comments, I must admit that I am quite at loss, maybe due to
> the lack of documentation. If so, please take my feedback as talking
> points to add to the documentation, to help others understand how this
> framework is supposed to work.
>
Sure, thank you so much for review.
--
Best wishes,
Vladimir
Powered by blists - more mailing lists