linux-kernel - Re: [v2] Enhanced support for MPC8xx/8xxx watchdog

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1376092951.15633.93.camel@snotra.buserror.net>
Date:	Fri, 9 Aug 2013 19:02:31 -0500
From:	Scott Wood <scottwood@...escale.com>
To:	leroy christophe <christophe.leroy@....fr>
CC:	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 Thu, 2013-08-08 at 07:50 +0200, leroy christophe wrote:
> Le 26/06/2013 01:04, Scott Wood a écrit :
> > 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
> > 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.

Hmm, it looks like mpc8641d (which is the only multi-core SoC among mpc8xx/mpc83xx/mpc86xx) does not have this watchdog, even though mpc8610 does.

So pretend I said "what if you get preempted?". :-)

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

I suppose...  I don't like relying on things like that, though.

-Scott



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