linux-kernel - Re: [tip:x86/urgent] x86/mm: Add barriers and document switch

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 Jan 2016 11:21:05 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Rik van Riel <riel@...hat.com>,
	Brian Gerst <brgerst@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Borislav Petkov <bp@...en8.de>,
	Andrew Lutomirski <luto@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"linux-tip-commits@...r.kernel.org" 
	<linux-tip-commits@...r.kernel.org>
Subject: Re: [tip:x86/urgent] x86/mm: Add barriers and document switch_mm()
 -vs-flush synchronization


* Andy Lutomirski <luto@...capital.net> wrote:

> On Mon, Jan 11, 2016 at 10:25 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> > On Mon, Jan 11, 2016 at 03:42:40AM -0800, tip-bot for Andy Lutomirski wrote:
> >> --- a/arch/x86/include/asm/mmu_context.h
> >> +++ b/arch/x86/include/asm/mmu_context.h
> >> @@ -116,8 +116,34 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
> >>  #endif
> >>               cpumask_set_cpu(cpu, mm_cpumask(next));
> >>
> >> -             /* Re-load page tables */
> >> +             /*
> >> +              * Re-load page tables.
> >> +              *
> >> +              * This logic has an ordering constraint:
> >> +              *
> >> +              *  CPU 0: Write to a PTE for 'next'
> >> +              *  CPU 0: load bit 1 in mm_cpumask.  if nonzero, send IPI.
> >> +              *  CPU 1: set bit 1 in next's mm_cpumask
> >> +              *  CPU 1: load from the PTE that CPU 0 writes (implicit)
> >> +              *
> >> +              * We need to prevent an outcome in which CPU 1 observes
> >> +              * the new PTE value and CPU 0 observes bit 1 clear in
> >> +              * mm_cpumask.  (If that occurs, then the IPI will never
> >> +              * be sent, and CPU 0's TLB will contain a stale entry.)
> >> +              *
> >> +              * The bad outcome can occur if either CPU's load is
> >> +              * reordered before that CPU's store, so both CPUs much
> >
> > s/much/must/ ?
> 
> Indeed.  Is this worth a follow-up patch?

Absolutely! Any typos in code noticed by humans are worth fixing, especially when 
it's comments around tricky code. Could be done together with improving this part 
of the comments:

> > +
> >                       /*
> >                        * We were in lazy tlb mode and leave_mm disabled
> >                        * tlb flush IPI delivery. We must reload CR3
> >                        * to make sure to use no freed page tables.
> > +                      *
> > +                      * As above, this is a barrier that forces
> > +                      * TLB repopulation to be ordered after the
> > +                      * store to mm_cpumask.
>
> somewhat confused by this comment, cpumask_set_cpu() is a LOCK BTS, that is 
> already fully ordered.

... as pretty much any barriers related comment that confuses Peter needs to be 
improved, by definition. ;-)

Thanks,

	Ingo