Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1671290 > unrolled thread

Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of poison pages

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings  of poison pages Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-06-21 04:20 +0200
    Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings  of poison pages "Luck, Tony" <tony.luck@intel.com> - 2017-06-21 20:00 +0200
      RE: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of  poison pages "Elliott, Robert (Persistent Memory)" <elliott@hpe.com> - 2017-06-21 21:50 +0200
        RE: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings  of poison pages "Luck, Tony" <tony.luck@intel.com> - 2017-06-21 22:40 +0200
          Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of  poison pages Dan Williams <dan.j.williams@intel.com> - 2017-06-23 07:10 +0200
            Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings  of poison pages "Luck, Tony" <tony.luck@intel.com> - 2017-06-23 23:00 +0200

#1671290 — Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of poison pages

(drop stable from CC)

On Fri, Jun 16, 2017 at 12:02:00PM -0700, Luck, Tony wrote:
> From: Tony Luck <tony.luck@intel.com>
> 
> Speculative processor accesses may reference any memory that has a
> valid page table entry.  While a speculative access won't generate
> a machine check, it will log the error in a machine check bank. That
> could cause escalation of a subsequent error since the overflow bit
> will be then set in the machine check bank status register.
> 
> Code has to be double-plus-tricky to avoid mentioning the 1:1 virtual
> address of the page we want to map out otherwise we may trigger the
> very problem we are trying to avoid.  We use a non-canonical address
> that passes through the usual Linux table walking code to get to the
> same "pte".
> 
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: x86@kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> Thanks to Dave Hansen for reviewing several iterations of this.
> 
>  arch/x86/include/asm/page_64.h   |  4 ++++
>  arch/x86/kernel/cpu/mcheck/mce.c | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/mm_inline.h        |  6 ++++++
>  mm/memory-failure.c              |  2 ++
>  4 files changed, 47 insertions(+)
> 
> diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
> index b4a0d43248cf..b50df06ad251 100644
> --- a/arch/x86/include/asm/page_64.h
> +++ b/arch/x86/include/asm/page_64.h
> @@ -51,6 +51,10 @@ static inline void clear_page(void *page)
>  
>  void copy_page(void *to, void *from);
>  
> +#ifdef CONFIG_X86_MCE
> +#define arch_unmap_kpfn arch_unmap_kpfn
> +#endif
> +
>  #endif	/* !__ASSEMBLY__ */
>  
>  #ifdef CONFIG_X86_VSYSCALL_EMULATION
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 5cfbaeb6529a..56563db0b2be 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -51,6 +51,7 @@
>  #include <asm/mce.h>
>  #include <asm/msr.h>
>  #include <asm/reboot.h>
> +#include <asm/set_memory.h>
>  
>  #include "mce-internal.h"
>  
> @@ -1056,6 +1057,40 @@ static int do_memory_failure(struct mce *m)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_X86_64
> +
> +void arch_unmap_kpfn(unsigned long pfn)
> +{
> +	unsigned long decoy_addr;
> +
> +	/*
> +	 * Unmap this page from the kernel 1:1 mappings to make sure
> +	 * we don't log more errors because of speculative access to
> +	 * the page.
> +	 * We would like to just call:
> +	 *	set_memory_np((unsigned long)pfn_to_kaddr(pfn), 1);
> +	 * but doing that would radically increase the odds of a
> +	 * speculative access to the posion page because we'd have
> +	 * the virtual address of the kernel 1:1 mapping sitting
> +	 * around in registers.
> +	 * Instead we get tricky.  We create a non-canonical address
> +	 * that looks just like the one we want, but has bit 63 flipped.
> +	 * This relies on set_memory_np() not checking whether we passed
> +	 * a legal address.
> +	 */
> +
> +#if PGDIR_SHIFT + 9 < 63 /* 9 because cpp doesn't grok ilog2(PTRS_PER_PGD) */
> +	decoy_addr = (pfn << PAGE_SHIFT) + (PAGE_OFFSET ^ BIT(63));
> +#else
> +#error "no unused virtual bit available"
> +#endif
> +
> +	if (set_memory_np(decoy_addr, 1))
> +		pr_warn("Could not invalidate pfn=0x%lx from 1:1 map \n", pfn);
> +
> +}
> +#endif
> +
>  /*
>   * The actual machine check handler. This only handles real
>   * exceptions when something got corrupted coming in through int 18.
> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> index e030a68ead7e..25438b2b6f22 100644
> --- a/include/linux/mm_inline.h
> +++ b/include/linux/mm_inline.h
> @@ -126,4 +126,10 @@ static __always_inline enum lru_list page_lru(struct page *page)
>  
>  #define lru_to_page(head) (list_entry((head)->prev, struct page, lru))
>  
> +#ifdef arch_unmap_kpfn
> +extern void arch_unmap_kpfn(unsigned long pfn);
> +#else
> +static __always_inline void arch_unmap_kpfn(unsigned long pfn) { }
> +#endif
> +
>  #endif
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 342fac9ba89b..9479e190dcbd 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1071,6 +1071,8 @@ int memory_failure(unsigned long pfn, int trapno, int flags)
>  		return 0;
>  	}
>  
> +	arch_unmap_kpfn(pfn);
> +

We had better have a reverse operation of this to cancel the unmapping
when unpoisoning?

Thanks,
Naoya Horiguchi

[toc] | [next] | [standalone]

#1671862

On Wed, Jun 21, 2017 at 02:12:27AM +0000, Naoya Horiguchi wrote:

> We had better have a reverse operation of this to cancel the unmapping
> when unpoisoning?

When we have unpoisoning, we can add something.  We don't seem to have
an inverse function for "set_memory_np" to just flip the _PRESENT bit
back on again. But it would be trivial to write a set_memory_pp().

Since we'd be doing this after the poison has been cleared, we wouldn't
need to play games with the address.  We'd just use:

	set_memory_pp((unsigned long)pfn_to_kaddr(pfn), 1);

-Tony

[toc] | [prev] | [next] | [standalone]

#1671991 — RE: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of poison pages

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Luck, Tony
> Sent: Wednesday, June 21, 2017 12:54 PM
> To: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Borislav Petkov <bp@suse.de>; Dave Hansen <dave.hansen@intel.com>;
> x86@kernel.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org

(adding linux-nvdimm list in this reply)

> Subject: Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1
> mappings of poison pages
> 
> On Wed, Jun 21, 2017 at 02:12:27AM +0000, Naoya Horiguchi wrote:
> 
> > We had better have a reverse operation of this to cancel the unmapping
> > when unpoisoning?
> 
> When we have unpoisoning, we can add something.  We don't seem to have
> an inverse function for "set_memory_np" to just flip the _PRESENT bit
> back on again. But it would be trivial to write a set_memory_pp().
> 
> Since we'd be doing this after the poison has been cleared, we wouldn't
> need to play games with the address.  We'd just use:
> 
> 	set_memory_pp((unsigned long)pfn_to_kaddr(pfn), 1);
> 
> -Tony

Persistent memory does have unpoisoning and would require this inverse
operation - see drivers/nvdimm/pmem.c pmem_clear_poison() and core.c
nvdimm_clear_poison().

---
Robert Elliott, HPE Persistent Memory

[toc] | [prev] | [next] | [standalone]

#1672015

> Persistent memory does have unpoisoning and would require this inverse
> operation - see drivers/nvdimm/pmem.c pmem_clear_poison() and core.c
> nvdimm_clear_poison().

Nice.  Well this code will need to cooperate with that ... in particular if the page
is in an area that can be unpoisoned ... then we should do that *instead* of marking
the page not present (which breaks up huge/large pages and so affects performance).

Instead of calling it "arch_unmap_pfn" it could be called something like arch_handle_poison()
and do something like:

void arch_handle_poison(unsigned long pfn)
{
	if this is a pmem page && pmem_clear_poison(pfn)
		return
	if this is a nvdimm page && nvdimm_clear_poison(pfn)
		return
	/* can't clear, map out from 1:1 region */
	... code from my patch ...
}

I'm just not sure how those first two "if" bits work ... particularly in terms of CONFIG dependencies and system
capabilities.  Perhaps each of pmem and nvdimm could register their unpoison functions and this code could
just call each in turn?

-Tony

[toc] | [prev] | [next] | [standalone]

#1673253 — Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of poison pages

On Wed, Jun 21, 2017 at 1:30 PM, Luck, Tony <tony.luck@intel.com> wrote:
>> Persistent memory does have unpoisoning and would require this inverse
>> operation - see drivers/nvdimm/pmem.c pmem_clear_poison() and core.c
>> nvdimm_clear_poison().
>
> Nice.  Well this code will need to cooperate with that ... in particular if the page
> is in an area that can be unpoisoned ... then we should do that *instead* of marking
> the page not present (which breaks up huge/large pages and so affects performance).
>
> Instead of calling it "arch_unmap_pfn" it could be called something like arch_handle_poison()
> and do something like:
>
> void arch_handle_poison(unsigned long pfn)
> {
>         if this is a pmem page && pmem_clear_poison(pfn)
>                 return
>         if this is a nvdimm page && nvdimm_clear_poison(pfn)
>                 return
>         /* can't clear, map out from 1:1 region */
>         ... code from my patch ...
> }
>
> I'm just not sure how those first two "if" bits work ... particularly in terms of CONFIG dependencies and system
> capabilities.  Perhaps each of pmem and nvdimm could register their unpoison functions and this code could
> just call each in turn?

We don't unpoison pmem without new data to write in it's place. What
context is arch_handle_poison() called? Ideally we only "clear" poison
when we know we are trying to write zero over the poisoned range.

[toc] | [prev] | [next] | [standalone]

#1673866

On Thu, Jun 22, 2017 at 10:07:18PM -0700, Dan Williams wrote:
> On Wed, Jun 21, 2017 at 1:30 PM, Luck, Tony <tony.luck@intel.com> wrote:
> >> Persistent memory does have unpoisoning and would require this inverse
> >> operation - see drivers/nvdimm/pmem.c pmem_clear_poison() and core.c
> >> nvdimm_clear_poison().
> >
> > Nice.  Well this code will need to cooperate with that ... in particular if the page
> > is in an area that can be unpoisoned ... then we should do that *instead* of marking
> > the page not present (which breaks up huge/large pages and so affects performance).
> >
> > Instead of calling it "arch_unmap_pfn" it could be called something like arch_handle_poison()
> > and do something like:
> >
> > void arch_handle_poison(unsigned long pfn)
> > {
> >         if this is a pmem page && pmem_clear_poison(pfn)
> >                 return
> >         if this is a nvdimm page && nvdimm_clear_poison(pfn)
> >                 return
> >         /* can't clear, map out from 1:1 region */
> >         ... code from my patch ...
> > }
> >
> > I'm just not sure how those first two "if" bits work ... particularly in terms of CONFIG dependencies and system
> > capabilities.  Perhaps each of pmem and nvdimm could register their unpoison functions and this code could
> > just call each in turn?
> 
> We don't unpoison pmem without new data to write in it's place. What
> context is arch_handle_poison() called? Ideally we only "clear" poison
> when we know we are trying to write zero over the poisoned range.

Context is that of the process that did the access (but we've moved
off the machine check stack and are now in normal kernel context).
We are about to unmap this page from all applications that are
using it.  But they may be running ... so now it a bad time to
clear the poison. They might access the page and not get a signal.

If I move this code to after all the users PTEs have been cleared
and TLBs flushed, then it would be safe to try to unpoison the page
and not invalidate from the 1:1 mapping.

But I'm not sure what happens next. For a normal DDR4 page I could
put it back on the free list and allow it to be re-used. But for
PMEM you have some other cleanup that you need to do to mark the
block as lost from your file system.

Is this too early for you to be able to do that?

-Tony

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web