linux-kernel - Re: [PATCH 1/3] x86/mm/pat: Disable preemption around __flush_tlb

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:   Wed, 17 Oct 2018 13:11:45 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     'Sebastian Andrzej Siewior' <bigeasy@...utronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH 1/3] x86/mm/pat: Disable preemption around
 __flush_tlb_all()

On Wed, Oct 17, 2018 at 09:54:38AM +0000, David Laight wrote:
> From: Sebastian Andrzej Siewior
> > Sent: 16 October 2018 21:25
> > I've seen the WARN_ON_ONCE(__read_cr3() != build_cr3()) in
> > switch_mm_irqs_off() every once in a while during a snapshotted system
> > upgrade.
> > I also saw the warning early during which was introduced in commit
> > decab0888e6e ("x86/mm: Remove preempt_disable/enable() from
> > __native_flush_tlb()"). The callchain is
> > 
> >   get_page_from_freelist() -> post_alloc_hook() -> __kernel_map_pages()
> > 
> > with CONFIG_DEBUG_PAGEALLOC enabled.
> > 
> > Turns out, once I disable preemption around __flush_tlb_all() both
> > warnings do not appear.
> > 
> > Disable preemption during CR3 reset / __flush_tlb_all().
> > 
> > Fixes: decab0888e6e ("x86/mm: Remove preempt_disable/enable() from __native_flush_tlb()")
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> > ---
> >  arch/x86/mm/pageattr.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
> > index 51a5a69ecac9f..fe6b21f0a6631 100644
> > --- a/arch/x86/mm/pageattr.c
> > +++ b/arch/x86/mm/pageattr.c
> > @@ -2088,7 +2088,9 @@ void __kernel_map_pages(struct page *page, int numpages, int enable)
> >  	 * We should perform an IPI and flush all tlbs,
> >  	 * but that can deadlock->flush only current cpu:
> >  	 */
> > +	preempt_disable();
> >  	__flush_tlb_all();
> > +	preempt_enable();
> 
> Can it make any sense to flush the tlb with preemption enabled?
> Surely preemption must be disabled over something else as well?

This code is fishy anyway, for only doing that local invalidate.

Ideally we'd never ever merge anything that only does local invalidates,
on a global address space, that's just broken.