linux-kernel - Re: [patch 54/55] timekeeping: Provide fast and NMI safe access to CLOCK_MONOTONIC[

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 for Android: free password hash cracker in your pocket

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

Date:	Sat, 12 Jul 2014 20:33:09 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	John Stultz <john.stultz@...aro.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [patch 54/55] timekeeping: Provide fast and NMI safe access to
 CLOCK_MONOTONIC[_RAW]

----- Original Message -----
> From: "Thomas Gleixner" <tglx@...utronix.de>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@...icios.com>
> Cc: "LKML" <linux-kernel@...r.kernel.org>, "John Stultz" <john.stultz@...aro.org>, "Peter Zijlstra"
> <peterz@...radead.org>, "Steven Rostedt" <rostedt@...dmis.org>
> Sent: Saturday, July 12, 2014 4:04:59 PM
> Subject: Re: [patch 54/55] timekeeping: Provide fast and NMI safe access to CLOCK_MONOTONIC[_RAW]
> 
> On Sat, 12 Jul 2014, Thomas Gleixner wrote:
> > On Sat, 12 Jul 2014, Mathieu Desnoyers wrote:
> > > I'm perhaps missing something here, but what happens with the
> > > following scenario ?
> > > 
> > > Initial conditions:
> > > 
> > > tkf->seq = 0
> > > tkf->base[0] and tkf->base[1] are initialized.
> > > 
> > > CPU 0                                      CPU 1
> > > ------------                               ----------------
> > > update:
> > >   tkf->seq++
> > >   smb_wmb()
> > >   tkf->seq++ (reordered before update)
> > >                                            reader:
> > >                                            seq = tkf->seq (reads 2)
> > >                                            smp_rmb()
> > >                                            idx = seq & 0x01
> > >                                            now = now(tkf->base[idx]
> > >                                            (reads base[0])
> > >   update(tkf->base[0], tk) (racy concurrent update)
> > >                                            smp_rmb()
> > >                                            while (seq != tkf->seq) (they
> > >                                            are equal)
> > > 
> > > So AFAIU, we end up returning a corrupted value. Adding a
> > > smp_wmb() between update of base[0] and increment of seq,
> > > as well as between update of base[1] and the _following_
> > > increment of seq (next update call) would fix this.
> > > 
> > > Thoughts ?
> 
> Second thoughts :)
> 
> > Well, the actual implementation does:
> > 
> > +       /* Force readers off to base[1] */
> > +       raw_write_seqcount_begin(&tkf->seq);
> 
> i.e:
> 	seq++;
> 	smp_wmb();
> 
> > +
> > +       /* Update base[0] */
> > +       base->clock = clk;
> > +       base->cycle_last = clk->cycle_last;
> > +       base->base = tbase;
> > +       base->shift = shift;
> > +       base->mult = mult;
> > +
> > +       /* Force readers back to base[0] */
> > +       raw_write_seqcount_end(&tkf->seq);
> 
> i.e:
> 	smp_wmb();
> 	seq++;
> 
> So while this orders against the update of base0, it does not prevent
> reordering against the update of base1. So you're right, we need a
> 
>   	smp_wmb();
> 
> before we start updating base1.
> 
> > +       /* Update base[1] */
> > +       base++;
> > +       base->clock = clk;
> > +       base->cycle_last = clk->cycle_last;
> > +       base->base = tbase;
> > +       base->shift = shift;
> > +       base->mult = mult;
> 
> So as a consequence we need another one here:
> 
>       	smp_wmb();
> 
> to protect against the unlikely, but possible seq++ at the begin of
> the update. Debatable whether this can happen without another wmb()
> between the two calls, but yes for sanity reasons we should add it
> until we can prove that the actual call chains prevent this.
> 
> Nice catch!

Thanks! Yep, the barriers you propose are what appears
to be missing,

Mathieu

> 
>      tglx
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
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/