* [PATCH] ARM: kexec: Fix panic after TLB are invalidated @ 2021-02-01 0:44 Giancarlo Ferrari 2021-02-01 11:34 ` Russell King - ARM Linux admin 2021-02-01 12:47 ` Mark Rutland 0 siblings, 2 replies; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-01 0:44 UTC (permalink / raw) To: linux-arm-kernel, linux Cc: linux-kernel, akpm, rppt, penberg, geert, giancarlo.ferrari, Giancarlo Ferrari machine_kexec() need to set rw permission in text and rodata sections to assign some variables (e.g. kexec_start_address). To do that at the end (after flushing pdm in memory, etc.) it needs to invalidate TLB [section] entries. If during the TLB invalidation an interrupt occours, which might cause a context switch, there is the risk to inject invalid TLBs, with ro permissions. When trying to assign .text labels, this lead to the following: Unable to handle kernel paging request at virtual address 80112f38 pgd = fd7ef03e [80112f38] *pgd=0001141e(bad) Internal error: Oops: 80d [#1] PREEMPT SMP ARM ... Signed-off-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> --- arch/arm/kernel/machine_kexec.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c index 5d84ad3..23e8816 100644 --- a/arch/arm/kernel/machine_kexec.c +++ b/arch/arm/kernel/machine_kexec.c @@ -174,6 +174,13 @@ void machine_kexec(struct kimage *image) reboot_code_buffer = page_address(image->control_code_page); + /* + * If below part is not atomic TLB entries might be corrupted after TLB + * invalidation, which leads to Data Abort in .text variable assignment + */ + raw_local_irq_disable(); + local_fiq_disable(); + /* Prepare parameters for reboot_code_buffer*/ set_kernel_text_rw(); kexec_start_address = image->start; @@ -181,6 +188,9 @@ void machine_kexec(struct kimage *image) kexec_mach_type = machine_arch_type; kexec_boot_atags = image->arch.kernel_r2; + local_fiq_enable(); + raw_local_irq_enable(); + /* copy our kernel relocation code to the control code page */ reboot_entry = fncpy(reboot_code_buffer, &relocate_new_kernel, -- 2.7.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 0:44 [PATCH] ARM: kexec: Fix panic after TLB are invalidated Giancarlo Ferrari @ 2021-02-01 11:34 ` Russell King - ARM Linux admin 2021-02-01 12:47 ` Mark Rutland 1 sibling, 0 replies; 22+ messages in thread From: Russell King - ARM Linux admin @ 2021-02-01 11:34 UTC (permalink / raw) To: Giancarlo Ferrari Cc: linux-arm-kernel, linux-kernel, akpm, rppt, penberg, geert, giancarlo.ferrari I wish others who know this code would get involved, and such stuff wasn't left to me to research and work out whether a patch is correct or not. On Mon, Feb 01, 2021 at 12:44:56AM +0000, Giancarlo Ferrari wrote: > machine_kexec() need to set rw permission in text and rodata sections > to assign some variables (e.g. kexec_start_address). To do that at > the end (after flushing pdm in memory, etc.) it needs to invalidate > TLB [section] entries. > > If during the TLB invalidation an interrupt occours, which might cause > a context switch, there is the risk to inject invalid TLBs, with ro > permissions. > > When trying to assign .text labels, this lead to the following: > > Unable to handle kernel paging request at virtual address 80112f38 > pgd = fd7ef03e > [80112f38] *pgd=0001141e(bad) > Internal error: Oops: 80d [#1] PREEMPT SMP ARM > ... > > Signed-off-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> I don't know this code very well, but I don't think this patch is correct. What happens if we have CRASH_DUMP enabled, and we enter this function with IRQs already disabled? Should we really be re-enabling IRQs? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 0:44 [PATCH] ARM: kexec: Fix panic after TLB are invalidated Giancarlo Ferrari 2021-02-01 11:34 ` Russell King - ARM Linux admin @ 2021-02-01 12:47 ` Mark Rutland 2021-02-01 13:03 ` Russell King - ARM Linux admin 2021-02-01 14:39 ` Giancarlo Ferrari 1 sibling, 2 replies; 22+ messages in thread From: Mark Rutland @ 2021-02-01 12:47 UTC (permalink / raw) To: Giancarlo Ferrari Cc: linux-arm-kernel, linux, linux-kernel, akpm, rppt, penberg, geert, giancarlo.ferrari On Mon, Feb 01, 2021 at 12:44:56AM +0000, Giancarlo Ferrari wrote: > machine_kexec() need to set rw permission in text and rodata sections > to assign some variables (e.g. kexec_start_address). To do that at > the end (after flushing pdm in memory, etc.) it needs to invalidate > TLB [section] entries. It'd be worth noting explicitly that set_kernel_text_rw() alters current->active_mm... > If during the TLB invalidation an interrupt occours, which might cause > a context switch, there is the risk to inject invalid TLBs, with ro > permissions. ... which is why if there's a context switch things can go wrong, since active_mm isn't stable, and so it's possible that set_kernel_text_rw() updates multiple tables, none of which might be the active table at the point we try to make an access. It would be nice to spell that out rather than saying "invalid TLBs". We could disable preemption to prevent that, which is possibly better than disabling interrupts. Overall, it would be much better to avoid having to mess with the kernel page tables. So rather than going: 1. mark kernel RW 2. alter variables in reloc code 3. copy reloc code into buffer 4. branch to buffer ... we should be able to go: 1. copy reloc code into buffer 2. alter variables in copy of reloc code 3. branch to buffer ... which would avoid this class of problem too. Thanks, Mark. > When trying to assign .text labels, this lead to the following: > > Unable to handle kernel paging request at virtual address 80112f38 > pgd = fd7ef03e > [80112f38] *pgd=0001141e(bad) > Internal error: Oops: 80d [#1] PREEMPT SMP ARM > ... > > Signed-off-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> > --- > arch/arm/kernel/machine_kexec.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c > index 5d84ad3..23e8816 100644 > --- a/arch/arm/kernel/machine_kexec.c > +++ b/arch/arm/kernel/machine_kexec.c > @@ -174,6 +174,13 @@ void machine_kexec(struct kimage *image) > > reboot_code_buffer = page_address(image->control_code_page); > > + /* > + * If below part is not atomic TLB entries might be corrupted after TLB > + * invalidation, which leads to Data Abort in .text variable assignment > + */ > + raw_local_irq_disable(); > + local_fiq_disable(); > + > /* Prepare parameters for reboot_code_buffer*/ > set_kernel_text_rw(); > kexec_start_address = image->start; > @@ -181,6 +188,9 @@ void machine_kexec(struct kimage *image) > kexec_mach_type = machine_arch_type; > kexec_boot_atags = image->arch.kernel_r2; > > + local_fiq_enable(); > + raw_local_irq_enable(); > + > /* copy our kernel relocation code to the control code page */ > reboot_entry = fncpy(reboot_code_buffer, > &relocate_new_kernel, > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 12:47 ` Mark Rutland @ 2021-02-01 13:03 ` Russell King - ARM Linux admin 2021-02-01 13:57 ` Mark Rutland 2021-02-01 14:39 ` Giancarlo Ferrari 1 sibling, 1 reply; 22+ messages in thread From: Russell King - ARM Linux admin @ 2021-02-01 13:03 UTC (permalink / raw) To: Mark Rutland Cc: Giancarlo Ferrari, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari On Mon, Feb 01, 2021 at 12:47:20PM +0000, Mark Rutland wrote: > 1. copy reloc code into buffer > 2. alter variables in copy of reloc code > 3. branch to buffer > > ... which would avoid this class of problem too. Yep, slightly messy to do though: diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c index 5d84ad333f05..6058e0d3a40d 100644 --- a/arch/arm/kernel/machine_kexec.c +++ b/arch/arm/kernel/machine_kexec.c @@ -174,18 +174,27 @@ void machine_kexec(struct kimage *image) reboot_code_buffer = page_address(image->control_code_page); - /* Prepare parameters for reboot_code_buffer*/ - set_kernel_text_rw(); - kexec_start_address = image->start; - kexec_indirection_page = page_list; - kexec_mach_type = machine_arch_type; - kexec_boot_atags = image->arch.kernel_r2; - /* copy our kernel relocation code to the control code page */ reboot_entry = fncpy(reboot_code_buffer, &relocate_new_kernel, relocate_new_kernel_size); +#define set(what, val) \ + do { \ + uintptr_t __funcp_address; \ + int __offset; \ + void *__ptr; \ + asm("" : "=r" (__funcp_address) : "0" (&relocate_new_kernel)); \ + __offset = (uintptr_t)&(what) - (__funcp_address & ~1); \ + __ptr = reboot_code_buffer + __offset; \ + *(__typeof__(&(what)))__ptr = val; \ + } while (0) + + set(kexec_start_address, image->start); + set(kexec_indirection_page, page_list); + set(kexec_mach_type, machine_arch_type); + set(kexec_boot_atags, image->arch.kernel_r2); + /* get the identity mapping physical address for the reboot code */ reboot_entry_phys = virt_to_idmap(reboot_entry); -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 13:03 ` Russell King - ARM Linux admin @ 2021-02-01 13:57 ` Mark Rutland 2021-02-01 16:08 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 22+ messages in thread From: Mark Rutland @ 2021-02-01 13:57 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Giancarlo Ferrari, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari On Mon, Feb 01, 2021 at 01:03:45PM +0000, Russell King - ARM Linux admin wrote: > On Mon, Feb 01, 2021 at 12:47:20PM +0000, Mark Rutland wrote: > > 1. copy reloc code into buffer > > 2. alter variables in copy of reloc code > > 3. branch to buffer > > > > ... which would avoid this class of problem too. > > Yep, slightly messy to do though: > > diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c > index 5d84ad333f05..6058e0d3a40d 100644 > --- a/arch/arm/kernel/machine_kexec.c > +++ b/arch/arm/kernel/machine_kexec.c > @@ -174,18 +174,27 @@ void machine_kexec(struct kimage *image) > > reboot_code_buffer = page_address(image->control_code_page); > > - /* Prepare parameters for reboot_code_buffer*/ > - set_kernel_text_rw(); > - kexec_start_address = image->start; > - kexec_indirection_page = page_list; > - kexec_mach_type = machine_arch_type; > - kexec_boot_atags = image->arch.kernel_r2; > - > /* copy our kernel relocation code to the control code page */ > reboot_entry = fncpy(reboot_code_buffer, > &relocate_new_kernel, > relocate_new_kernel_size); > > +#define set(what, val) \ > + do { \ > + uintptr_t __funcp_address; \ > + int __offset; \ > + void *__ptr; \ > + asm("" : "=r" (__funcp_address) : "0" (&relocate_new_kernel)); \ > + __offset = (uintptr_t)&(what) - (__funcp_address & ~1); \ > + __ptr = reboot_code_buffer + __offset; \ > + *(__typeof__(&(what)))__ptr = val; \ > + } while (0) > + > + set(kexec_start_address, image->start); > + set(kexec_indirection_page, page_list); > + set(kexec_mach_type, machine_arch_type); > + set(kexec_boot_atags, image->arch.kernel_r2); We could simplify this slightly if we moved the kexec_& variables into a struct (using asm-offset KEXEC_VAR_* offsets and a KEXEC_VAR_SIZE region reserved in the asm), then here we could do something like: static struct kexec_vars *kexec_buffer_vars(void *buffer) { unsigned long code = ((unisigned long)relocate_new_kernel) & ~1; unsigned long vars - (unsigned long)relocate_vars; unsigned long offset = vars - code; return buffer + offset; } ... and in machine_kexec() do: struct kexec_vars *kv = kexec_buffer_vars(reboot_code_buffer); kv->start_address = image->start; kv->indirection_page = page_list; kv->mach_type = machine-arch_type; kv->boot_atags = arch.kernel_r2; ... if that looks any better to you? Mark. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 13:57 ` Mark Rutland @ 2021-02-01 16:08 ` Russell King - ARM Linux admin 2021-02-01 16:32 ` Mark Rutland 2021-02-01 20:07 ` Giancarlo Ferrari 0 siblings, 2 replies; 22+ messages in thread From: Russell King - ARM Linux admin @ 2021-02-01 16:08 UTC (permalink / raw) To: Mark Rutland Cc: Giancarlo Ferrari, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari On Mon, Feb 01, 2021 at 01:57:14PM +0000, Mark Rutland wrote: > We could simplify this slightly if we moved the kexec_& variables into a > struct (using asm-offset KEXEC_VAR_* offsets and a KEXEC_VAR_SIZE region > reserved in the asm), then here we could do something like: > > static struct kexec_vars *kexec_buffer_vars(void *buffer) > { > unsigned long code = ((unisigned long)relocate_new_kernel) & ~1; > unsigned long vars - (unsigned long)relocate_vars; > unsigned long offset = vars - code; > > return buffer + offset; > } > > ... and in machine_kexec() do: > > struct kexec_vars *kv = kexec_buffer_vars(reboot_code_buffer); > > kv->start_address = image->start; > kv->indirection_page = page_list; > kv->mach_type = machine-arch_type; > kv->boot_atags = arch.kernel_r2; > > ... if that looks any better to you? Something like this? diff --git a/arch/arm/include/asm/kexec-internal.h b/arch/arm/include/asm/kexec-internal.h new file mode 100644 index 000000000000..ecc2322db7aa --- /dev/null +++ b/arch/arm/include/asm/kexec-internal.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ARM_KEXEC_INTERNAL_H +#define _ARM_KEXEC_INTERNAL_H + +struct kexec_relocate_data { + unsigned long kexec_start_address; + unsigned long kexec_indirection_page; + unsigned long kexec_mach_type; + unsigned long kexec_r2; +}; + +#endif diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c index a1570c8bab25..be8050b0c3df 100644 --- a/arch/arm/kernel/asm-offsets.c +++ b/arch/arm/kernel/asm-offsets.c @@ -12,6 +12,7 @@ #include <linux/mm.h> #include <linux/dma-mapping.h> #include <asm/cacheflush.h> +#include <asm/kexec-internal.h> #include <asm/glue-df.h> #include <asm/glue-pf.h> #include <asm/mach/arch.h> @@ -170,5 +171,9 @@ int main(void) DEFINE(MPU_RGN_PRBAR, offsetof(struct mpu_rgn, prbar)); DEFINE(MPU_RGN_PRLAR, offsetof(struct mpu_rgn, prlar)); #endif + DEFINE(KEXEC_START_ADDR, offsetof(struct kexec_relocate_data, kexec_start_address)); + DEFINE(KEXEC_INDIR_PAGE, offsetof(struct kexec_relocate_data, kexec_indirection_page)); + DEFINE(KEXEC_MACH_TYPE, offsetof(struct kexec_relocate_data, kexec_mach_type)); + DEFINE(KEXEC_R2, offsetof(struct kexec_relocate_data, kexec_r2)); return 0; } diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c index 5d84ad333f05..2b09dad7935e 100644 --- a/arch/arm/kernel/machine_kexec.c +++ b/arch/arm/kernel/machine_kexec.c @@ -13,6 +13,7 @@ #include <linux/of_fdt.h> #include <asm/mmu_context.h> #include <asm/cacheflush.h> +#include <asm/kexec-internal.h> #include <asm/fncpy.h> #include <asm/mach-types.h> #include <asm/smp_plat.h> @@ -22,11 +23,6 @@ extern void relocate_new_kernel(void); extern const unsigned int relocate_new_kernel_size; -extern unsigned long kexec_start_address; -extern unsigned long kexec_indirection_page; -extern unsigned long kexec_mach_type; -extern unsigned long kexec_boot_atags; - static atomic_t waiting_for_crash_ipi; /* @@ -159,6 +155,7 @@ void (*kexec_reinit)(void); void machine_kexec(struct kimage *image) { unsigned long page_list, reboot_entry_phys; + struct kexec_relocate_data *data; void (*reboot_entry)(void); void *reboot_code_buffer; @@ -174,18 +171,17 @@ void machine_kexec(struct kimage *image) reboot_code_buffer = page_address(image->control_code_page); - /* Prepare parameters for reboot_code_buffer*/ - set_kernel_text_rw(); - kexec_start_address = image->start; - kexec_indirection_page = page_list; - kexec_mach_type = machine_arch_type; - kexec_boot_atags = image->arch.kernel_r2; - /* copy our kernel relocation code to the control code page */ reboot_entry = fncpy(reboot_code_buffer, &relocate_new_kernel, relocate_new_kernel_size); + data = reboot_code_buffer + relocate_new_kernel_size; + data->kexec_start_address = image->start; + data->kexec_indirection_page = page_list; + data->kexec_mach_type = machine_arch_type; + data->kexec_r2 = image->arch.kernel_r2; + /* get the identity mapping physical address for the reboot code */ reboot_entry_phys = virt_to_idmap(reboot_entry); diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S index 72a08786e16e..218d524360fc 100644 --- a/arch/arm/kernel/relocate_kernel.S +++ b/arch/arm/kernel/relocate_kernel.S @@ -5,14 +5,16 @@ #include <linux/linkage.h> #include <asm/assembler.h> +#include <asm/asm-offsets.h> #include <asm/kexec.h> .align 3 /* not needed for this code, but keeps fncpy() happy */ ENTRY(relocate_new_kernel) - ldr r0,kexec_indirection_page - ldr r1,kexec_start_address + adr r7, relocate_new_kernel_end + ldr r0, [r7, #KEXEC_INDIR_PAGE] + ldr r1, [r7, #KEXEC_START_ADDR] /* * If there is no indirection page (we are doing crashdumps) @@ -57,34 +59,16 @@ ENTRY(relocate_new_kernel) 2: /* Jump to relocated kernel */ - mov lr,r1 - mov r0,#0 - ldr r1,kexec_mach_type - ldr r2,kexec_boot_atags - ARM( ret lr ) - THUMB( bx lr ) - - .align - - .globl kexec_start_address -kexec_start_address: - .long 0x0 - - .globl kexec_indirection_page -kexec_indirection_page: - .long 0x0 - - .globl kexec_mach_type -kexec_mach_type: - .long 0x0 - - /* phy addr of the atags for the new kernel */ - .globl kexec_boot_atags -kexec_boot_atags: - .long 0x0 + mov lr, r1 + mov r0, #0 + ldr r1, [r7, #KEXEC_MACH_TYPE] + ldr r2, [r7, #KEXEC_R2] + ARM( ret lr ) + THUMB( bx lr ) ENDPROC(relocate_new_kernel) + .align 3 relocate_new_kernel_end: .globl relocate_new_kernel_size -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 16:08 ` Russell King - ARM Linux admin @ 2021-02-01 16:32 ` Mark Rutland 2021-02-01 16:37 ` Russell King - ARM Linux admin 2021-02-01 20:07 ` Giancarlo Ferrari 1 sibling, 1 reply; 22+ messages in thread From: Mark Rutland @ 2021-02-01 16:32 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Giancarlo Ferrari, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari On Mon, Feb 01, 2021 at 04:08:38PM +0000, Russell King - ARM Linux admin wrote: > On Mon, Feb 01, 2021 at 01:57:14PM +0000, Mark Rutland wrote: > > We could simplify this slightly if we moved the kexec_& variables into a > > struct (using asm-offset KEXEC_VAR_* offsets and a KEXEC_VAR_SIZE region > > reserved in the asm), then here we could do something like: > > > > static struct kexec_vars *kexec_buffer_vars(void *buffer) > > { > > unsigned long code = ((unisigned long)relocate_new_kernel) & ~1; > > unsigned long vars - (unsigned long)relocate_vars; > > unsigned long offset = vars - code; > > > > return buffer + offset; > > } > > > > ... and in machine_kexec() do: > > > > struct kexec_vars *kv = kexec_buffer_vars(reboot_code_buffer); > > > > kv->start_address = image->start; > > kv->indirection_page = page_list; > > kv->mach_type = machine-arch_type; > > kv->boot_atags = arch.kernel_r2; > > > > ... if that looks any better to you? > > Something like this? Nice! That looks about right to me, modulo a bit of cache maintenance noted below. > diff --git a/arch/arm/include/asm/kexec-internal.h b/arch/arm/include/asm/kexec-internal.h > new file mode 100644 > index 000000000000..ecc2322db7aa > --- /dev/null > +++ b/arch/arm/include/asm/kexec-internal.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ARM_KEXEC_INTERNAL_H > +#define _ARM_KEXEC_INTERNAL_H > + > +struct kexec_relocate_data { > + unsigned long kexec_start_address; > + unsigned long kexec_indirection_page; > + unsigned long kexec_mach_type; > + unsigned long kexec_r2; > +}; > + > +#endif > diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c > index a1570c8bab25..be8050b0c3df 100644 > --- a/arch/arm/kernel/asm-offsets.c > +++ b/arch/arm/kernel/asm-offsets.c > @@ -12,6 +12,7 @@ > #include <linux/mm.h> > #include <linux/dma-mapping.h> > #include <asm/cacheflush.h> > +#include <asm/kexec-internal.h> > #include <asm/glue-df.h> > #include <asm/glue-pf.h> > #include <asm/mach/arch.h> > @@ -170,5 +171,9 @@ int main(void) > DEFINE(MPU_RGN_PRBAR, offsetof(struct mpu_rgn, prbar)); > DEFINE(MPU_RGN_PRLAR, offsetof(struct mpu_rgn, prlar)); > #endif > + DEFINE(KEXEC_START_ADDR, offsetof(struct kexec_relocate_data, kexec_start_address)); > + DEFINE(KEXEC_INDIR_PAGE, offsetof(struct kexec_relocate_data, kexec_indirection_page)); > + DEFINE(KEXEC_MACH_TYPE, offsetof(struct kexec_relocate_data, kexec_mach_type)); > + DEFINE(KEXEC_R2, offsetof(struct kexec_relocate_data, kexec_r2)); > return 0; > } > diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c > index 5d84ad333f05..2b09dad7935e 100644 > --- a/arch/arm/kernel/machine_kexec.c > +++ b/arch/arm/kernel/machine_kexec.c > @@ -13,6 +13,7 @@ > #include <linux/of_fdt.h> > #include <asm/mmu_context.h> > #include <asm/cacheflush.h> > +#include <asm/kexec-internal.h> > #include <asm/fncpy.h> > #include <asm/mach-types.h> > #include <asm/smp_plat.h> > @@ -22,11 +23,6 @@ > extern void relocate_new_kernel(void); > extern const unsigned int relocate_new_kernel_size; > > -extern unsigned long kexec_start_address; > -extern unsigned long kexec_indirection_page; > -extern unsigned long kexec_mach_type; > -extern unsigned long kexec_boot_atags; > - > static atomic_t waiting_for_crash_ipi; > > /* > @@ -159,6 +155,7 @@ void (*kexec_reinit)(void); > void machine_kexec(struct kimage *image) > { > unsigned long page_list, reboot_entry_phys; > + struct kexec_relocate_data *data; > void (*reboot_entry)(void); > void *reboot_code_buffer; > > @@ -174,18 +171,17 @@ void machine_kexec(struct kimage *image) > > reboot_code_buffer = page_address(image->control_code_page); > > - /* Prepare parameters for reboot_code_buffer*/ > - set_kernel_text_rw(); > - kexec_start_address = image->start; > - kexec_indirection_page = page_list; > - kexec_mach_type = machine_arch_type; > - kexec_boot_atags = image->arch.kernel_r2; > - > /* copy our kernel relocation code to the control code page */ > reboot_entry = fncpy(reboot_code_buffer, > &relocate_new_kernel, > relocate_new_kernel_size); > > + data = reboot_code_buffer + relocate_new_kernel_size; > + data->kexec_start_address = image->start; > + data->kexec_indirection_page = page_list; > + data->kexec_mach_type = machine_arch_type; > + data->kexec_r2 = image->arch.kernel_r2; I reckon here we need: __cpuc_flush_dcache_area(reboot_code_buffer, relocate_new_kernel_size + sizeof(*data)); ... to make sure both the instructions and data are visible with the MMU off (since fncpy() only cleans to the PoU, not the PoC). Otherwise this all looks good to me. Mark. > + > /* get the identity mapping physical address for the reboot code */ > reboot_entry_phys = virt_to_idmap(reboot_entry); > > diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S > index 72a08786e16e..218d524360fc 100644 > --- a/arch/arm/kernel/relocate_kernel.S > +++ b/arch/arm/kernel/relocate_kernel.S > @@ -5,14 +5,16 @@ > > #include <linux/linkage.h> > #include <asm/assembler.h> > +#include <asm/asm-offsets.h> > #include <asm/kexec.h> > > .align 3 /* not needed for this code, but keeps fncpy() happy */ > > ENTRY(relocate_new_kernel) > > - ldr r0,kexec_indirection_page > - ldr r1,kexec_start_address > + adr r7, relocate_new_kernel_end > + ldr r0, [r7, #KEXEC_INDIR_PAGE] > + ldr r1, [r7, #KEXEC_START_ADDR] > > /* > * If there is no indirection page (we are doing crashdumps) > @@ -57,34 +59,16 @@ ENTRY(relocate_new_kernel) > > 2: > /* Jump to relocated kernel */ > - mov lr,r1 > - mov r0,#0 > - ldr r1,kexec_mach_type > - ldr r2,kexec_boot_atags > - ARM( ret lr ) > - THUMB( bx lr ) > - > - .align > - > - .globl kexec_start_address > -kexec_start_address: > - .long 0x0 > - > - .globl kexec_indirection_page > -kexec_indirection_page: > - .long 0x0 > - > - .globl kexec_mach_type > -kexec_mach_type: > - .long 0x0 > - > - /* phy addr of the atags for the new kernel */ > - .globl kexec_boot_atags > -kexec_boot_atags: > - .long 0x0 > + mov lr, r1 > + mov r0, #0 > + ldr r1, [r7, #KEXEC_MACH_TYPE] > + ldr r2, [r7, #KEXEC_R2] > + ARM( ret lr ) > + THUMB( bx lr ) > > ENDPROC(relocate_new_kernel) > > + .align 3 > relocate_new_kernel_end: > > .globl relocate_new_kernel_size > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 16:32 ` Mark Rutland @ 2021-02-01 16:37 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 22+ messages in thread From: Russell King - ARM Linux admin @ 2021-02-01 16:37 UTC (permalink / raw) To: Mark Rutland Cc: Giancarlo Ferrari, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari On Mon, Feb 01, 2021 at 04:32:40PM +0000, Mark Rutland wrote: > I reckon here we need: > > __cpuc_flush_dcache_area(reboot_code_buffer, > relocate_new_kernel_size + sizeof(*data)); > > ... to make sure both the instructions and data are visible with the MMU > off (since fncpy() only cleans to the PoU, not the PoC). We don't. When soft_restart() turns the MMU off, and it calls flush_cache_all() before and afterwards to ensure that all dirty lines are pushed out. The flushing to PoU in fncpy() is to cover other cases. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 16:08 ` Russell King - ARM Linux admin 2021-02-01 16:32 ` Mark Rutland @ 2021-02-01 20:07 ` Giancarlo Ferrari 2021-02-01 20:16 ` Russell King - ARM Linux admin 1 sibling, 1 reply; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-01 20:07 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Mark Rutland, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari Hi, On Mon, Feb 01, 2021 at 04:08:38PM +0000, Russell King - ARM Linux admin wrote: > On Mon, Feb 01, 2021 at 01:57:14PM +0000, Mark Rutland wrote: > > We could simplify this slightly if we moved the kexec_& variables into a > > struct (using asm-offset KEXEC_VAR_* offsets and a KEXEC_VAR_SIZE region > > reserved in the asm), then here we could do something like: > > > > static struct kexec_vars *kexec_buffer_vars(void *buffer) > > { > > unsigned long code = ((unisigned long)relocate_new_kernel) & ~1; > > unsigned long vars - (unsigned long)relocate_vars; > > unsigned long offset = vars - code; > > > > return buffer + offset; > > } > > > > ... and in machine_kexec() do: > > > > struct kexec_vars *kv = kexec_buffer_vars(reboot_code_buffer); > > > > kv->start_address = image->start; > > kv->indirection_page = page_list; > > kv->mach_type = machine-arch_type; > > kv->boot_atags = arch.kernel_r2; > > > > ... if that looks any better to you? > > Something like this? > > diff --git a/arch/arm/include/asm/kexec-internal.h b/arch/arm/include/asm/kexec-internal.h > new file mode 100644 > index 000000000000..ecc2322db7aa > --- /dev/null > +++ b/arch/arm/include/asm/kexec-internal.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ARM_KEXEC_INTERNAL_H > +#define _ARM_KEXEC_INTERNAL_H > + > +struct kexec_relocate_data { > + unsigned long kexec_start_address; > + unsigned long kexec_indirection_page; > + unsigned long kexec_mach_type; > + unsigned long kexec_r2; > +}; > + > +#endif > diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c > index a1570c8bab25..be8050b0c3df 100644 > --- a/arch/arm/kernel/asm-offsets.c > +++ b/arch/arm/kernel/asm-offsets.c > @@ -12,6 +12,7 @@ > #include <linux/mm.h> > #include <linux/dma-mapping.h> > #include <asm/cacheflush.h> > +#include <asm/kexec-internal.h> > #include <asm/glue-df.h> > #include <asm/glue-pf.h> > #include <asm/mach/arch.h> > @@ -170,5 +171,9 @@ int main(void) > DEFINE(MPU_RGN_PRBAR, offsetof(struct mpu_rgn, prbar)); > DEFINE(MPU_RGN_PRLAR, offsetof(struct mpu_rgn, prlar)); > #endif > + DEFINE(KEXEC_START_ADDR, offsetof(struct kexec_relocate_data, kexec_start_address)); > + DEFINE(KEXEC_INDIR_PAGE, offsetof(struct kexec_relocate_data, kexec_indirection_page)); > + DEFINE(KEXEC_MACH_TYPE, offsetof(struct kexec_relocate_data, kexec_mach_type)); > + DEFINE(KEXEC_R2, offsetof(struct kexec_relocate_data, kexec_r2)); > return 0; > } > diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c > index 5d84ad333f05..2b09dad7935e 100644 > --- a/arch/arm/kernel/machine_kexec.c > +++ b/arch/arm/kernel/machine_kexec.c > @@ -13,6 +13,7 @@ > #include <linux/of_fdt.h> > #include <asm/mmu_context.h> > #include <asm/cacheflush.h> > +#include <asm/kexec-internal.h> > #include <asm/fncpy.h> > #include <asm/mach-types.h> > #include <asm/smp_plat.h> > @@ -22,11 +23,6 @@ > extern void relocate_new_kernel(void); > extern const unsigned int relocate_new_kernel_size; > > -extern unsigned long kexec_start_address; > -extern unsigned long kexec_indirection_page; > -extern unsigned long kexec_mach_type; > -extern unsigned long kexec_boot_atags; > - > static atomic_t waiting_for_crash_ipi; > > /* > @@ -159,6 +155,7 @@ void (*kexec_reinit)(void); > void machine_kexec(struct kimage *image) > { > unsigned long page_list, reboot_entry_phys; > + struct kexec_relocate_data *data; > void (*reboot_entry)(void); > void *reboot_code_buffer; > > @@ -174,18 +171,17 @@ void machine_kexec(struct kimage *image) > > reboot_code_buffer = page_address(image->control_code_page); > > - /* Prepare parameters for reboot_code_buffer*/ > - set_kernel_text_rw(); > - kexec_start_address = image->start; > - kexec_indirection_page = page_list; > - kexec_mach_type = machine_arch_type; > - kexec_boot_atags = image->arch.kernel_r2; > - > /* copy our kernel relocation code to the control code page */ > reboot_entry = fncpy(reboot_code_buffer, > &relocate_new_kernel, > relocate_new_kernel_size); > > + data = reboot_code_buffer + relocate_new_kernel_size; > + data->kexec_start_address = image->start; > + data->kexec_indirection_page = page_list; > + data->kexec_mach_type = machine_arch_type; > + data->kexec_r2 = image->arch.kernel_r2; > + > /* get the identity mapping physical address for the reboot code */ > reboot_entry_phys = virt_to_idmap(reboot_entry); > > diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S > index 72a08786e16e..218d524360fc 100644 > --- a/arch/arm/kernel/relocate_kernel.S > +++ b/arch/arm/kernel/relocate_kernel.S > @@ -5,14 +5,16 @@ > > #include <linux/linkage.h> > #include <asm/assembler.h> > +#include <asm/asm-offsets.h> > #include <asm/kexec.h> > > .align 3 /* not needed for this code, but keeps fncpy() happy */ > > ENTRY(relocate_new_kernel) > > - ldr r0,kexec_indirection_page > - ldr r1,kexec_start_address > + adr r7, relocate_new_kernel_end > + ldr r0, [r7, #KEXEC_INDIR_PAGE] > + ldr r1, [r7, #KEXEC_START_ADDR] > > /* > * If there is no indirection page (we are doing crashdumps) > @@ -57,34 +59,16 @@ ENTRY(relocate_new_kernel) > > 2: > /* Jump to relocated kernel */ > - mov lr,r1 > - mov r0,#0 > - ldr r1,kexec_mach_type > - ldr r2,kexec_boot_atags > - ARM( ret lr ) > - THUMB( bx lr ) > - > - .align > - > - .globl kexec_start_address > -kexec_start_address: > - .long 0x0 > - > - .globl kexec_indirection_page > -kexec_indirection_page: > - .long 0x0 > - > - .globl kexec_mach_type > -kexec_mach_type: > - .long 0x0 > - > - /* phy addr of the atags for the new kernel */ > - .globl kexec_boot_atags > -kexec_boot_atags: > - .long 0x0 > + mov lr, r1 > + mov r0, #0 > + ldr r1, [r7, #KEXEC_MACH_TYPE] > + ldr r2, [r7, #KEXEC_R2] > + ARM( ret lr ) > + THUMB( bx lr ) > > ENDPROC(relocate_new_kernel) > > + .align 3 Nice. Why we should align 3 ? For the fncpy I suppose. > relocate_new_kernel_end: > > .globl relocate_new_kernel_size > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! I don't know now how to proceed now, as you (Mark and you) do completely the patch. You see is my first kernel patch submission :) . Thanks, GF ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 20:07 ` Giancarlo Ferrari @ 2021-02-01 20:16 ` Russell King - ARM Linux admin 2021-02-01 22:18 ` Giancarlo Ferrari 0 siblings, 1 reply; 22+ messages in thread From: Russell King - ARM Linux admin @ 2021-02-01 20:16 UTC (permalink / raw) To: Giancarlo Ferrari Cc: Mark Rutland, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari On Mon, Feb 01, 2021 at 08:07:37PM +0000, Giancarlo Ferrari wrote: > Hi, Hi, > Why we should align 3 ? For the fncpy I suppose. Slightly arbitary really - it gives a nice 8-byte alignment to the data. .align 2 would also be sufficient. > I don't know now how to proceed now, as you (Mark and you) do completely > the patch. Please can you test my patch and let us know if it solves your problem (or even if it works! I haven't tested it beyond build-testing.) > You see is my first kernel patch submission :) . Yay. Sorry for giving you a different patch - Mark is quite right that there's a better solution to this problem, which is eliminating the set_kernel_text_rw() call. The only reason I cooked up the patch was doing that would be more in-depth (as you can see from the increased size of the patch.) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 20:16 ` Russell King - ARM Linux admin @ 2021-02-01 22:18 ` Giancarlo Ferrari 2021-02-04 23:48 ` Giancarlo Ferrari 0 siblings, 1 reply; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-01 22:18 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Mark Rutland, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari Russell, On Mon, Feb 01, 2021 at 08:16:33PM +0000, Russell King - ARM Linux admin wrote: > On Mon, Feb 01, 2021 at 08:07:37PM +0000, Giancarlo Ferrari wrote: > > Hi, > > Hi, > > > Why we should align 3 ? For the fncpy I suppose. > > Slightly arbitary really - it gives a nice 8-byte alignment to the data. > .align 2 would also be sufficient. > > > I don't know now how to proceed now, as you (Mark and you) do completely > > the patch. > > Please can you test my patch and let us know if it solves your problem > (or even if it works! I haven't tested it beyond build-testing.) > sure, unfortunately due to restriction, I hope to test it by the end of the week. I hope there will be no rush. Otherwise, please let me know. > > You see is my first kernel patch submission :) . > > Yay. Sorry for giving you a different patch - Mark is quite right that > there's a better solution to this problem, which is eliminating the > set_kernel_text_rw() call. The only reason I cooked up the patch was > doing that would be more in-depth (as you can see from the increased > size of the patch.) > I definitely agree with you. > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! Thanks again, GF ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 22:18 ` Giancarlo Ferrari @ 2021-02-04 23:48 ` Giancarlo Ferrari 2021-02-05 0:18 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-04 23:48 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Mark Rutland, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari Hi all, On Mon, Feb 01, 2021 at 10:18:26PM +0000, Giancarlo Ferrari wrote: > Russell, > > On Mon, Feb 01, 2021 at 08:16:33PM +0000, Russell King - ARM Linux admin wrote: > > On Mon, Feb 01, 2021 at 08:07:37PM +0000, Giancarlo Ferrari wrote: > > > Hi, > > > > Hi, > > > > > Why we should align 3 ? For the fncpy I suppose. > > > > Slightly arbitary really - it gives a nice 8-byte alignment to the data. > > .align 2 would also be sufficient. > > > > > I don't know now how to proceed now, as you (Mark and you) do completely > > > the patch. > > > > Please can you test my patch and let us know if it solves your problem > > (or even if it works! I haven't tested it beyond build-testing.) > > I have tested your patch and it works. I have tested using kexec -l/kexec -e. Considering that .text var are no more written, I assume the issue is gone. > > sure, unfortunately due to restriction, I hope to test it by the end of the > week. I hope there will be no rush. Otherwise, please let me know. > > > > You see is my first kernel patch submission :) . > > > > Yay. Sorry for giving you a different patch - Mark is quite right that > > there's a better solution to this problem, which is eliminating the > > set_kernel_text_rw() call. The only reason I cooked up the patch was > > doing that would be more in-depth (as you can see from the increased > > size of the patch.) > > > > I definitely agree with you. > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! > > Thanks again, > > > GF Can I ask about having it integrated ? Thanks in advance, GF ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-04 23:48 ` Giancarlo Ferrari @ 2021-02-05 0:18 ` Russell King - ARM Linux admin 2021-02-05 0:40 ` Giancarlo Ferrari 0 siblings, 1 reply; 22+ messages in thread From: Russell King - ARM Linux admin @ 2021-02-05 0:18 UTC (permalink / raw) To: Giancarlo Ferrari Cc: Mark Rutland, linux-kernel, penberg, geert, rppt, akpm, linux-arm-kernel, giancarlo.ferrari On Thu, Feb 04, 2021 at 11:48:42PM +0000, Giancarlo Ferrari wrote: > Can I ask about having it integrated ? Thanks for testing. Are you willing for me to add: Tested-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> to the commit log? I can move it into the fixes branch which I want to send to Linus by Saturday at the very latest. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-05 0:18 ` Russell King - ARM Linux admin @ 2021-02-05 0:40 ` Giancarlo Ferrari 2021-02-05 0:45 ` Giancarlo Ferrari 2021-02-05 9:44 ` Russell King - ARM Linux admin 0 siblings, 2 replies; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-05 0:40 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Mark Rutland, linux-kernel, penberg, geert, rppt, akpm, linux-arm-kernel, giancarlo.ferrari Russell, On Fri, Feb 05, 2021 at 12:18:33AM +0000, Russell King - ARM Linux admin wrote: > On Thu, Feb 04, 2021 at 11:48:42PM +0000, Giancarlo Ferrari wrote: > > Can I ask about having it integrated ? > > Thanks for testing. Are you willing for me to add: > > Tested-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> > > to the commit log? > Sure. I have a question regarding the patch. I saw that the structure start at the end of the relocation code: data = reboot_code_buffer + relocate_new_kernel_size; which means it overlap with the global symbol relocate_new_kernel_size. I think is minor comment as the variable is only used in the fncpy() then thrown away. Something like: data = reboot_code_buffer + relocate_new_kernel_size + sizeof(long); and accordingly in the instruction (arch/arm/kernel/relocate_kernel.S): adr r7, relocate_new_kernel_end > I can move it into the fixes branch which I want to send to Linus by > Saturday at the very latest. > I am not used with all the merging process. However it sounds great ! > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! Thanks in advance, GF ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-05 0:40 ` Giancarlo Ferrari @ 2021-02-05 0:45 ` Giancarlo Ferrari 2021-02-05 9:44 ` Russell King - ARM Linux admin 1 sibling, 0 replies; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-05 0:45 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Mark Rutland, linux-kernel, penberg, geert, rppt, akpm, linux-arm-kernel, giancarlo.ferrari Sorry for polluting, On Fri, Feb 05, 2021 at 12:40:54AM +0000, Giancarlo Ferrari wrote: > Russell, > > On Fri, Feb 05, 2021 at 12:18:33AM +0000, Russell King - ARM Linux admin wrote: > > On Thu, Feb 04, 2021 at 11:48:42PM +0000, Giancarlo Ferrari wrote: > > > Can I ask about having it integrated ? > > > > Thanks for testing. Are you willing for me to add: > > > > Tested-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> > > > > to the commit log? > > > > Sure. > > I have a question regarding the patch. I saw that the structure start at > the end of the relocation code: > > data = reboot_code_buffer + relocate_new_kernel_size; > > which means it overlap with the global symbol relocate_new_kernel_size. > I think is minor comment as the variable is only used in the fncpy() > then thrown away. Something like: > > data = reboot_code_buffer + relocate_new_kernel_size + sizeof(long); > > and accordingly in the instruction (arch/arm/kernel/relocate_kernel.S): > > adr r7, relocate_new_kernel_end > my fault. Do as I didn't talk... one is in virtual memory the other in the initial code. > > I can move it into the fixes branch which I want to send to Linus by > > Saturday at the very latest. > > > > I am not used with all the merging process. However it sounds great ! > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! > > Thanks in advance, > > > GF Thanks, GF ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-05 0:40 ` Giancarlo Ferrari 2021-02-05 0:45 ` Giancarlo Ferrari @ 2021-02-05 9:44 ` Russell King - ARM Linux admin 2021-02-05 14:36 ` Giancarlo Ferrari 1 sibling, 1 reply; 22+ messages in thread From: Russell King - ARM Linux admin @ 2021-02-05 9:44 UTC (permalink / raw) To: Giancarlo Ferrari Cc: Mark Rutland, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari On Fri, Feb 05, 2021 at 12:40:54AM +0000, Giancarlo Ferrari wrote: > Russell, > > On Fri, Feb 05, 2021 at 12:18:33AM +0000, Russell King - ARM Linux admin wrote: > > On Thu, Feb 04, 2021 at 11:48:42PM +0000, Giancarlo Ferrari wrote: > > > Can I ask about having it integrated ? > > > > Thanks for testing. Are you willing for me to add: > > > > Tested-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> > > > > to the commit log? > > > > Sure. > > I have a question regarding the patch. I saw that the structure start at > the end of the relocation code: > > data = reboot_code_buffer + relocate_new_kernel_size; > > which means it overlap with the global symbol relocate_new_kernel_size. > I think is minor comment as the variable is only used in the fncpy() > then thrown away. The same is true of the rest of the kernel image if that's how you'd like to look at it. relocate_new_kernel_size is just there to tell the C code the size of the function, nothing more nothing less. It isn't there to be copied. The copied code doesn't use it. > Something like: > > data = reboot_code_buffer + relocate_new_kernel_size + sizeof(long); No. That will place the structure after the size variable, which we don't want, and... > and accordingly in the instruction (arch/arm/kernel/relocate_kernel.S): > > adr r7, relocate_new_kernel_end ... we will then need to follow this with: add r7, r7, #4 or replace it with: adr r7, relocate_new_kernel_end + 4 so that r7 points at "data". -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-05 9:44 ` Russell King - ARM Linux admin @ 2021-02-05 14:36 ` Giancarlo Ferrari 0 siblings, 0 replies; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-05 14:36 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Mark Rutland, linux-kernel, penberg, geert, linux-arm-kernel, akpm, rppt, giancarlo.ferrari On Fri, Feb 05, 2021 at 09:44:59AM +0000, Russell King - ARM Linux admin wrote: > On Fri, Feb 05, 2021 at 12:40:54AM +0000, Giancarlo Ferrari wrote: > > Russell, > > > > On Fri, Feb 05, 2021 at 12:18:33AM +0000, Russell King - ARM Linux admin wrote: > > > On Thu, Feb 04, 2021 at 11:48:42PM +0000, Giancarlo Ferrari wrote: > > > > Can I ask about having it integrated ? > > > > > > Thanks for testing. Are you willing for me to add: > > > > > > Tested-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> > > > > > > to the commit log? > > > > > > > Sure. > > > > I have a question regarding the patch. I saw that the structure start at > > the end of the relocation code: > > > > data = reboot_code_buffer + relocate_new_kernel_size; > > > > which means it overlap with the global symbol relocate_new_kernel_size. > > I think is minor comment as the variable is only used in the fncpy() > > then thrown away. > > The same is true of the rest of the kernel image if that's how you'd > like to look at it. > > relocate_new_kernel_size is just there to tell the C code the size of > the function, nothing more nothing less. It isn't there to be copied. > The copied code doesn't use it. > Yes, clear. > > Something like: > > > > data = reboot_code_buffer + relocate_new_kernel_size + sizeof(long); > > No. That will place the structure after the size variable, which we > don't want, and... > > > and accordingly in the instruction (arch/arm/kernel/relocate_kernel.S): > > > > adr r7, relocate_new_kernel_end > > ... we will then need to follow this with: > add r7, r7, #4 > > or replace it with: > adr r7, relocate_new_kernel_end + 4 > > so that r7 points at "data". > It was what I meant with "accordingly". > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! Regards, GF ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 12:47 ` Mark Rutland 2021-02-01 13:03 ` Russell King - ARM Linux admin @ 2021-02-01 14:39 ` Giancarlo Ferrari 2021-02-01 15:30 ` Mark Rutland 1 sibling, 1 reply; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-01 14:39 UTC (permalink / raw) To: Mark Rutland Cc: linux-arm-kernel, linux, linux-kernel, akpm, rppt, penberg, geert, giancarlo.ferrari Hi, On Mon, Feb 01, 2021 at 12:47:20PM +0000, Mark Rutland wrote: > On Mon, Feb 01, 2021 at 12:44:56AM +0000, Giancarlo Ferrari wrote: > > machine_kexec() need to set rw permission in text and rodata sections > > to assign some variables (e.g. kexec_start_address). To do that at > > the end (after flushing pdm in memory, etc.) it needs to invalidate > > TLB [section] entries. > > It'd be worth noting explicitly that set_kernel_text_rw() alters > current->active_mm... > > > If during the TLB invalidation an interrupt occours, which might cause > > a context switch, there is the risk to inject invalid TLBs, with ro > > permissions. > > ... which is why if there's a context switch things can go wrong, since > active_mm isn't stable, and so it's possible that set_kernel_text_rw() > updates multiple tables, none of which might be the active table at the > point we try to make an access. > Maybe the behaviour causing issue is not completely clear to me, and I do apologize for that (moreover I haven't eougth debug capabilities). However, current-active_mm is switched among context switches. Correct ? So, in principle, the invalidation, if stopped, is carried on where it left. I thought the issue was that the PageTable entry for the section 0x8010_0000 is global, thus not indexed by ASID (Address Space ID). By the fact that each process has its own version of that entry, is the cause of the issue, as the schedule process might bringing a spurious entry (with ro permission) in the MMU cache. If the entry is not global holds the ASID, and the issue cannot happen. Please note that this behaviour was tested on a armv7 arch board. > It would be nice to spell that out rather than saying "invalid TLBs". > > We could disable preemption to prevent that, which is possibly better > than disabling interrupts. > > Overall, it would be much better to avoid having to mess with the kernel > page tables. So rather than going: > > 1. mark kernel RW > 2. alter variables in reloc code > 3. copy reloc code into buffer > 4. branch to buffer > > ... we should be able to go: > > 1. copy reloc code into buffer > 2. alter variables in copy of reloc code > 3. branch to buffer > > ... which would avoid this class of problem too. > > Thanks, > Mark. Thanks, GF ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 14:39 ` Giancarlo Ferrari @ 2021-02-01 15:30 ` Mark Rutland 2021-02-01 19:09 ` Giancarlo Ferrari 0 siblings, 1 reply; 22+ messages in thread From: Mark Rutland @ 2021-02-01 15:30 UTC (permalink / raw) To: Giancarlo Ferrari Cc: linux-arm-kernel, linux, linux-kernel, akpm, rppt, penberg, geert, giancarlo.ferrari Hi, On Mon, Feb 01, 2021 at 02:39:46PM +0000, Giancarlo Ferrari wrote: > On Mon, Feb 01, 2021 at 12:47:20PM +0000, Mark Rutland wrote: > > On Mon, Feb 01, 2021 at 12:44:56AM +0000, Giancarlo Ferrari wrote: > > > machine_kexec() need to set rw permission in text and rodata sections > > > to assign some variables (e.g. kexec_start_address). To do that at > > > the end (after flushing pdm in memory, etc.) it needs to invalidate > > > TLB [section] entries. > > > > It'd be worth noting explicitly that set_kernel_text_rw() alters > > current->active_mm... > > > > > If during the TLB invalidation an interrupt occours, which might cause > > > a context switch, there is the risk to inject invalid TLBs, with ro > > > permissions. > > > > ... which is why if there's a context switch things can go wrong, since > > active_mm isn't stable, and so it's possible that set_kernel_text_rw() > > updates multiple tables, none of which might be the active table at the > > point we try to make an access. > > Maybe the behaviour causing issue is not completely clear to me, and I do > apologize for that (moreover I haven't eougth debug capabilities). I think we're in rough agreement that the issue is likely related to the context switch, but our understanding of the specifics differs (and I think we're missing a detail here). > However, current-active_mm is switched among context switches. Correct ? In some cases current->active_mm is not switched, and can be inherited over a context switch. When switching to a user task, we always switch to its mm (which becomes the active_mm), but when switching to a kthread we retain the previous task's mm as the active_mm as part of the lazy context switch. So while a kthread is preemptible, its active_mm (and active ASID) can change under its feet. That could happen anywhere while the task is preemptible, e.g. in the middle of set_kernel_text_rw(), or mid-modification to the kexec variables. > So, in principle, the invalidation, if stopped, is carried on where it > left. That's true so long as all the processes we switch between share the same leaf tables for the region we're altering. If not, then the lazy context switch means that those tables can change under our feet. I believe the tables mapping the kernel text are shared by all threads, and if so this _should_ be true. Russell might be able to confirm that or correct me if I have that wrong. > I thought the issue was that the PageTable entry for the section 0x8010_0000 > is global, thus not indexed by ASID (Address Space ID). By the fact that each > process has its own version of that entry, is the cause of the issue, as the > schedule process might bringing a spurious entry (with ro permission) in the > MMU cache. The TLB invalidation performed under set_kernel_text_rw() affects all ASIDs on the current CPU, so there shouldn't be any stale RO TLB entries to hit unless the kthread is migrated to another CPU. > If the entry is not global holds the ASID, and the issue cannot happen. I don't think that's true, since switching to a different active_mm would also change ASID, and we'd have no additional guarantee. Thanks, Mark. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-02-01 15:30 ` Mark Rutland @ 2021-02-01 19:09 ` Giancarlo Ferrari 0 siblings, 0 replies; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-01 19:09 UTC (permalink / raw) To: Mark Rutland Cc: linux-arm-kernel, linux, linux-kernel, akpm, rppt, penberg, geert, giancarlo.ferrari Hi, On Mon, Feb 01, 2021 at 03:30:12PM +0000, Mark Rutland wrote: > Hi, > > On Mon, Feb 01, 2021 at 02:39:46PM +0000, Giancarlo Ferrari wrote: > > On Mon, Feb 01, 2021 at 12:47:20PM +0000, Mark Rutland wrote: > > > On Mon, Feb 01, 2021 at 12:44:56AM +0000, Giancarlo Ferrari wrote: > > > > machine_kexec() need to set rw permission in text and rodata sections > > > > to assign some variables (e.g. kexec_start_address). To do that at > > > > the end (after flushing pdm in memory, etc.) it needs to invalidate > > > > TLB [section] entries. > > > > > > It'd be worth noting explicitly that set_kernel_text_rw() alters > > > current->active_mm... > > > > > > > If during the TLB invalidation an interrupt occours, which might cause > > > > a context switch, there is the risk to inject invalid TLBs, with ro > > > > permissions. > > > > > > ... which is why if there's a context switch things can go wrong, since > > > active_mm isn't stable, and so it's possible that set_kernel_text_rw() > > > updates multiple tables, none of which might be the active table at the > > > point we try to make an access. > > > > Maybe the behaviour causing issue is not completely clear to me, and I do > > apologize for that (moreover I haven't eougth debug capabilities). > > I think we're in rough agreement that the issue is likely related to the > context switch, but our understanding of the specifics differs (and I > think we're missing a detail here). > > > However, current-active_mm is switched among context switches. Correct ? > > In some cases current->active_mm is not switched, and can be inherited > over a context switch. When switching to a user task, we always switch > to its mm (which becomes the active_mm), but when switching to a kthread > we retain the previous task's mm as the active_mm as part of the lazy > context switch. > > So while a kthread is preemptible, its active_mm (and active ASID) can > change under its feet. That could happen anywhere while the task is > preemptible, e.g. in the middle of set_kernel_text_rw(), or > mid-modification to the kexec variables. > Yes. In my understanding, even in the case of user process, when current->active_mm is switched, we can run into trouble. For instance: - Process A issue kexec (PageTables entry of A has 0x8000_0000-0x8010_0000 with ro permission and section is global, no NG bit set). - A context switch happens in the middle of set_kernel_text_rw(), right after the section 0x8000_0000-0x8010_0000 has been invalidated. - Process B, in its execution, re-inject its own PageTable entry with ro permission, which is not shared with Process A (and is not touched by the previous invalidation) in the MMU cache. - When Process A, is rescheduled, it carries on with the invalidation, but unfortunately I have "wrong" entries in the MMU. > > So, in principle, the invalidation, if stopped, is carried on where it > > left. > > That's true so long as all the processes we switch between share the > same leaf tables for the region we're altering. If not, then the lazy > context switch means that those tables can change under our feet. > > I believe the tables mapping the kernel text are shared by all threads, > and if so this _should_ be true. Russell might be able to confirm that > or correct me if I have that wrong. > I am not ready to put my hand on the fire, but I seen the behaviour described above. > > I thought the issue was that the PageTable entry for the section 0x8010_0000 > > is global, thus not indexed by ASID (Address Space ID). By the fact that each > > process has its own version of that entry, is the cause of the issue, as the > > schedule process might bringing a spurious entry (with ro permission) in the > > MMU cache. > > The TLB invalidation performed under set_kernel_text_rw() affects all > ASIDs on the current CPU, so there shouldn't be any stale RO TLB entries > to hit unless the kthread is migrated to another CPU. > > > If the entry is not global holds the ASID, and the issue cannot happen. > > I don't think that's true, since switching to a different active_mm > would also change ASID, and we'd have no additional guarantee. > I agree, my fault. > Thanks, > Mark. Thanks, GF ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] ARM: kexec: Fix panic after TLB are invalidated @ 2021-01-12 16:49 Giancarlo Ferrari 2021-02-01 10:10 ` Giancarlo Ferrari 0 siblings, 1 reply; 22+ messages in thread From: Giancarlo Ferrari @ 2021-01-12 16:49 UTC (permalink / raw) To: linux Cc: giancarlo.ferrari, michal.simek, Giancarlo Ferrari, linux-arm-kernel, linux-kernel machine_kexec() need to set rw permission in text and rodata sections to assign some variables (e.g. kexec_start_address). To do that at the end (after flushing pdm in memory, inv D-Cache, etc.) it needs to invalidate TLB [section] entries. If during the TLB invalidation an interrupt occours, which might cause a context switch, there is the risk to inject invalid TLBs, with ro permissions. When trying to assign .text labels, this lead to the following issue: "Unable to handle kernel paging request at virtual address <valid_addr>" with FSR 0x80d. Signed-off-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> --- arch/arm/kernel/machine_kexec.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c index 76300f3..bbe912d 100644 --- a/arch/arm/kernel/machine_kexec.c +++ b/arch/arm/kernel/machine_kexec.c @@ -176,6 +176,13 @@ void machine_kexec(struct kimage *image) reboot_code_buffer = page_address(image->control_code_page); + /* + * If below part is not atomic TLB entries might be corrupted after TLB + * invalidation, which leads to Data Abort in .text variable assignment + */ + raw_local_irq_disable(); + local_fiq_disable(); + /* Prepare parameters for reboot_code_buffer*/ set_kernel_text_rw(); kexec_start_address = image->start; @@ -183,6 +190,9 @@ void machine_kexec(struct kimage *image) kexec_mach_type = machine_arch_type; kexec_boot_atags = image->arch.kernel_r2; + local_fiq_enable(); + raw_local_irq_enable(); + /* copy our kernel relocation code to the control code page */ reboot_entry = fncpy(reboot_code_buffer, &relocate_new_kernel, -- 2.7.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: kexec: Fix panic after TLB are invalidated 2021-01-12 16:49 Giancarlo Ferrari @ 2021-02-01 10:10 ` Giancarlo Ferrari 0 siblings, 0 replies; 22+ messages in thread From: Giancarlo Ferrari @ 2021-02-01 10:10 UTC (permalink / raw) To: linux; +Cc: giancarlo.ferrari, michal.simek, linux-arm-kernel, linux-kernel Hi all, On Tue, Jan 12, 2021 at 04:49:06PM +0000, Giancarlo Ferrari wrote: > machine_kexec() need to set rw permission in text and rodata sections > to assign some variables (e.g. kexec_start_address). To do that at > the end (after flushing pdm in memory, inv D-Cache, etc.) it needs to > invalidate TLB [section] entries. > > If during the TLB invalidation an interrupt occours, which might cause > a context switch, there is the risk to inject invalid TLBs, with ro > permissions. > > When trying to assign .text labels, this lead to the following issue: > > "Unable to handle kernel paging request at virtual address <valid_addr>" > > with FSR 0x80d. > > Signed-off-by: Giancarlo Ferrari <giancarlo.ferrari89@gmail.com> > --- > arch/arm/kernel/machine_kexec.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) has been re-submitted here: https://lore.kernel.org/lkml/1612140296-12546-1-git-send-email-giancarlo.ferrari89@gmail.com/ GF ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-02-05 22:54 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-01 0:44 [PATCH] ARM: kexec: Fix panic after TLB are invalidated Giancarlo Ferrari 2021-02-01 11:34 ` Russell King - ARM Linux admin 2021-02-01 12:47 ` Mark Rutland 2021-02-01 13:03 ` Russell King - ARM Linux admin 2021-02-01 13:57 ` Mark Rutland 2021-02-01 16:08 ` Russell King - ARM Linux admin 2021-02-01 16:32 ` Mark Rutland 2021-02-01 16:37 ` Russell King - ARM Linux admin 2021-02-01 20:07 ` Giancarlo Ferrari 2021-02-01 20:16 ` Russell King - ARM Linux admin 2021-02-01 22:18 ` Giancarlo Ferrari 2021-02-04 23:48 ` Giancarlo Ferrari 2021-02-05 0:18 ` Russell King - ARM Linux admin 2021-02-05 0:40 ` Giancarlo Ferrari 2021-02-05 0:45 ` Giancarlo Ferrari 2021-02-05 9:44 ` Russell King - ARM Linux admin 2021-02-05 14:36 ` Giancarlo Ferrari 2021-02-01 14:39 ` Giancarlo Ferrari 2021-02-01 15:30 ` Mark Rutland 2021-02-01 19:09 ` Giancarlo Ferrari -- strict thread matches above, loose matches on Subject: below -- 2021-01-12 16:49 Giancarlo Ferrari 2021-02-01 10:10 ` Giancarlo Ferrari
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).