From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757063Ab1K3LDC (ORCPT ); Wed, 30 Nov 2011 06:03:02 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:50707 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753505Ab1K3LDA (ORCPT ); Wed, 30 Nov 2011 06:03:00 -0500 Message-ID: <4ED60D59.6080308@canonical.com> Date: Wed, 30 Nov 2011 12:02:49 +0100 From: Stefan Bader User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20111124 Thunderbird/8.0 MIME-Version: 1.0 To: Andrew Morton CC: Ingo Molnar , Konrad Rzeszutek Wilk , linux-kernel@vger.kernel.org, a.p.zijlstra@chello.nl, hpa@zytor.com, jeremy.fitzhardinge@citrix.com, mingo@redhat.com, stable@kernel.org, tglx@linutronix.de, Stephen Rothwell , Rusty Russell Subject: Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree References: <201110141951.p9EJpn3A006989@hpaq5.eem.corp.google.com> <20111025182450.GA9843@phenom.dumpdata.com> <20111027155329.0adc1358.akpm@linux-foundation.org> <20111028070838.GG12995@elte.hu> <20111028003935.a75d16b6.akpm@linux-foundation.org> In-Reply-To: <20111028003935.a75d16b6.akpm@linux-foundation.org> X-Enigmail-Version: 1.4a1pre Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 28.10.2011 09:39, Andrew Morton wrote: > On Fri, 28 Oct 2011 09:08:39 +0200 Ingo Molnar wrote: > >> >> * Andrew Morton wrote: >> >>> On Tue, 25 Oct 2011 14:24:50 -0400 >>> Konrad Rzeszutek Wilk wrote: >>> >>>> On Fri, Oct 14, 2011 at 12:51:48PM -0700, akpm@google.com wrote: >>>>> >>>>> The patch titled >>>>> Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode >>>>> has been removed from the -mm tree. Its filename was >>>>> x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch >>>>> >>>>> This patch was dropped because it was merged into mainline or a subsystem tree >>>> >>>> Hey Andrew, >>>> >>>> I am actually not seeing this in mainline? Was it accidently dropped out of your tree? >>> >>> hm, well spotted. I'm not sure what happened here - possibly the >>> patch was merged into an x86 tree (and hence linux-next) but later >>> got lost. Or possibly not, and I just screwed up. >> >> No, a patch with the -i 'paravirt.*lazy' pattern never touched -tip, >> even temporarily. >> >> Could it be that someone else (say the Xen guys) picked it up, it >> went into linux-next, you thought it's applied - but then they >> dropped it? >> >> Do we have a full log of all linux-next patches? > > Don't know. > > The patch was present in the linux-next which I pulled on 14 Oct. It > is no longer in linux-next. > So, as of today, this seems to be back on the master branch of linux-next (I guess from Andrew putting it back, but I am never sure with linux-next). But I am not sure how/when this would go into Linus tree. I assume without any specific action maybe merge window for 3.3... We got some positive feedback on it from users running into the problem. So it seems like a valuable change. From the discusions so far I take that technically the change did not trigger resistance. For that reason I wanted to ask whether there is a chance that this looks important enough to be pushed before the next merge window... Thanks, Stefan > Here's my copy: > > > From: Konrad Rzeszutek Wilk > Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode > > Fix an outstanding issue that has been reported since 2.6.37. Under a > heavy loaded machine processing "fork()" calls could crash with: > > BUG: unable to handle kernel paging request at f573fc8c > IP: [] swap_count_continued+0x104/0x180 > *pdpt = 000000002a3b9027 *pde = 0000000001bed067 *pte = 0000000000000000 > Oops: 0000 [#1] SMP > Modules linked in: > Pid: 1638, comm: apache2 Not tainted 3.0.4-linode37 #1 > EIP: 0061:[] EFLAGS: 00210246 CPU: 3 > EIP is at swap_count_continued+0x104/0x180 > .. snip.. > Call Trace: > [] ? __swap_duplicate+0xc2/0x160 > [] ? pte_mfn_to_pfn+0x87/0xe0 > [] ? swap_duplicate+0x14/0x40 > [] ? copy_pte_range+0x45b/0x500 > [] ? copy_page_range+0x195/0x200 > [] ? dup_mmap+0x1c6/0x2c0 > [] ? dup_mm+0xa8/0x130 > [] ? copy_process+0x98a/0xb30 > [] ? do_fork+0x4f/0x280 > [] ? getnstimeofday+0x43/0x100 > [] ? sys_clone+0x30/0x40 > [] ? ptregs_clone+0x15/0x48 > [] ? syscall_call+0x7/0xb > > The problem is that in copy_page_range() we turn lazy mode on, and then in > swap_entry_free() we call swap_count_continued() which ends up in: > > map = kmap_atomic(page, KM_USER0) + offset; > > and then later we touch *map. > > Since we are running in batched mode (lazy) we don't actually set up the > PTE mappings and the kmap_atomic is not done synchronously and ends up > trying to dereference a page that has not been set. > > Looking at kmap_atomic_prot_pfn(), it uses 'arch_flush_lazy_mmu_mode' and > doing the same in kmap_atomic_prot() and __kunmap_atomic() makes the problem > go away. > > Interestingly, commit b8bcfe997e4615 ("x86/paravirt: remove lazy mode in > interrupts") removed part of this to fix an interrupt issue - but it went > to far and did not consider this scenario. > > Signed-off-by: Konrad Rzeszutek Wilk > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: Peter Zijlstra > Cc: Jeremy Fitzhardinge > Cc: > Signed-off-by: Andrew Morton > --- > > arch/x86/mm/highmem_32.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff -puN arch/x86/mm/highmem_32.c~x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode arch/x86/mm/highmem_32.c > --- a/arch/x86/mm/highmem_32.c~x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode > +++ a/arch/x86/mm/highmem_32.c > @@ -45,6 +45,7 @@ void *kmap_atomic_prot(struct page *page > vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx); > BUG_ON(!pte_none(*(kmap_pte-idx))); > set_pte(kmap_pte-idx, mk_pte(page, prot)); > + arch_flush_lazy_mmu_mode(); > > return (void *)vaddr; > } > @@ -88,6 +89,7 @@ void __kunmap_atomic(void *kvaddr) > */ > kpte_clear_flush(kmap_pte-idx, vaddr); > kmap_atomic_idx_pop(); > + arch_flush_lazy_mmu_mode(); > } > #ifdef CONFIG_DEBUG_HIGHMEM > else { > _ > > >> But IMO it's at least as important to figure out what went wrong. I >> somehow doubt it that you spuriously dropped it - that someone else >> messes up has a far higher likelihood. > > My drop was legitimate. > > Here's the commit from the Oct 14 linux-next: > > > commit ab67482036cee590753dd42b7f66aada97e6dcde > Author: Konrad Rzeszutek Wilk > AuthorDate: Fri Sep 23 17:02:29 2011 -0400 > Commit: Konrad Rzeszutek Wilk > CommitDate: Mon Sep 26 09:12:37 2011 -0400 > > x86/paravirt: Partially revert "remove lazy mode in interrupts" > > which has git commit b8bcfe997e46150fedcc3f5b26b846400122fdd9. > > The unintended consequence of removing the flushing of MMU > updates when doing kmap_atomic (or kunmap_atomic) is that we can > hit a dereference bug when processing a "fork()" under a heavy loaded > machine. Specifically we can hit: > > BUG: unable to handle kernel paging request at f573fc8c > IP: [] swap_count_continued+0x104/0x180 > *pdpt = 000000002a3b9027 *pde = 0000000001bed067 *pte = 0000000000000000 > Oops: 0000 [#1] SMP > Modules linked in: > Pid: 1638, comm: apache2 Not tainted 3.0.4-linode37 #1 > EIP: 0061:[] EFLAGS: 00210246 CPU: 3 > EIP is at swap_count_continued+0x104/0x180 > .. snip.. > Call Trace: > [] ? __swap_duplicate+0xc2/0x160 > [] ? pte_mfn_to_pfn+0x87/0xe0 > [] ? swap_duplicate+0x14/0x40 > [] ? copy_pte_range+0x45b/0x500 > [] ? copy_page_range+0x195/0x200 > [] ? dup_mmap+0x1c6/0x2c0 > [] ? dup_mm+0xa8/0x130 > [] ? copy_process+0x98a/0xb30 > [] ? do_fork+0x4f/0x280 > [] ? getnstimeofday+0x43/0x100 > [] ? sys_clone+0x30/0x40 > [] ? ptregs_clone+0x15/0x48 > [] ? syscall_call+0x7/0xb > > The problem looks that in copy_page_range we turn lazy mode on, and then > in swap_entry_free we call swap_count_continued which ends up in: > > map = kmap_atomic(page, KM_USER0) + offset; > > and then later touches *map. > > Since we are running in batched mode (lazy) we don't actually set up the > PTE mappings and the kmap_atomic is not done synchronously and ends up > trying to dereference a page that has not been set. > > Looking at kmap_atomic_prot_pfn, it uses 'arch_flush_lazy_mmu_mode' and > sprinkling that in kmap_atomic_prot and __kunmap_atomic makes the problem > go away. > > CC: Thomas Gleixner > CC: Ingo Molnar > CC: "H. Peter Anvin" > CC: x86@kernel.org > CC: Peter Zijlstra > CC: Jeremy Fitzhardinge > CC: stable@kernel.org > Signed-off-by: Konrad Rzeszutek Wilk > > diff --git a/arch/x86/mm/highmem_32.c b/arch/x86/mm/highmem_32.c > index b499626..f4f29b1 100644 > --- a/arch/x86/mm/highmem_32.c > +++ b/arch/x86/mm/highmem_32.c > @@ -45,6 +45,7 @@ void *kmap_atomic_prot(struct page *page, pgprot_t prot) > vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx); > BUG_ON(!pte_none(*(kmap_pte-idx))); > set_pte(kmap_pte-idx, mk_pte(page, prot)); > + arch_flush_lazy_mmu_mode(); > > return (void *)vaddr; > } > @@ -88,6 +89,7 @@ void __kunmap_atomic(void *kvaddr) > */ > kpte_clear_flush(kmap_pte-idx, vaddr); > kmap_atomic_idx_pop(); > + arch_flush_lazy_mmu_mode(); > } > #ifdef CONFIG_DEBUG_HIGHMEM > else { > > > I'm not sure what to make of that. The signoffs imply that Konrad is > running his own git tree, but I don't think he is. Or someone (Jeremy > or Rusty I think) merged it but didn't add a signoff. > > Note that the patch was merged using its old name "x86/paravirt: > Partially revert "remove lazy mode in interrupts"". The patch got > renamed to "x86/paravirt: PTE updates in k(un)map_atomic need to be > synchronous, regardless of lazy_mmu mode" and perhaps this prompted > someone to drop the old-named patch then lose the new-named one. > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/