Re: [PATCH v3 8/8] x86, mm, gup: prevent get_page() race with munmap in paravirt guest

From: Peter Zijlstra <peterz@infradead.org>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Ajay Kaher <akaher@vmware.com>,
	gregkh@linuxfoundation.org, stable@vger.kernel.org,
	torvalds@linux-foundation.org, punit.agrawal@arm.com,
	akpm@linux-foundation.org, kirill.shutemov@linux.intel.com,
	willy@infradead.org, will.deacon@arm.com, mszeredi@redhat.com,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	srivatsab@vmware.com, srivatsa@csail.mit.edu,
	amakhalov@vmware.com, srinidhir@vmware.com, bvikas@vmware.com,
	anishs@vmware.com, vsirnapalli@vmware.com, srostedt@vmware.com,
	Vlastimil Babka <vbabka@suse.cz>,
	Oscar Salvador <osalvador@suse.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Juergen Gross <jgross@suse.com>,
	Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Andy Lutomirski <luto@kernel.org>
Subject: Re: [PATCH v3 8/8] x86, mm, gup: prevent get_page() race with munmap in paravirt guest
Date: Mon, 16 Dec 2019 14:47:25 +0100	[thread overview]
Message-ID: <20191216134725.GE2827@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <87lfrc9z3v.fsf@vitty.brq.redhat.com>

On Mon, Dec 16, 2019 at 02:30:44PM +0100, Vitaly Kuznetsov wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > On Tue, Dec 17, 2019 at 02:15:48AM +0530, Ajay Kaher wrote:
> >> From: Vlastimil Babka <vbabka@suse.cz>
> >> 
> >> The x86 version of get_user_pages_fast() relies on disabled interrupts to
> >> synchronize gup_pte_range() between gup_get_pte(ptep); and get_page() against
> >> a parallel munmap. The munmap side nulls the pte, then flushes TLBs, then
> >> releases the page. As TLB flush is done synchronously via IPI disabling
> >> interrupts blocks the page release, and get_page(), which assumes existing
> >> reference on page, is thus safe.
> >> However when TLB flush is done by a hypercall, e.g. in a Xen PV guest, there is
> >> no blocking thanks to disabled interrupts, and get_page() can succeed on a page
> >> that was already freed or even reused.
> >> 
> >> We have recently seen this happen with our 4.4 and 4.12 based kernels, with
> >> userspace (java) that exits a thread, where mm_release() performs a futex_wake()
> >> on tsk->clear_child_tid, and another thread in parallel unmaps the page where
> >> tsk->clear_child_tid points to. The spurious get_page() succeeds, but futex code
> >> immediately releases the page again, while it's already on a freelist. Symptoms
> >> include a bad page state warning, general protection faults acessing a poisoned
> >> list prev/next pointer in the freelist, or free page pcplists of two cpus joined
> >> together in a single list. Oscar has also reproduced this scenario, with a
> >> patch inserting delays before the get_page() to make the race window larger.
> >> 
> >> Fix this by removing the dependency on TLB flush interrupts the same way as the
> >
> > This is suppsed to be fixed by:
> >
> > arch/x86/Kconfig:       select HAVE_RCU_TABLE_FREE              if PARAVIRT
> >
> 
> Yes,
> 
> but HAVE_RCU_TABLE_FREE was enabled on x86 only in 4.14:
> 
> commit 9e52fc2b50de3a1c08b44f94c610fbe998c0031a
> Author: Vitaly Kuznetsov <vkuznets@redhat.com>
> Date:   Mon Aug 28 10:22:51 2017 +0200
> 
>     x86/mm: Enable RCU based page table freeing (CONFIG_HAVE_RCU_TABLE_FREE=y)
> 
> and, if I understood correctly, Ajay is suggesting the patch for older
> stable kernels (4.9 and 4.4 I would guess).

It wasn't at all clear this was targeted at old kernels (I only got this
one patch).

And why can't those necro kernels do backports of the upstream solution?