Re: [PATCH v34 06/14] arm64: kdump: protect crash dump kernel memory

2017-03-30 Thread Ard Biesheuvel 
On 30 March 2017 at 10:56, AKASHI Takahiro  wrote:
> Date: Tue, 28 Mar 2017 15:51:24 +0900
> Subject: [PATCH] arm64: kdump: protect crash dump kernel memory
>
> arch_kexec_protect_crashkres() and arch_kexec_unprotect_crashkres()
> are meant to be called by kexec_load() in order to protect the memory
> allocated for crash dump kernel once the image is loaded.
>
> The protection is implemented by unmapping the relevant segments in crash
> dump kernel memory, rather than making it read-only as other archs do,
> to prevent coherency issues due to potential cache aliasing (with
> mismatched attributes).
>
> Page-level mappings are consistently used here so that we can change
> the attributes of segments in page granularity as well as shrink the region
> also in page granularity through /sys/kernel/kexec_crash_size, putting
> the freed memory back to buddy system.
>
> Signed-off-by: AKASHI Takahiro 

This looks mostly correct. One comment below.

> ---
>  arch/arm64/kernel/machine_kexec.c |  32 +---
>  arch/arm64/mm/mmu.c   | 103 
> --
>  2 files changed, 80 insertions(+), 55 deletions(-)
>
> diff --git a/arch/arm64/kernel/machine_kexec.c 
> b/arch/arm64/kernel/machine_kexec.c
> index bc96c8a7fc79..b63baa749609 100644
> --- a/arch/arm64/kernel/machine_kexec.c
> +++ b/arch/arm64/kernel/machine_kexec.c
> @@ -14,7 +14,9 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>
>  #include "cpu-reset.h"
>
> @@ -22,8 +24,6 @@
>  extern const unsigned char arm64_relocate_new_kernel[];
>  extern const unsigned long arm64_relocate_new_kernel_size;
>
> -static unsigned long kimage_start;
> -
>  /**
>   * kexec_image_info - For debugging output.
>   */
> @@ -64,8 +64,6 @@ void machine_kexec_cleanup(struct kimage *kimage)
>   */
>  int machine_kexec_prepare(struct kimage *kimage)
>  {
> -   kimage_start = kimage->start;
> -
> kexec_image_info(kimage);
>
> if (kimage->type != KEXEC_TYPE_CRASH && cpus_are_stuck_in_kernel()) {
> @@ -183,7 +181,7 @@ void machine_kexec(struct kimage *kimage)
> kexec_list_flush(kimage);
>
> /* Flush the new image if already in place. */
> -   if (kimage->head & IND_DONE)
> +   if ((kimage != kexec_crash_image) && (kimage->head & IND_DONE))
> kexec_segment_flush(kimage);
>
> pr_info("Bye!\n");
> @@ -201,7 +199,7 @@ void machine_kexec(struct kimage *kimage)
>  */
>
> cpu_soft_restart(1, reboot_code_buffer_phys, kimage->head,
> -   kimage_start, 0);
> +   kimage->start, 0);
>
> BUG(); /* Should never get here. */
>  }
> @@ -210,3 +208,25 @@ void machine_crash_shutdown(struct pt_regs *regs)
>  {
> /* Empty routine needed to avoid build errors. */
>  }
> +
> +void arch_kexec_protect_crashkres(void)
> +{
> +   int i;
> +
> +   kexec_segment_flush(kexec_crash_image);
> +
> +   for (i = 0; i < kexec_crash_image->nr_segments; i++)
> +   set_memory_valid(
> +   __phys_to_virt(kexec_crash_image->segment[i].mem),
> +   kexec_crash_image->segment[i].memsz >> PAGE_SHIFT, 0);
> +}
> +
> +void arch_kexec_unprotect_crashkres(void)
> +{
> +   int i;
> +
> +   for (i = 0; i < kexec_crash_image->nr_segments; i++)
> +   set_memory_valid(
> +   __phys_to_virt(kexec_crash_image->segment[i].mem),
> +   kexec_crash_image->segment[i].memsz >> PAGE_SHIFT, 1);
> +}
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 91502e36e6d9..3cde5f2d30ec 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -22,6 +22,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -393,10 +395,28 @@ static void update_mapping_prot(phys_addr_t phys, 
> unsigned long virt,
> flush_tlb_kernel_range(virt, virt + size);
>  }
>
> -static void __init __map_memblock(pgd_t *pgd, phys_addr_t start, phys_addr_t 
> end)
> +static void __init __map_memblock(pgd_t *pgd, phys_addr_t start,
> + phys_addr_t end, pgprot_t prot, int flags)
> +{
> +   __create_pgd_mapping(pgd, start, __phys_to_virt(start), end - start,
> +prot, early_pgtable_alloc, flags);
> +}
> +
> +void __init mark_linear_text_alias_ro(void)
> +{
> +   /*
> +* Remove the write permissions from the linear alias of .text/.rodata
> +*/
> +   update_mapping_prot(__pa_symbol(_text), (unsigned 
> long)lm_alias(_text),
> +   (unsigned long)__init_begin - (unsigned 
> long)_text,
> +   PAGE_KERNEL_RO);
> +}
> +
> +static void __init map_mem(pgd_t *pgd)
>  {
> phys_addr_t kernel_start = __pa_symbol(_text);
> phys_addr_t kernel_end = __pa_symbol(__init_begin);
> +   struct memblock_region

Re: [RFC PATCH] x86_64/mm/boot: Fix kernel_ident_mapping_init() failure for kexec

2017-03-30 Thread Xunlei Pang 
On 03/30/2017 at 07:21 PM, Xunlei Pang wrote:
> On 03/24/2017 at 08:04 PM, Kirill A. Shutemov wrote:
>> On Mon, Mar 20, 2017 at 02:11:31PM +0800, Xunlei Pang wrote:
>>> I found that the kdump is broken on linux-4.11.0-rc2+
>> That's actually tip tree or linux-next. The problematic change is not in
>> Linus' tree.
>>
>>> , probably
>>> due to the 5level-paging feature that "#define p4d_present(p4d) 1",
>>> as a result in ident_p4d_init(), it will go into ident_pud_init()
>>> directly without allocating the new pud.
>>>
>>> Looks like this patch can make it work again.
>> Okay, that's bisectability issue. Uncovered by splitting my patchset into
>> parts.
>>
>> Could you check if applying "Part 2" of 5-level paging changes[1] would
>> help you?
> I confirmed that it works after applying your following patches:
>   x86: Convert the rest of the code to support p4d_t

To be exact, this one("x86: Convert the rest of the code to support p4d_t") 
fixes the issue.

>   x86/xen: Change __xen_pgd_walk() and xen_cleanmfnmap() to support p4d
>   x86/kasan: Prepare clear_pgds() to switch to 
>   x86/mm/pat: Add 5-level paging support
>   x86/efi: Add 5-level paging support
>   x86/kexec: Add 5-level paging support
>
> Regards,
> Xunlei
>
>> Making the code work with both  and
>>  would make it even uglier. Not sure if it
>> makes sense to address it on its own if second part fixes the situation.
>>
>> [1] 
>> http://lkml.kernel.org/r/20170317185515.8636-1-kirill.shute...@linux.intel.com
>>
>
> ___
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [RFC PATCH] x86_64/mm/boot: Fix kernel_ident_mapping_init() failure for kexec

2017-03-30 Thread Xunlei Pang 
On 03/24/2017 at 08:04 PM, Kirill A. Shutemov wrote:
> On Mon, Mar 20, 2017 at 02:11:31PM +0800, Xunlei Pang wrote:
>> I found that the kdump is broken on linux-4.11.0-rc2+
> That's actually tip tree or linux-next. The problematic change is not in
> Linus' tree.
>
>> , probably
>> due to the 5level-paging feature that "#define p4d_present(p4d) 1",
>> as a result in ident_p4d_init(), it will go into ident_pud_init()
>> directly without allocating the new pud.
>>
>> Looks like this patch can make it work again.
> Okay, that's bisectability issue. Uncovered by splitting my patchset into
> parts.
>
> Could you check if applying "Part 2" of 5-level paging changes[1] would
> help you?

I confirmed that it works after applying your following patches:
  x86: Convert the rest of the code to support p4d_t
  x86/xen: Change __xen_pgd_walk() and xen_cleanmfnmap() to support p4d
  x86/kasan: Prepare clear_pgds() to switch to 
  x86/mm/pat: Add 5-level paging support
  x86/efi: Add 5-level paging support
  x86/kexec: Add 5-level paging support

Regards,
Xunlei

>
> Making the code work with both  and
>  would make it even uglier. Not sure if it
> makes sense to address it on its own if second part fixes the situation.
>
> [1] 
> http://lkml.kernel.org/r/20170317185515.8636-1-kirill.shute...@linux.intel.com
>


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v34 06/14] arm64: kdump: protect crash dump kernel memory

2017-03-30 Thread AKASHI Takahiro 
Ard,

On Tue, Mar 28, 2017 at 03:05:58PM +0100, Ard Biesheuvel wrote:
> On 28 March 2017 at 12:07, AKASHI Takahiro  wrote:
> > Ard,
> >
> > On Tue, Mar 28, 2017 at 11:07:05AM +0100, Ard Biesheuvel wrote:
> >> On 28 March 2017 at 07:51, AKASHI Takahiro  
> >> wrote:
> >> > arch_kexec_protect_crashkres() and arch_kexec_unprotect_crashkres()
> >> > are meant to be called by kexec_load() in order to protect the memory
> >> > allocated for crash dump kernel once the image is loaded.
> >> >
> >> > The protection is implemented by unmapping the relevant segments in crash
> >> > dump kernel memory, rather than making it read-only as other archs do,
> >> > to prevent any corruption due to potential cache alias (with different
> >> > attributes) problem.
> >> >
> >>
> >> I think it would be more accurate to replace 'corruption' with
> >> 'coherency issues', given that this patch does not solve the issue of
> >> writable aliases that may be used to modify the contents of the
> >> region, but it does prevent issues related to mismatched attributes
> >> (which are arguably a bigger concern)
> >
> > OK
> >
> >> > Page-level mappings are consistently used here so that we can change
> >> > the attributes of segments in page granularity as well as shrink the 
> >> > region
> >> > also in page granularity through /sys/kernel/kexec_crash_size, putting
> >> > the freed memory back to buddy system.
> >> >
> >> > Signed-off-by: AKASHI Takahiro 
> >>
> >> As a head's up, this patch is going to conflict heavily with patches
> >> that are queued up in arm64/for-next/core atm.
> >
> > I'll look into it later, but
> >
> >> Some questions below.
> >>
> >> > ---
> >> >  arch/arm64/kernel/machine_kexec.c | 32 +++---
> >> >  arch/arm64/mm/mmu.c   | 90 
> >> > ---
> >> >  2 files changed, 72 insertions(+), 50 deletions(-)
> >> >
> >> > diff --git a/arch/arm64/kernel/machine_kexec.c 
> >> > b/arch/arm64/kernel/machine_kexec.c
> >> > index bc96c8a7fc79..b63baa749609 100644
> >> > --- a/arch/arm64/kernel/machine_kexec.c
> >> > +++ b/arch/arm64/kernel/machine_kexec.c
> >> > @@ -14,7 +14,9 @@
> >> >
> >> >  #include 
> >> >  #include 
> >> > +#include 
> >> >  #include 
> >> > +#include 
> >> >
> >> >  #include "cpu-reset.h"
> >> >
> >> > @@ -22,8 +24,6 @@
> >> >  extern const unsigned char arm64_relocate_new_kernel[];
> >> >  extern const unsigned long arm64_relocate_new_kernel_size;
> >> >
> >> > -static unsigned long kimage_start;
> >> > -
> >> >  /**
> >> >   * kexec_image_info - For debugging output.
> >> >   */
> >> > @@ -64,8 +64,6 @@ void machine_kexec_cleanup(struct kimage *kimage)
> >> >   */
> >> >  int machine_kexec_prepare(struct kimage *kimage)
> >> >  {
> >> > -   kimage_start = kimage->start;
> >> > -
> >> > kexec_image_info(kimage);
> >> >
> >> > if (kimage->type != KEXEC_TYPE_CRASH && 
> >> > cpus_are_stuck_in_kernel()) {
> >> > @@ -183,7 +181,7 @@ void machine_kexec(struct kimage *kimage)
> >> > kexec_list_flush(kimage);
> >> >
> >> > /* Flush the new image if already in place. */
> >> > -   if (kimage->head & IND_DONE)
> >> > +   if ((kimage != kexec_crash_image) && (kimage->head & IND_DONE))
> >> > kexec_segment_flush(kimage);
> >> >
> >> > pr_info("Bye!\n");
> >> > @@ -201,7 +199,7 @@ void machine_kexec(struct kimage *kimage)
> >> >  */
> >> >
> >> > cpu_soft_restart(1, reboot_code_buffer_phys, kimage->head,
> >> > -   kimage_start, 0);
> >> > +   kimage->start, 0);
> >> >
> >> > BUG(); /* Should never get here. */
> >> >  }
> >> > @@ -210,3 +208,25 @@ void machine_crash_shutdown(struct pt_regs *regs)
> >> >  {
> >> > /* Empty routine needed to avoid build errors. */
> >> >  }
> >> > +
> >> > +void arch_kexec_protect_crashkres(void)
> >> > +{
> >> > +   int i;
> >> > +
> >> > +   kexec_segment_flush(kexec_crash_image);
> >> > +
> >> > +   for (i = 0; i < kexec_crash_image->nr_segments; i++)
> >> > +   set_memory_valid(
> >> > +   
> >> > __phys_to_virt(kexec_crash_image->segment[i].mem),
> >> > +   kexec_crash_image->segment[i].memsz >> 
> >> > PAGE_SHIFT, 0);
> >> > +}
> >> > +
> >> > +void arch_kexec_unprotect_crashkres(void)
> >> > +{
> >> > +   int i;
> >> > +
> >> > +   for (i = 0; i < kexec_crash_image->nr_segments; i++)
> >> > +   set_memory_valid(
> >> > +   
> >> > __phys_to_virt(kexec_crash_image->segment[i].mem),
> >> > +   kexec_crash_image->segment[i].memsz >> 
> >> > PAGE_SHIFT, 1);
> >> > +}
> >> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> > index d28dbcf596b6..f6a3c0e9d37f 100644
> >> > --- a/arch/arm64/mm/mmu.c
> >> > +++ b/arch/arm64/mm/mmu.c
> >> > @@ -22,6 +22,8 @@
> >> >  #include 
> >> >  #include 
>