| 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: <52035BA5.1030405@roeck-us.net>
Date: Thu, 08 Aug 2013 01:49:41 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: leroy christophe <christophe.leroy@....fr>
CC: Scott Wood <scottwood@...escale.com>,
Wim Van Sebroeck <wim@...ana.be>,
linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
linux-watchdog@...r.kernel.org
Subject: Re: [v2] Enhanced support for MPC8xx/8xxx watchdog
On 08/07/2013 10:50 PM, leroy christophe wrote:
> Le 26/06/2013 01:04, Scott Wood a écrit :
>> On Thu, Feb 28, 2013 at 09:52:22AM +0100, LEROY Christophe wrote:
>>> This patch modifies the behaviour of the MPC8xx/8xxx watchdog. On the MPC8xx,
>>> at 133Mhz, the maximum timeout of the watchdog timer is 1s, which means it must
>>> be pinged twice a second. This is not in line with the Linux watchdog concept
>>> which is based on a default watchdog timeout around 60s.
>>> This patch introduces an intermediate layer between the CPU and the userspace.
>>> The kernel pings the watchdog at the required frequency at the condition that
>>> userspace tools refresh it regularly.
>>> Existing parameter 'timeout' is renamed 'hw_time'.
>>> The new parameter 'timeout' allows to set up the userspace timeout.
>>> The driver also implements the WDIOC_SETTIMEOUT ioctl.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@....fr>
>>>
>>>
>>> diff -ur linux-3.7.9/drivers/watchdog/mpc8xxx_wdt.c linux/drivers/watchdog/mpc8xxx_wdt.c
>>> --- linux-3.7.9/drivers/watchdog/mpc8xxx_wdt.c 2013-02-17 19:53:32.000000000 +0100
>>> +++ linux/drivers/watchdog/mpc8xxx_wdt.c 2013-02-27 16:00:07.000000000 +0100
>>> @@ -52,10 +52,17 @@
>>> static struct mpc8xxx_wdt __iomem *wd_base;
>>> static int mpc8xxx_wdt_init_late(void);
>>> -static u16 timeout = 0xffff;
>>> -module_param(timeout, ushort, 0);
>>> +#define WD_TIMO 10 /* Default timeout = 10 seconds */
>> If the default Linux watchdog timeout is normally 60 seconds, why is it 10
>> here?
> Looks like each driver has its own default value, but agreed, I change it to 60 seconds
>
>>> +static uint timeout = WD_TIMO;
>>> +module_param(timeout, uint, 0);
>>> MODULE_PARM_DESC(timeout,
>>> - "Watchdog timeout in ticks. (0<timeout<65536, default=65535)");
>>> + "Watchdog SW timeout in seconds. (0 < timeout < 65536s, default = "
>>> + __MODULE_STRING(WD_TIMO) "s)");
>>> +static u16 hw_timo = 0xffff;
>>> +module_param(hw_timo, ushort, 0);
>>> +MODULE_PARM_DESC(hw_timo,
>>> + "Watchdog HW timeout in ticks. (0 < hw_timo < 65536, default = 65535)");
>> hw_timeout would be more legibile -- this is a public interface.
> Ok
>>
>>> static bool reset = 1;
>>> module_param(reset, bool, 0);
>>> @@ -72,10 +79,12 @@
>>> * to 0
>>> */
>>> static int prescale = 1;
>>> -static unsigned int timeout_sec;
>>> +static unsigned int hw_timo_sec;
>>> +static int wdt_auto = 1;
>> bool, and add a comment indicating what this means.
> Ok
>>
>>> static unsigned long wdt_is_open;
>>> static DEFINE_SPINLOCK(wdt_spinlock);
>>> +static unsigned long wdt_last_ping;
>>> static void mpc8xxx_wdt_keepalive(void)
>>> {
>>> @@ -91,9 +100,20 @@
>>> static void mpc8xxx_wdt_timer_ping(unsigned long arg)
>>> {
>>> - mpc8xxx_wdt_keepalive();
>>> - /* We're pinging it twice faster than needed, just to be sure. */
>>> - mod_timer(&wdt_timer, jiffies + HZ * timeout_sec / 2);
>>> + if (wdt_auto)
>>> + wdt_last_ping = jiffies;
>>> +
>>> + if (jiffies - wdt_last_ping <= timeout * HZ) {
>> So timeout cannot be more than UINT_MAX / HZ... Might want to check for
>> that, just in case.
> Ok.
>>
>> What happens if there's a race? If another CPU updates wdt_last_ping in
>> parallel, then you could see wdt_last_ping greater than the value you
Using the watchdog infrastructure (which has a mutex to avoid the problem)
would help avoiding this race.
>> read for jiffies. Since this is an unsigned comparison, it will fail to
>> call keepalive. You might get saved by pinging it twice as often as
>> necessary, but you shouldn't rely on that.
> Euh ... This watchdog is integrated inside the CPU, so there is no chance that any external CPU get access to it.
Unless I am missing something, neither jiffies nor wdt_last_ping nor timeout
is integrated inside a CPU.
There are macros for well defined time comparison which you possibly
might want to consider using (such as time_after() and time_before() etc).
My overall feedback is that I believe it would make more sense to convert
the driver to the watchdog infrastructure first, then add the softdog as
second patch.
Guenter
>>
>>> + mpc8xxx_wdt_keepalive();
>>> + /* We're pinging it twice faster than needed, to be sure. */
>>> + mod_timer(&wdt_timer, jiffies + HZ * hw_timo_sec / 2);
>>> + }
>>> +}
>>> +
>>> +static void mpc8xxx_wdt_sw_keepalive(void)
>>> +{
>>> + wdt_last_ping = jiffies;
>>> + mpc8xxx_wdt_timer_ping(0);
>>> }
>> This isn't new with this patch, but it looks like
>> mpc8xxx_wdt_keepalive() can be called either from timer context, or with
>> interrupts enabled... yet it uses a bare spin_lock() rather than an
>> irq-safe version. This should be fixed.
> Ok, I'll propose another patch for that. Indeed, is the spin_lock needed at all ? If we get two writes interleaved, it will make it anyway.
>
> Christophe
>
> --
> 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