* Re: [PATCH v2] powerpc/64s: Fix pte update for kernel memory on radix
From: Nicholas Piggin @ 2021-02-04 3:31 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: cmr
In-Reply-To: <20210203235907.961243-1-jniethe5@gmail.com>
Excerpts from Jordan Niethe's message of February 4, 2021 9:59 am:
> When adding a pte a ptesync is needed to order the update of the pte
> with subsequent accesses otherwise a spurious fault may be raised.
>
> radix__set_pte_at() does not do this for performance gains. For
> non-kernel memory this is not an issue as any faults of this kind are
> corrected by the page fault handler. For kernel memory these faults are
> not handled. The current solution is that there is a ptesync in
> flush_cache_vmap() which should be called when mapping from the vmalloc
> region.
>
> However, map_kernel_page() does not call flush_cache_vmap(). This is
> troublesome in particular for code patching with Strict RWX on radix. In
> do_patch_instruction() the page frame that contains the instruction to
> be patched is mapped and then immediately patched. With no ordering or
> synchronization between setting up the pte and writing to the page it is
> possible for faults.
>
> As the code patching is done using __put_user_asm_goto() the resulting
> fault is obscured - but using a normal store instead it can be seen:
>
> [ 418.498768][ T757] BUG: Unable to handle kernel data access on write at 0xc008000008f24a3c
> [ 418.498790][ T757] Faulting instruction address: 0xc00000000008bd74
> [ 418.498805][ T757] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 418.498828][ T757] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV
> [ 418.498843][ T757] Modules linked in: nop_module(PO+) [last unloaded: nop_module]
> [ 418.498872][ T757] CPU: 4 PID: 757 Comm: sh Tainted: P O 5.10.0-rc5-01361-ge3c1b78c8440-dirty #43
> [ 418.498936][ T757] NIP: c00000000008bd74 LR: c00000000008bd50 CTR: c000000000025810
> [ 418.498979][ T757] REGS: c000000016f634a0 TRAP: 0300 Tainted: P O (5.10.0-rc5-01361-ge3c1b78c8440-dirty)
> [ 418.499033][ T757] MSR: 9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 44002884 XER: 00000000
> [ 418.499084][ T757] CFAR: c00000000007c68c DAR: c008000008f24a3c DSISR: 42000000 IRQMASK: 1
>
> This results in the kind of issue reported here:
> https://lore.kernel.org/linuxppc-dev/15AC5B0E-A221-4B8C-9039-FA96B8EF7C88@lca.pw/
>
> Chris Riedl suggested a reliable way to reproduce the issue:
> $ mount -t debugfs none /sys/kernel/debug
> $ (while true; do echo function > /sys/kernel/debug/tracing/current_tracer ; echo nop > /sys/kernel/debug/tracing/current_tracer ; done)&
>
> Turning ftrace on and off does a large amount of code patching which in
> usually less then 5min will crash giving a trace like:
>
> [ 146.668988][ T809] ftrace-powerpc: (____ptrval____): replaced (4b473b11) != old (60000000)
> [ 146.668995][ T809] ------------[ ftrace bug ]------------
> [ 146.669031][ T809] ftrace failed to modify
> [ 146.669039][ T809] [<c000000000bf8e5c>] napi_busy_loop+0xc/0x390
> [ 146.669045][ T809] actual: 11:3b:47:4b
> [ 146.669070][ T809] Setting ftrace call site to call ftrace function
> [ 146.669075][ T809] ftrace record flags: 80000001
> [ 146.669081][ T809] (1)
> [ 146.669081][ T809] expected tramp: c00000000006c96c
> [ 146.669096][ T809] ------------[ cut here ]------------
> [ 146.669104][ T809] WARNING: CPU: 4 PID: 809 at kernel/trace/ftrace.c:2065 ftrace_bug+0x28c/0x2e8
> [ 146.669109][ T809] Modules linked in: nop_module(PO-) [last unloaded: nop_module]
> [ 146.669130][ T809] CPU: 4 PID: 809 Comm: sh Tainted: P O 5.10.0-rc5-01360-gf878ccaf250a #1
> [ 146.669136][ T809] NIP: c00000000024f334 LR: c00000000024f330 CTR: c0000000001a5af0
> [ 146.669142][ T809] REGS: c000000004c8b760 TRAP: 0700 Tainted: P O (5.10.0-rc5-01360-gf878ccaf250a)
> [ 146.669147][ T809] MSR: 900000000282b033 <SF,HV,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 28008848 XER: 20040000
> [ 146.669208][ T809] CFAR: c0000000001a9c98 IRQMASK: 0
> [ 146.669208][ T809] GPR00: c00000000024f330 c000000004c8b9f0 c000000002770600 0000000000000022
> [ 146.669208][ T809] GPR04: 00000000ffff7fff c000000004c8b6d0 0000000000000027 c0000007fe9bcdd8
> [ 146.669208][ T809] GPR08: 0000000000000023 ffffffffffffffd8 0000000000000027 c000000002613118
> [ 146.669208][ T809] GPR12: 0000000000008000 c0000007fffdca00 0000000000000000 0000000000000000
> [ 146.669208][ T809] GPR16: 0000000023ec37c5 0000000000000000 0000000000000000 0000000000000008
> [ 146.669208][ T809] GPR20: c000000004c8bc90 c0000000027a2d20 c000000004c8bcd0 c000000002612fe8
> [ 146.669208][ T809] GPR24: 0000000000000038 0000000000000030 0000000000000028 0000000000000020
> [ 146.669208][ T809] GPR28: c000000000ff1b68 c000000000bf8e5c c00000000312f700 c000000000fbb9b0
> [ 146.669384][ T809] NIP [c00000000024f334] ftrace_bug+0x28c/0x2e8
> [ 146.669391][ T809] LR [c00000000024f330] ftrace_bug+0x288/0x2e8
> [ 146.669396][ T809] Call Trace:
> [ 146.669403][ T809] [c000000004c8b9f0] [c00000000024f330] ftrace_bug+0x288/0x2e8 (unreliable)
> [ 146.669418][ T809] [c000000004c8ba80] [c000000000248778] ftrace_modify_all_code+0x168/0x210
> [ 146.669429][ T809] [c000000004c8bab0] [c00000000006c528] arch_ftrace_update_code+0x18/0x30
> [ 146.669440][ T809] [c000000004c8bad0] [c000000000248954] ftrace_run_update_code+0x44/0xc0
> [ 146.669451][ T809] [c000000004c8bb00] [c00000000024dc88] ftrace_startup+0xf8/0x1c0
> [ 146.669461][ T809] [c000000004c8bb40] [c00000000024dd9c] register_ftrace_function+0x4c/0xc0
> [ 146.669472][ T809] [c000000004c8bb70] [c00000000026e750] function_trace_init+0x80/0xb0
> [ 146.669484][ T809] [c000000004c8bba0] [c000000000266b84] tracing_set_tracer+0x2a4/0x4f0
> [ 146.669495][ T809] [c000000004c8bc70] [c000000000266ea4] tracing_set_trace_write+0xd4/0x130
> [ 146.669506][ T809] [c000000004c8bd20] [c000000000422790] vfs_write+0xf0/0x330
> [ 146.669518][ T809] [c000000004c8bd70] [c000000000422bb4] ksys_write+0x84/0x140
> [ 146.669529][ T809] [c000000004c8bdc0] [c00000000003499c] system_call_exception+0x14c/0x230
> [ 146.669540][ T809] [c000000004c8be20] [c00000000000d860] system_call_common+0xf0/0x27c
> [ 146.669549][ T809] Instruction dump:
> [ 146.669558][ T809] 48000014 3c62fe88 38631718 4bf5a941 60000000 7fc3f378 4bff877d 7c641b78
> [ 146.669598][ T809] 3c62fe88 38631730 4bf5a925 60000000 <0fe00000> 38210090 3d22fd90 39000001
> [ 146.669638][ T809] ---[ end trace 5ea7076ea28c0fbd ]---
>
> To fix this when updating kernel memory ptes using ptesync.
>
> Fixes: 37bc3e5fd764 ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
> Fixes: f1cb8f9beba8 ("powerpc/64s/radix: avoid ptesync after set_pte and ptep_set_access_flags")
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
Good catch. I would say it just fixes the latter patch doesn't it? The
previous one just happens to break because it's using the broken API?
Anyhow,
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> v2: Only ptesync is needed
> ---
> arch/powerpc/include/asm/book3s/64/radix.h | 6 ++++--
> arch/powerpc/mm/book3s64/radix_pgtable.c | 4 ++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
> index c7813dc628fc..59cab558e2f0 100644
> --- a/arch/powerpc/include/asm/book3s/64/radix.h
> +++ b/arch/powerpc/include/asm/book3s/64/radix.h
> @@ -222,8 +222,10 @@ static inline void radix__set_pte_at(struct mm_struct *mm, unsigned long addr,
> * from ptesync, it should probably go into update_mmu_cache, rather
> * than set_pte_at (which is used to set ptes unrelated to faults).
> *
> - * Spurious faults to vmalloc region are not tolerated, so there is
> - * a ptesync in flush_cache_vmap.
> + * Spurious faults from the kernel memory are not tolerated, so there
> + * is a ptesync in flush_cache_vmap, and __map_kernel_page() follows
> + * the pte update sequence from ISA Book III 6.10 Translation Table
> + * Update Synchronization Requirements.
> */
> }
>
> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
> index 3adcf730f478..1d5eec847b88 100644
> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
> @@ -108,7 +108,7 @@ static int early_map_kernel_page(unsigned long ea, unsigned long pa,
>
> set_the_pte:
> set_pte_at(&init_mm, ea, ptep, pfn_pte(pfn, flags));
> - smp_wmb();
> + asm volatile("ptesync": : :"memory");
> return 0;
> }
>
> @@ -168,7 +168,7 @@ static int __map_kernel_page(unsigned long ea, unsigned long pa,
>
> set_the_pte:
> set_pte_at(&init_mm, ea, ptep, pfn_pte(pfn, flags));
> - smp_wmb();
> + asm volatile("ptesync": : :"memory");
> return 0;
> }
>
> --
> 2.25.1
>
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/64s: Fix pte update for kernel memory on radix
From: Jordan Niethe @ 2021-02-04 4:45 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, cmr
In-Reply-To: <1612409379.adcakadx0b.astroid@bobo.none>
On Thu, Feb 4, 2021 at 2:31 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Excerpts from Jordan Niethe's message of February 4, 2021 9:59 am:
> > When adding a pte a ptesync is needed to order the update of the pte
> > with subsequent accesses otherwise a spurious fault may be raised.
> >
> > radix__set_pte_at() does not do this for performance gains. For
> > non-kernel memory this is not an issue as any faults of this kind are
> > corrected by the page fault handler. For kernel memory these faults are
> > not handled. The current solution is that there is a ptesync in
> > flush_cache_vmap() which should be called when mapping from the vmalloc
> > region.
> >
> > However, map_kernel_page() does not call flush_cache_vmap(). This is
> > troublesome in particular for code patching with Strict RWX on radix. In
> > do_patch_instruction() the page frame that contains the instruction to
> > be patched is mapped and then immediately patched. With no ordering or
> > synchronization between setting up the pte and writing to the page it is
> > possible for faults.
> >
> > As the code patching is done using __put_user_asm_goto() the resulting
> > fault is obscured - but using a normal store instead it can be seen:
> >
> > [ 418.498768][ T757] BUG: Unable to handle kernel data access on write at 0xc008000008f24a3c
> > [ 418.498790][ T757] Faulting instruction address: 0xc00000000008bd74
> > [ 418.498805][ T757] Oops: Kernel access of bad area, sig: 11 [#1]
> > [ 418.498828][ T757] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV
> > [ 418.498843][ T757] Modules linked in: nop_module(PO+) [last unloaded: nop_module]
> > [ 418.498872][ T757] CPU: 4 PID: 757 Comm: sh Tainted: P O 5.10.0-rc5-01361-ge3c1b78c8440-dirty #43
> > [ 418.498936][ T757] NIP: c00000000008bd74 LR: c00000000008bd50 CTR: c000000000025810
> > [ 418.498979][ T757] REGS: c000000016f634a0 TRAP: 0300 Tainted: P O (5.10.0-rc5-01361-ge3c1b78c8440-dirty)
> > [ 418.499033][ T757] MSR: 9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 44002884 XER: 00000000
> > [ 418.499084][ T757] CFAR: c00000000007c68c DAR: c008000008f24a3c DSISR: 42000000 IRQMASK: 1
> >
> > This results in the kind of issue reported here:
> > https://lore.kernel.org/linuxppc-dev/15AC5B0E-A221-4B8C-9039-FA96B8EF7C88@lca.pw/
> >
> > Chris Riedl suggested a reliable way to reproduce the issue:
> > $ mount -t debugfs none /sys/kernel/debug
> > $ (while true; do echo function > /sys/kernel/debug/tracing/current_tracer ; echo nop > /sys/kernel/debug/tracing/current_tracer ; done)&
> >
> > Turning ftrace on and off does a large amount of code patching which in
> > usually less then 5min will crash giving a trace like:
> >
> > [ 146.668988][ T809] ftrace-powerpc: (____ptrval____): replaced (4b473b11) != old (60000000)
> > [ 146.668995][ T809] ------------[ ftrace bug ]------------
> > [ 146.669031][ T809] ftrace failed to modify
> > [ 146.669039][ T809] [<c000000000bf8e5c>] napi_busy_loop+0xc/0x390
> > [ 146.669045][ T809] actual: 11:3b:47:4b
> > [ 146.669070][ T809] Setting ftrace call site to call ftrace function
> > [ 146.669075][ T809] ftrace record flags: 80000001
> > [ 146.669081][ T809] (1)
> > [ 146.669081][ T809] expected tramp: c00000000006c96c
> > [ 146.669096][ T809] ------------[ cut here ]------------
> > [ 146.669104][ T809] WARNING: CPU: 4 PID: 809 at kernel/trace/ftrace.c:2065 ftrace_bug+0x28c/0x2e8
> > [ 146.669109][ T809] Modules linked in: nop_module(PO-) [last unloaded: nop_module]
> > [ 146.669130][ T809] CPU: 4 PID: 809 Comm: sh Tainted: P O 5.10.0-rc5-01360-gf878ccaf250a #1
> > [ 146.669136][ T809] NIP: c00000000024f334 LR: c00000000024f330 CTR: c0000000001a5af0
> > [ 146.669142][ T809] REGS: c000000004c8b760 TRAP: 0700 Tainted: P O (5.10.0-rc5-01360-gf878ccaf250a)
> > [ 146.669147][ T809] MSR: 900000000282b033 <SF,HV,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 28008848 XER: 20040000
> > [ 146.669208][ T809] CFAR: c0000000001a9c98 IRQMASK: 0
> > [ 146.669208][ T809] GPR00: c00000000024f330 c000000004c8b9f0 c000000002770600 0000000000000022
> > [ 146.669208][ T809] GPR04: 00000000ffff7fff c000000004c8b6d0 0000000000000027 c0000007fe9bcdd8
> > [ 146.669208][ T809] GPR08: 0000000000000023 ffffffffffffffd8 0000000000000027 c000000002613118
> > [ 146.669208][ T809] GPR12: 0000000000008000 c0000007fffdca00 0000000000000000 0000000000000000
> > [ 146.669208][ T809] GPR16: 0000000023ec37c5 0000000000000000 0000000000000000 0000000000000008
> > [ 146.669208][ T809] GPR20: c000000004c8bc90 c0000000027a2d20 c000000004c8bcd0 c000000002612fe8
> > [ 146.669208][ T809] GPR24: 0000000000000038 0000000000000030 0000000000000028 0000000000000020
> > [ 146.669208][ T809] GPR28: c000000000ff1b68 c000000000bf8e5c c00000000312f700 c000000000fbb9b0
> > [ 146.669384][ T809] NIP [c00000000024f334] ftrace_bug+0x28c/0x2e8
> > [ 146.669391][ T809] LR [c00000000024f330] ftrace_bug+0x288/0x2e8
> > [ 146.669396][ T809] Call Trace:
> > [ 146.669403][ T809] [c000000004c8b9f0] [c00000000024f330] ftrace_bug+0x288/0x2e8 (unreliable)
> > [ 146.669418][ T809] [c000000004c8ba80] [c000000000248778] ftrace_modify_all_code+0x168/0x210
> > [ 146.669429][ T809] [c000000004c8bab0] [c00000000006c528] arch_ftrace_update_code+0x18/0x30
> > [ 146.669440][ T809] [c000000004c8bad0] [c000000000248954] ftrace_run_update_code+0x44/0xc0
> > [ 146.669451][ T809] [c000000004c8bb00] [c00000000024dc88] ftrace_startup+0xf8/0x1c0
> > [ 146.669461][ T809] [c000000004c8bb40] [c00000000024dd9c] register_ftrace_function+0x4c/0xc0
> > [ 146.669472][ T809] [c000000004c8bb70] [c00000000026e750] function_trace_init+0x80/0xb0
> > [ 146.669484][ T809] [c000000004c8bba0] [c000000000266b84] tracing_set_tracer+0x2a4/0x4f0
> > [ 146.669495][ T809] [c000000004c8bc70] [c000000000266ea4] tracing_set_trace_write+0xd4/0x130
> > [ 146.669506][ T809] [c000000004c8bd20] [c000000000422790] vfs_write+0xf0/0x330
> > [ 146.669518][ T809] [c000000004c8bd70] [c000000000422bb4] ksys_write+0x84/0x140
> > [ 146.669529][ T809] [c000000004c8bdc0] [c00000000003499c] system_call_exception+0x14c/0x230
> > [ 146.669540][ T809] [c000000004c8be20] [c00000000000d860] system_call_common+0xf0/0x27c
> > [ 146.669549][ T809] Instruction dump:
> > [ 146.669558][ T809] 48000014 3c62fe88 38631718 4bf5a941 60000000 7fc3f378 4bff877d 7c641b78
> > [ 146.669598][ T809] 3c62fe88 38631730 4bf5a925 60000000 <0fe00000> 38210090 3d22fd90 39000001
> > [ 146.669638][ T809] ---[ end trace 5ea7076ea28c0fbd ]---
> >
> > To fix this when updating kernel memory ptes using ptesync.
> >
> > Fixes: 37bc3e5fd764 ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
> > Fixes: f1cb8f9beba8 ("powerpc/64s/radix: avoid ptesync after set_pte and ptep_set_access_flags")
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>
> Good catch. I would say it just fixes the latter patch doesn't it? The
> previous one just happens to break because it's using the broken API?
Yes that is true. I will send another revision that removes that from
the 'Fixes' and also add a self test.
>
> Anyhow,
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> > ---
> > v2: Only ptesync is needed
> > ---
> > arch/powerpc/include/asm/book3s/64/radix.h | 6 ++++--
> > arch/powerpc/mm/book3s64/radix_pgtable.c | 4 ++--
> > 2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
> > index c7813dc628fc..59cab558e2f0 100644
> > --- a/arch/powerpc/include/asm/book3s/64/radix.h
> > +++ b/arch/powerpc/include/asm/book3s/64/radix.h
> > @@ -222,8 +222,10 @@ static inline void radix__set_pte_at(struct mm_struct *mm, unsigned long addr,
> > * from ptesync, it should probably go into update_mmu_cache, rather
> > * than set_pte_at (which is used to set ptes unrelated to faults).
> > *
> > - * Spurious faults to vmalloc region are not tolerated, so there is
> > - * a ptesync in flush_cache_vmap.
> > + * Spurious faults from the kernel memory are not tolerated, so there
> > + * is a ptesync in flush_cache_vmap, and __map_kernel_page() follows
> > + * the pte update sequence from ISA Book III 6.10 Translation Table
> > + * Update Synchronization Requirements.
> > */
> > }
> >
> > diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
> > index 3adcf730f478..1d5eec847b88 100644
> > --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
> > +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
> > @@ -108,7 +108,7 @@ static int early_map_kernel_page(unsigned long ea, unsigned long pa,
> >
> > set_the_pte:
> > set_pte_at(&init_mm, ea, ptep, pfn_pte(pfn, flags));
> > - smp_wmb();
> > + asm volatile("ptesync": : :"memory");
> > return 0;
> > }
> >
> > @@ -168,7 +168,7 @@ static int __map_kernel_page(unsigned long ea, unsigned long pa,
> >
> > set_the_pte:
> > set_pte_at(&init_mm, ea, ptep, pfn_pte(pfn, flags));
> > - smp_wmb();
> > + asm volatile("ptesync": : :"memory");
> > return 0;
> > }
> >
> > --
> > 2.25.1
> >
> >
^ permalink raw reply
* Re: [PATCH v4 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()
From: Michael Ellerman @ 2021-02-04 6:09 UTC (permalink / raw)
To: Christopher M. Riedl, Gabriel Paubert
Cc: David Laight, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <C8YHSKQ99VC4.M9Y0WOFVUTBQ@geist>
"Christopher M. Riedl" <cmr@codefail.de> writes:
> On Mon Feb 1, 2021 at 10:54 AM CST, Gabriel Paubert wrote:
>> On Mon, Feb 01, 2021 at 09:55:44AM -0600, Christopher M. Riedl wrote:
>> > On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote:
>> > > From: Christopher M. Riedl
>> > > > Sent: 28 January 2021 04:04
>> > > >
>> > > > Reuse the "safe" implementation from signal.c except for calling
>> > > > unsafe_copy_from_user() to copy into a local buffer.
>> > > >
>> > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
>> > > > ---
>> > > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++
>> > > > 1 file changed, 33 insertions(+)
>> > > >
>> > > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
>> > > > index 2559a681536e..c18402d625f1 100644
>> > > > --- a/arch/powerpc/kernel/signal.h
>> > > > +++ b/arch/powerpc/kernel/signal.h
>> > > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
>> > > > &buf[i], label);\
>> > > > } while (0)
>> > > >
>> > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \
>> > > > + struct task_struct *__t = task; \
>> > > > + u64 __user *__f = (u64 __user *)from; \
>> > > > + u64 buf[ELF_NFPREG]; \
>> > >
>> > > How big is that buffer?
>> > > Isn't is likely to be reasonably large compared to a reasonable
>> > > kernel stack frame.
>> > > Especially since this isn't even a leaf function.
>> > >
>> >
>> > I think Christophe answered this - I don't really have an opinion either
>> > way. What would be a 'reasonable' kernel stack frame for reference?
>>
>> See include/linux/poll.h, where the limit is of the order of 800 bytes
>> and the number of entries in an on stack array is chosen at compile time
>> (different between 32 and 64 bit for example).
>>
>> The values are used in do_sys_poll, which, with almost 1000 bytes of
>> stack footprint, appears close to the top of "make checkstack". In
>> addition do_sys_poll has to call the ->poll function of every file
>> descriptor in its table, so it is not a tail function.
>>
>> This 264 bytes array looks reasonable, but please use 'make checkstack'
>> to verify that the function's total stack usage stays within reason.
>
> Neat, looks like total usage is a bit larger but still reasonable and
> less than half of 800B:
>
> 0xc000000000017e900 __unsafe_restore_sigcontext.constprop.0 [vmlinux]:352
We warn for frames larger than 2KB on 64-bit, see FRAME_WARN in
lib/Kconfig.debug.
So 264 bytes is entirely reasonable IMHO.
cheers
^ permalink raw reply
* Re: [PATCH v2] powerpc/64s: Fix pte update for kernel memory on radix
From: Naveen N. Rao @ 2021-02-04 6:41 UTC (permalink / raw)
To: Jordan Niethe; +Cc: linuxppc-dev, npiggin, cmr
In-Reply-To: <20210203235907.961243-1-jniethe5@gmail.com>
Hi Jordan,
On 2021/02/04 10:59AM, Jordan Niethe wrote:
> When adding a pte a ptesync is needed to order the update of the pte
> with subsequent accesses otherwise a spurious fault may be raised.
>
> radix__set_pte_at() does not do this for performance gains. For
> non-kernel memory this is not an issue as any faults of this kind are
> corrected by the page fault handler. For kernel memory these faults are
> not handled. The current solution is that there is a ptesync in
> flush_cache_vmap() which should be called when mapping from the vmalloc
> region.
>
> However, map_kernel_page() does not call flush_cache_vmap(). This is
> troublesome in particular for code patching with Strict RWX on radix. In
> do_patch_instruction() the page frame that contains the instruction to
> be patched is mapped and then immediately patched. With no ordering or
> synchronization between setting up the pte and writing to the page it is
> possible for faults.
>
> As the code patching is done using __put_user_asm_goto() the resulting
> fault is obscured - but using a normal store instead it can be seen:
>
> [ 418.498768][ T757] BUG: Unable to handle kernel data access on write at 0xc008000008f24a3c
> [ 418.498790][ T757] Faulting instruction address: 0xc00000000008bd74
> [ 418.498805][ T757] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 418.498828][ T757] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV
> [ 418.498843][ T757] Modules linked in: nop_module(PO+) [last unloaded: nop_module]
> [ 418.498872][ T757] CPU: 4 PID: 757 Comm: sh Tainted: P O 5.10.0-rc5-01361-ge3c1b78c8440-dirty #43
> [ 418.498936][ T757] NIP: c00000000008bd74 LR: c00000000008bd50 CTR: c000000000025810
> [ 418.498979][ T757] REGS: c000000016f634a0 TRAP: 0300 Tainted: P O (5.10.0-rc5-01361-ge3c1b78c8440-dirty)
> [ 418.499033][ T757] MSR: 9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 44002884 XER: 00000000
> [ 418.499084][ T757] CFAR: c00000000007c68c DAR: c008000008f24a3c DSISR: 42000000 IRQMASK: 1
>
> This results in the kind of issue reported here:
> https://lore.kernel.org/linuxppc-dev/15AC5B0E-A221-4B8C-9039-FA96B8EF7C88@lca.pw/
>
Thanks for fixing this!
- Naveen
^ permalink raw reply
* Re: [PATCH] powerpc64/idle: Fix SP offsets when saving GPRs
From: Christopher M. Riedl @ 2021-02-04 6:59 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev, Michael Ellerman
In-Reply-To: <1612014056.e1qcnzac7c.astroid@bobo.none>
On Sat Jan 30, 2021 at 7:44 AM CST, Nicholas Piggin wrote:
> Excerpts from Michael Ellerman's message of January 30, 2021 9:32 pm:
> > "Christopher M. Riedl" <cmr@codefail.de> writes:
> >> The idle entry/exit code saves/restores GPRs in the stack "red zone"
> >> (Protected Zone according to PowerPC64 ELF ABI v2). However, the offset
> >> used for the first GPR is incorrect and overwrites the back chain - the
> >> Protected Zone actually starts below the current SP. In practice this is
> >> probably not an issue, but it's still incorrect so fix it.
> >
> > Nice catch.
> >
> > Corrupting the back chain means you can't backtrace from there, which
> > could be confusing for debugging one day.
>
> Yeah, we seem to have got away without noticing because the CPU will
> wake up and return out of here before it tries to unwind the stack,
> but if you tried to walk it by hand if the CPU got stuck in idle or
> something, then we'd get confused.
>
> > It does make me wonder why we don't just create a stack frame and use
> > the normal macros? It would use a bit more stack space, but we shouldn't
> > be short of stack space when going idle.
> >
> > Nick, was there a particular reason for using the red zone?
>
> I don't recall a particular reason, I think a normal stack frame is
> probably a good idea.
I'll send a version using STACKFRAMESIZE - I assume that's the "normal"
stack frame :)
I admit I am a bit confused when I saw the similar but much smaller
STACK_FRAME_OVERHEAD which is also used in _some_ cases to save/restore
a few registers.
>
> Thanks,
> Nick
^ permalink raw reply
* [PATCH v3 1/2] powerpc: sstep: Fix load-store and update emulation
From: Sandipan Das @ 2021-02-04 7:14 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, ananth, jniethe5, paulus, naveen.n.rao,
linuxppc-dev, dja
The Power ISA says that the fixed-point load and update
instructions must neither use R0 for the base address (RA)
nor have the destination (RT) and the base address (RA) as
the same register. Similarly, for fixed-point stores and
floating-point loads and stores, the instruction is invalid
when R0 is used as the base address (RA).
This is applicable to the following instructions.
* Load Byte and Zero with Update (lbzu)
* Load Byte and Zero with Update Indexed (lbzux)
* Load Halfword and Zero with Update (lhzu)
* Load Halfword and Zero with Update Indexed (lhzux)
* Load Halfword Algebraic with Update (lhau)
* Load Halfword Algebraic with Update Indexed (lhaux)
* Load Word and Zero with Update (lwzu)
* Load Word and Zero with Update Indexed (lwzux)
* Load Word Algebraic with Update Indexed (lwaux)
* Load Doubleword with Update (ldu)
* Load Doubleword with Update Indexed (ldux)
* Load Floating Single with Update (lfsu)
* Load Floating Single with Update Indexed (lfsux)
* Load Floating Double with Update (lfdu)
* Load Floating Double with Update Indexed (lfdux)
* Store Byte with Update (stbu)
* Store Byte with Update Indexed (stbux)
* Store Halfword with Update (sthu)
* Store Halfword with Update Indexed (sthux)
* Store Word with Update (stwu)
* Store Word with Update Indexed (stwux)
* Store Doubleword with Update (stdu)
* Store Doubleword with Update Indexed (stdux)
* Store Floating Single with Update (stfsu)
* Store Floating Single with Update Indexed (stfsux)
* Store Floating Double with Update (stfdu)
* Store Floating Double with Update Indexed (stfdux)
E.g. the following behaviour is observed for an invalid
load and update instruction having RA = RT.
While an userspace program having an instruction word like
0xe9ce0001, i.e. ldu r14, 0(r14), runs without getting
receiving a SIGILL on a Power system (observed on P8 and
P9), the outcome of executing that instruction word varies
and its behaviour can be considered to be undefined.
Attaching an uprobe at that instruction's address results
in emulation which currently performs the load as well as
writes the effective address back to the base register.
This might not match the outcome from hardware.
To remove any inconsistencies, this adds additional checks
for the aforementioned instructions to make sure that the
emulation infrastructure treats them as unknown. The kernel
can then fallback to executing such instructions on hardware.
Fixes: 0016a4cf5582 ("powerpc: Emulate most Book I instructions in emulate_step()")
Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
---
Previous versions can be found at:
v2: https://lore.kernel.org/linuxppc-dev/20210203063841.431063-1-sandipan@linux.ibm.com/
v1: https://lore.kernel.org/linuxppc-dev/20201119054139.244083-1-sandipan@linux.ibm.com/
Changes in v3:
- Dropped CONFIG_PPC_FPU check as suggested by Michael.
- Consolidated the checks as suggested by Naveen.
Changes in v2:
- Jump to unknown_opcode instead of returning -1 for invalid
instruction forms.
---
arch/powerpc/lib/sstep.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index e96cff845ef7..9138967eb82e 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -3017,6 +3017,20 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
}
+ if (OP_IS_LOAD_STORE(op->type) && (op->type & UPDATE)) {
+ switch (GETTYPE(op->type)) {
+ case LOAD:
+ if (ra == rd)
+ goto unknown_opcode;
+ fallthrough;
+ case STORE:
+ case LOAD_FP:
+ case STORE_FP:
+ if (ra == 0)
+ goto unknown_opcode;
+ }
+ }
+
#ifdef CONFIG_VSX
if ((GETTYPE(op->type) == LOAD_VSX ||
GETTYPE(op->type) == STORE_VSX) &&
--
2.25.1
^ permalink raw reply related
* [PATCH v3 2/2] powerpc: sstep: Fix darn emulation
From: Sandipan Das @ 2021-02-04 7:14 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, ananth, jniethe5, paulus, naveen.n.rao,
linuxppc-dev, dja
In-Reply-To: <20210204071432.116439-1-sandipan@linux.ibm.com>
Commit 8813ff49607e ("powerpc/sstep: Check instruction
validity against ISA version before emulation") introduced
a proper way to skip unknown instructions. This makes sure
that the same is used for the darn instruction when the
range selection bits have a reserved value.
Fixes: a23987ef267a ("powerpc: sstep: Add support for darn instruction")
Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
---
arch/powerpc/lib/sstep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 9138967eb82e..e7e8bc974c0f 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1916,7 +1916,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
goto compute_done;
}
- return -1;
+ goto unknown_opcode;
#ifdef __powerpc64__
case 777: /* modsd */
if (!cpu_has_feature(CPU_FTR_ARCH_300))
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v3 1/2] powerpc: sstep: Fix load-store and update emulation
From: Sandipan Das @ 2021-02-04 7:23 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, ananth, jniethe5, paulus, naveen.n.rao,
linuxppc-dev, dja
In-Reply-To: <20210204071432.116439-1-sandipan@linux.ibm.com>
On 04/02/21 12:44 pm, Sandipan Das wrote:
> The Power ISA says that the fixed-point load and update
> instructions must neither use R0 for the base address (RA)
> nor have the destination (RT) and the base address (RA) as
> the same register. Similarly, for fixed-point stores and
> floating-point loads and stores, the instruction is invalid
> when R0 is used as the base address (RA).
>
> This is applicable to the following instructions.
> * Load Byte and Zero with Update (lbzu)
> * Load Byte and Zero with Update Indexed (lbzux)
> * Load Halfword and Zero with Update (lhzu)
> * Load Halfword and Zero with Update Indexed (lhzux)
> * Load Halfword Algebraic with Update (lhau)
> * Load Halfword Algebraic with Update Indexed (lhaux)
> * Load Word and Zero with Update (lwzu)
> * Load Word and Zero with Update Indexed (lwzux)
> * Load Word Algebraic with Update Indexed (lwaux)
> * Load Doubleword with Update (ldu)
> * Load Doubleword with Update Indexed (ldux)
> * Load Floating Single with Update (lfsu)
> * Load Floating Single with Update Indexed (lfsux)
> * Load Floating Double with Update (lfdu)
> * Load Floating Double with Update Indexed (lfdux)
> * Store Byte with Update (stbu)
> * Store Byte with Update Indexed (stbux)
> * Store Halfword with Update (sthu)
> * Store Halfword with Update Indexed (sthux)
> * Store Word with Update (stwu)
> * Store Word with Update Indexed (stwux)
> * Store Doubleword with Update (stdu)
> * Store Doubleword with Update Indexed (stdux)
> * Store Floating Single with Update (stfsu)
> * Store Floating Single with Update Indexed (stfsux)
> * Store Floating Double with Update (stfdu)
> * Store Floating Double with Update Indexed (stfdux)
>
> E.g. the following behaviour is observed for an invalid
> load and update instruction having RA = RT.
>
> While an userspace program having an instruction word like
> 0xe9ce0001, i.e. ldu r14, 0(r14), runs without getting
> receiving a SIGILL on a Power system (observed on P8 and
> P9), the outcome of executing that instruction word varies
> and its behaviour can be considered to be undefined.
>
> Attaching an uprobe at that instruction's address results
> in emulation which currently performs the load as well as
> writes the effective address back to the base register.
> This might not match the outcome from hardware.
>
> To remove any inconsistencies, this adds additional checks
> for the aforementioned instructions to make sure that the
> emulation infrastructure treats them as unknown. The kernel
> can then fallback to executing such instructions on hardware.
>
> Fixes: 0016a4cf5582 ("powerpc: Emulate most Book I instructions in emulate_step()")
> Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
> ---
> Previous versions can be found at:
> v2: https://lore.kernel.org/linuxppc-dev/20210203063841.431063-1-sandipan@linux.ibm.com/
> v1: https://lore.kernel.org/linuxppc-dev/20201119054139.244083-1-sandipan@linux.ibm.com/
>
> Changes in v3:
> - Dropped CONFIG_PPC_FPU check as suggested by Michael.
> - Consolidated the checks as suggested by Naveen.
Sorry. Missed these in the changelog:
- Consolidated load/store changes into a single patch.
- Included floating-point load/store and update instructions.
- Sandipan
^ permalink raw reply
* Re: [PATCH RFC v1 2/6] swiotlb: convert variables to arrays
From: Christoph Hellwig @ 2021-02-04 7:29 UTC (permalink / raw)
To: Dongli Zhang
Cc: ulf.hansson, airlied, joonas.lahtinen, dri-devel, linux-kernel,
bhelgaas, paulus, hpa, hch, m.szyprowski, sstabellini,
adrian.hunter, x86, joe.jin, mingo, peterz, mingo, bskeggs,
linux-pci, xen-devel, matthew.auld, thomas.lendacky, konrad.wilk,
intel-gfx, jani.nikula, bp, rodrigo.vivi, nouveau, Claire Chang,
boris.ostrovsky, chris, jgross, tsbogend, robin.murphy, linux-mmc,
linux-mips, iommu, tglx, bauerman, daniel, akpm, linuxppc-dev,
rppt
In-Reply-To: <20210203233709.19819-3-dongli.zhang@oracle.com>
On Wed, Feb 03, 2021 at 03:37:05PM -0800, Dongli Zhang wrote:
> This patch converts several swiotlb related variables to arrays, in
> order to maintain stat/status for different swiotlb buffers. Here are
> variables involved:
>
> - io_tlb_start and io_tlb_end
> - io_tlb_nslabs and io_tlb_used
> - io_tlb_list
> - io_tlb_index
> - max_segment
> - io_tlb_orig_addr
> - no_iotlb_memory
>
> There is no functional change and this is to prepare to enable 64-bit
> swiotlb.
Claire Chang (on Cc) already posted a patch like this a month ago,
which looks much better because it actually uses a struct instead
of all the random variables.
^ permalink raw reply
* Re: [PATCH v3 1/2] powerpc: sstep: Fix load-store and update emulation
From: Naveen N. Rao @ 2021-02-04 7:39 UTC (permalink / raw)
To: Sandipan Das; +Cc: ravi.bangoria, ananth, jniethe5, paulus, linuxppc-dev, dja
In-Reply-To: <20210204071432.116439-1-sandipan@linux.ibm.com>
On 2021/02/04 12:44PM, Sandipan Das wrote:
> The Power ISA says that the fixed-point load and update
> instructions must neither use R0 for the base address (RA)
> nor have the destination (RT) and the base address (RA) as
> the same register. Similarly, for fixed-point stores and
> floating-point loads and stores, the instruction is invalid
> when R0 is used as the base address (RA).
>
> This is applicable to the following instructions.
> * Load Byte and Zero with Update (lbzu)
> * Load Byte and Zero with Update Indexed (lbzux)
> * Load Halfword and Zero with Update (lhzu)
> * Load Halfword and Zero with Update Indexed (lhzux)
> * Load Halfword Algebraic with Update (lhau)
> * Load Halfword Algebraic with Update Indexed (lhaux)
> * Load Word and Zero with Update (lwzu)
> * Load Word and Zero with Update Indexed (lwzux)
> * Load Word Algebraic with Update Indexed (lwaux)
> * Load Doubleword with Update (ldu)
> * Load Doubleword with Update Indexed (ldux)
> * Load Floating Single with Update (lfsu)
> * Load Floating Single with Update Indexed (lfsux)
> * Load Floating Double with Update (lfdu)
> * Load Floating Double with Update Indexed (lfdux)
> * Store Byte with Update (stbu)
> * Store Byte with Update Indexed (stbux)
> * Store Halfword with Update (sthu)
> * Store Halfword with Update Indexed (sthux)
> * Store Word with Update (stwu)
> * Store Word with Update Indexed (stwux)
> * Store Doubleword with Update (stdu)
> * Store Doubleword with Update Indexed (stdux)
> * Store Floating Single with Update (stfsu)
> * Store Floating Single with Update Indexed (stfsux)
> * Store Floating Double with Update (stfdu)
> * Store Floating Double with Update Indexed (stfdux)
>
> E.g. the following behaviour is observed for an invalid
> load and update instruction having RA = RT.
>
> While an userspace program having an instruction word like
> 0xe9ce0001, i.e. ldu r14, 0(r14), runs without getting
> receiving a SIGILL on a Power system (observed on P8 and
> P9), the outcome of executing that instruction word varies
> and its behaviour can be considered to be undefined.
>
> Attaching an uprobe at that instruction's address results
> in emulation which currently performs the load as well as
> writes the effective address back to the base register.
> This might not match the outcome from hardware.
>
> To remove any inconsistencies, this adds additional checks
> for the aforementioned instructions to make sure that the
> emulation infrastructure treats them as unknown. The kernel
> can then fallback to executing such instructions on hardware.
>
> Fixes: 0016a4cf5582 ("powerpc: Emulate most Book I instructions in emulate_step()")
> Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
> ---
> Previous versions can be found at:
> v2: https://lore.kernel.org/linuxppc-dev/20210203063841.431063-1-sandipan@linux.ibm.com/
> v1: https://lore.kernel.org/linuxppc-dev/20201119054139.244083-1-sandipan@linux.ibm.com/
>
> Changes in v3:
> - Dropped CONFIG_PPC_FPU check as suggested by Michael.
> - Consolidated the checks as suggested by Naveen.
>
> Changes in v2:
> - Jump to unknown_opcode instead of returning -1 for invalid
> instruction forms.
>
> ---
> arch/powerpc/lib/sstep.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index e96cff845ef7..9138967eb82e 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -3017,6 +3017,20 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>
> }
>
> + if (OP_IS_LOAD_STORE(op->type) && (op->type & UPDATE)) {
> + switch (GETTYPE(op->type)) {
> + case LOAD:
> + if (ra == rd)
> + goto unknown_opcode;
> + fallthrough;
> + case STORE:
> + case LOAD_FP:
> + case STORE_FP:
> + if (ra == 0)
> + goto unknown_opcode;
> + }
> + }
> +
> #ifdef CONFIG_VSX
> if ((GETTYPE(op->type) == LOAD_VSX ||
> GETTYPE(op->type) == STORE_VSX) &&
I'm afraid there is one more thing. scripts/checkpatch.pl reports:
WARNING: 'an userspace' may be misspelled - perhaps 'a userspace'?
#52:
While an userspace program having an instruction word like
^^^^^^^^^^^^
ERROR: switch and case should be at the same indent
#96: FILE: arch/powerpc/lib/sstep.c:3021:
+ switch (GETTYPE(op->type)) {
+ case LOAD:
[...]
+ case STORE:
+ case LOAD_FP:
+ case STORE_FP:
- Naveen
^ permalink raw reply
* Re: [PATCH v3 1/2] powerpc: sstep: Fix load-store and update emulation
From: Sandipan Das @ 2021-02-04 7:59 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: ravi.bangoria, ananth, jniethe5, paulus, linuxppc-dev, dja
In-Reply-To: <20210204073954.GH210@DESKTOP-TDPLP67.localdomain>
On 04/02/21 1:09 pm, Naveen N. Rao wrote:
> [...]
>
> I'm afraid there is one more thing. scripts/checkpatch.pl reports:
>
> WARNING: 'an userspace' may be misspelled - perhaps 'a userspace'?
> #52:
> While an userspace program having an instruction word like
> ^^^^^^^^^^^^
>
> ERROR: switch and case should be at the same indent
> #96: FILE: arch/powerpc/lib/sstep.c:3021:
> + switch (GETTYPE(op->type)) {
> + case LOAD:
> [...]
> + case STORE:
> + case LOAD_FP:
> + case STORE_FP:
>
>
Yikes! Thanks for pointing that out. Sending v4.
- Sandipan
^ permalink raw reply
* Re: [PATCH v3 28/32] powerpc/64s: interrupt implement exit logic in C
From: Christophe Leroy @ 2021-02-04 8:03 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Michal Suchanek
In-Reply-To: <1612409077.fadt3kvld9.astroid@bobo.none>
Le 04/02/2021 à 04:27, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of February 4, 2021 2:25 am:
>>
>>
>> Le 25/02/2020 à 18:35, Nicholas Piggin a écrit :
>>> Implement the bulk of interrupt return logic in C. The asm return code
>>> must handle a few cases: restoring full GPRs, and emulating stack store.
>>>
>>
>>
>>> +notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsigned long msr)
>>> +{
>>> + unsigned long *ti_flagsp = ¤t_thread_info()->flags;
>>> + unsigned long flags;
>>> +
>>> + if (IS_ENABLED(CONFIG_PPC_BOOK3S) && unlikely(!(regs->msr & MSR_RI)))
>>> + unrecoverable_exception(regs);
>>> + BUG_ON(regs->msr & MSR_PR);
>>> + BUG_ON(!FULL_REGS(regs));
>>> +
>>> + local_irq_save(flags);
>>> +
>>> + if (regs->softe == IRQS_ENABLED) {
>>> + /* Returning to a kernel context with local irqs enabled. */
>>> + WARN_ON_ONCE(!(regs->msr & MSR_EE));
>>> +again:
>>> + if (IS_ENABLED(CONFIG_PREEMPT)) {
>>> + /* Return to preemptible kernel context */
>>> + if (unlikely(*ti_flagsp & _TIF_NEED_RESCHED)) {
>>> + if (preempt_count() == 0)
>>> + preempt_schedule_irq();
>>> + }
>>> + }
>>> +
>>> + trace_hardirqs_on();
>>> + __hard_EE_RI_disable();
>>> + if (unlikely(lazy_irq_pending())) {
>>> + __hard_RI_enable();
>>> + irq_soft_mask_set(IRQS_ALL_DISABLED);
>>> + trace_hardirqs_off();
>>> + local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
>>> + /*
>>> + * Can't local_irq_enable in case we are in interrupt
>>> + * context. Must replay directly.
>>> + */
>>> + replay_soft_interrupts();
>>> + irq_soft_mask_set(flags);
>>> + /* Took an interrupt, may have more exit work to do. */
>>> + goto again;
>>> + }
>>> + local_paca->irq_happened = 0;
>>> + irq_soft_mask_set(IRQS_ENABLED);
>>> + } else {
>>> + /* Returning to a kernel context with local irqs disabled. */
>>> + trace_hardirqs_on();
>>> + __hard_EE_RI_disable();
>>> + if (regs->msr & MSR_EE)
>>> + local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>>> + }
>>> +
>>> +
>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>> + local_paca->tm_scratch = regs->msr;
>>> +#endif
>>> +
>>> + /*
>>> + * We don't need to restore AMR on the way back to userspace for KUAP.
>>> + * The value of AMR only matters while we're in the kernel.
>>> + */
>>> + kuap_restore_amr(regs);
>>
>> Is that correct to restore KUAP state here ? Shouldn't we have it at lower level in assembly ?
>>
>> Isn't there a risk that someone manages to call interrupt_exit_kernel_prepare() or the end of it in
>> a way or another, and get the previous KUAP state restored by this way ?
>
> I'm not sure if there much more risk if it's here rather than the
> instruction being in another place in the code.
>
> There's a lot of user access around the kernel too if you want to find a
> gadget to unlock KUAP then I suppose there is a pretty large attack
> surface.
My understanding is that user access scope is strictly limited, for instance we enforce the
begin/end of user access to be in the same function, and we refrain from calling any other function
inside the user access window. x86 even have 'objtool' to enforce it at build time. So in theory
there is no way to get out of the function while user access is open.
Here with the interrupt exit function it is free beer. You have a place where you re-open user
access and return with a simple blr. So that's open bar. If someone manages to just call the
interrupt exit function, then user access remains open
>
>> Also, it looks a bit strange to have kuap_save_amr_and_lock() done at lowest level in assembly, and
>> kuap_restore_amr() done in upper level. That looks unbalanced.
>
> I'd like to bring the entry assembly into C.
>
I really think it's not a good idea. We'll get better control and readability by keeping it at the
lowest possible level in assembly.
x86 only save and restore SMAC state in assembly.
Christophe
^ permalink raw reply
* [PATCH v4 1/2] powerpc: sstep: Fix load-store and update emulation
From: Sandipan Das @ 2021-02-04 8:07 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, ananth, jniethe5, paulus, naveen.n.rao,
linuxppc-dev, dja
The Power ISA says that the fixed-point load and update
instructions must neither use R0 for the base address (RA)
nor have the destination (RT) and the base address (RA) as
the same register. Similarly, for fixed-point stores and
floating-point loads and stores, the instruction is invalid
when R0 is used as the base address (RA).
This is applicable to the following instructions.
* Load Byte and Zero with Update (lbzu)
* Load Byte and Zero with Update Indexed (lbzux)
* Load Halfword and Zero with Update (lhzu)
* Load Halfword and Zero with Update Indexed (lhzux)
* Load Halfword Algebraic with Update (lhau)
* Load Halfword Algebraic with Update Indexed (lhaux)
* Load Word and Zero with Update (lwzu)
* Load Word and Zero with Update Indexed (lwzux)
* Load Word Algebraic with Update Indexed (lwaux)
* Load Doubleword with Update (ldu)
* Load Doubleword with Update Indexed (ldux)
* Load Floating Single with Update (lfsu)
* Load Floating Single with Update Indexed (lfsux)
* Load Floating Double with Update (lfdu)
* Load Floating Double with Update Indexed (lfdux)
* Store Byte with Update (stbu)
* Store Byte with Update Indexed (stbux)
* Store Halfword with Update (sthu)
* Store Halfword with Update Indexed (sthux)
* Store Word with Update (stwu)
* Store Word with Update Indexed (stwux)
* Store Doubleword with Update (stdu)
* Store Doubleword with Update Indexed (stdux)
* Store Floating Single with Update (stfsu)
* Store Floating Single with Update Indexed (stfsux)
* Store Floating Double with Update (stfdu)
* Store Floating Double with Update Indexed (stfdux)
E.g. the following behaviour is observed for an invalid
load and update instruction having RA = RT.
While a userspace program having an instruction word like
0xe9ce0001, i.e. ldu r14, 0(r14), runs without getting
receiving a SIGILL on a Power system (observed on P8 and
P9), the outcome of executing that instruction word varies
and its behaviour can be considered to be undefined.
Attaching an uprobe at that instruction's address results
in emulation which currently performs the load as well as
writes the effective address back to the base register.
This might not match the outcome from hardware.
To remove any inconsistencies, this adds additional checks
for the aforementioned instructions to make sure that the
emulation infrastructure treats them as unknown. The kernel
can then fallback to executing such instructions on hardware.
Fixes: 0016a4cf5582 ("powerpc: Emulate most Book I instructions in emulate_step()")
Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
---
Previous versions can be found at:
v3: https://lore.kernel.org/linuxppc-dev/20210204071432.116439-1-sandipan@linux.ibm.com/
v2: https://lore.kernel.org/linuxppc-dev/20210203063841.431063-1-sandipan@linux.ibm.com/
v1: https://lore.kernel.org/linuxppc-dev/20201119054139.244083-1-sandipan@linux.ibm.com/
Changes in v4:
- Fixed grammar and switch-case alignment.
Changes in v3:
- Consolidated the checks as suggested by Naveen.
- Consolidated load/store changes into a single patch.
- Included floating-point load/store and update instructions.
Changes in v2:
- Jump to unknown_opcode instead of returning -1 for invalid
instruction forms.
---
arch/powerpc/lib/sstep.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index e96cff845ef7..11f14b209d7f 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -3017,6 +3017,20 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
}
+ if (OP_IS_LOAD_STORE(op->type) && (op->type & UPDATE)) {
+ switch (GETTYPE(op->type)) {
+ case LOAD:
+ if (ra == rd)
+ goto unknown_opcode;
+ fallthrough;
+ case STORE:
+ case LOAD_FP:
+ case STORE_FP:
+ if (ra == 0)
+ goto unknown_opcode;
+ }
+ }
+
#ifdef CONFIG_VSX
if ((GETTYPE(op->type) == LOAD_VSX ||
GETTYPE(op->type) == STORE_VSX) &&
--
2.25.1
^ permalink raw reply related
* [PATCH v4 2/2] powerpc: sstep: Fix darn emulation
From: Sandipan Das @ 2021-02-04 8:07 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, ananth, jniethe5, paulus, naveen.n.rao,
linuxppc-dev, dja
In-Reply-To: <20210204080744.135785-1-sandipan@linux.ibm.com>
Commit 8813ff49607e ("powerpc/sstep: Check instruction
validity against ISA version before emulation") introduced
a proper way to skip unknown instructions. This makes sure
that the same is used for the darn instruction when the
range selection bits have a reserved value.
Fixes: a23987ef267a ("powerpc: sstep: Add support for darn instruction")
Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
---
arch/powerpc/lib/sstep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 11f14b209d7f..683f7c20f74b 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1916,7 +1916,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
goto compute_done;
}
- return -1;
+ goto unknown_opcode;
#ifdef __powerpc64__
case 777: /* modsd */
if (!cpu_has_feature(CPU_FTR_ARCH_300))
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v2 0/5] shoot lazy tlbs
From: Nicholas Piggin @ 2021-02-04 8:21 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-arch, linux-mm, linuxppc-dev, Andy Lutomirski
In-Reply-To: <20201214065312.270062-1-npiggin@gmail.com>
I'll ask Andrew to put this in -mm if no objections.
The series now doesn't touch other archs in non-trivial ways, and core code
is functionally not changed much / at all if the option is not selected so
it's actually pretty simple aside from the powerpc change.
Thanks,
Nick
Excerpts from Nicholas Piggin's message of December 14, 2020 4:53 pm:
> This is another rebase, on top of mainline now (don't need the
> asm-generic tree), and without any x86 or membarrier changes.
> This makes the series far smaller and more manageable and
> without the controversial bits.
>
> Thanks,
> Nick
>
> Nicholas Piggin (5):
> lazy tlb: introduce lazy mm refcount helper functions
> lazy tlb: allow lazy tlb mm switching to be configurable
> lazy tlb: shoot lazies, a non-refcounting lazy tlb option
> powerpc: use lazy mm refcount helper functions
> powerpc/64s: enable MMU_LAZY_TLB_SHOOTDOWN
>
> arch/Kconfig | 30 ++++++++++
> arch/arm/mach-rpc/ecard.c | 2 +-
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/kernel/smp.c | 2 +-
> arch/powerpc/mm/book3s64/radix_tlb.c | 4 +-
> fs/exec.c | 4 +-
> include/linux/sched/mm.h | 20 +++++++
> kernel/cpu.c | 2 +-
> kernel/exit.c | 2 +-
> kernel/fork.c | 52 ++++++++++++++++
> kernel/kthread.c | 11 ++--
> kernel/sched/core.c | 88 ++++++++++++++++++++--------
> kernel/sched/sched.h | 4 +-
> 13 files changed, 184 insertions(+), 38 deletions(-)
>
> --
> 2.23.0
>
>
^ permalink raw reply
* Re: [PATCH v2 1/3] powerpc: sstep: Fix load and update emulation
From: Naveen N. Rao @ 2021-02-04 8:27 UTC (permalink / raw)
To: Segher Boessenkool
Cc: ravi.bangoria, ananth, jniethe5, paulus, Sandipan Das,
linuxppc-dev, dja
In-Reply-To: <20210203211732.GD30983@gate.crashing.org>
On 2021/02/03 03:17PM, Segher Boessenkool wrote:
> On Wed, Feb 03, 2021 at 03:19:09PM +0530, Naveen N. Rao wrote:
> > On 2021/02/03 12:08PM, Sandipan Das wrote:
> > > The Power ISA says that the fixed-point load and update
> > > instructions must neither use R0 for the base address (RA)
> > > nor have the destination (RT) and the base address (RA) as
> > > the same register. In these cases, the instruction is
> > > invalid.
>
> > > However, the following behaviour is observed using some
> > > invalid opcodes where RA = RT.
> > >
> > > An userspace program using an invalid instruction word like
> > > 0xe9ce0001, i.e. "ldu r14, 0(r14)", runs and exits without
> > > getting terminated abruptly. The instruction performs the
> > > load operation but does not write the effective address to
> > > the base address register.
> >
> > While the processor (p8 in my test) doesn't seem to be throwing an
> > exception, I don't think it is necessarily loading the value. Qemu
> > throws an exception though. It's probably best to term the behavior as
> > being undefined.
>
> Power8 does:
>
> Load with Update Instructions (RA = 0)
> EA is placed into R0.
> Load with Update Instructions (RA = RT)
> EA is placed into RT. The storage operand addressed by EA is
> accessed, but the data returned by the load is discarded.
I'm actually not seeing that. This is what I am testing with:
li 8,0xaaa
mr 6,1
std 8,64(6)
#ldu 6,64(6)
.long 0xe8c60041
And, r6 always ends up with 0xaea. It changes with the value I put into
r6 though.
Granted, this is all up in the air, but it does look like there is more
going on and the value isn't the EA or the value at the address.
>
> Power9 does:
>
> Load with Update Instructions (RA = 0)
> EA is placed into R0.
> Load with Update Instructions (RA = RT)
> The storage operand addressed by EA is accessed. The displacement
> field is added to the data returned by the load and placed into RT.
>
> Both UMs also say
>
> Invalid Forms
> In general, the POWER9 core handles invalid forms of instructions in
> the manner that is most convenient for the particular case (within
> the scope of meeting the boundedly-undefined definition described in
> the Power ISA). This document specifies the behavior for these
> cases. However, it is not recommended that software or other system
> facilities make use of the POWER9 behavior in these cases because
> such behavior might be different in another processor that
> implements the Power ISA.
>
> (or POWER8 instead of POWER9 of course). Always complaining about most
> invalid forms seems wise, certainly if not all recent CPUs behave the
> same :-)
Agreed.
- Naveen
^ permalink raw reply
* Re: [PATCH v4 1/2] powerpc: sstep: Fix load-store and update emulation
From: Naveen N. Rao @ 2021-02-04 8:31 UTC (permalink / raw)
To: Sandipan Das; +Cc: ravi.bangoria, ananth, jniethe5, paulus, linuxppc-dev, dja
In-Reply-To: <20210204080744.135785-1-sandipan@linux.ibm.com>
On 2021/02/04 01:37PM, Sandipan Das wrote:
> The Power ISA says that the fixed-point load and update
> instructions must neither use R0 for the base address (RA)
> nor have the destination (RT) and the base address (RA) as
> the same register. Similarly, for fixed-point stores and
> floating-point loads and stores, the instruction is invalid
> when R0 is used as the base address (RA).
>
> This is applicable to the following instructions.
> * Load Byte and Zero with Update (lbzu)
> * Load Byte and Zero with Update Indexed (lbzux)
> * Load Halfword and Zero with Update (lhzu)
> * Load Halfword and Zero with Update Indexed (lhzux)
> * Load Halfword Algebraic with Update (lhau)
> * Load Halfword Algebraic with Update Indexed (lhaux)
> * Load Word and Zero with Update (lwzu)
> * Load Word and Zero with Update Indexed (lwzux)
> * Load Word Algebraic with Update Indexed (lwaux)
> * Load Doubleword with Update (ldu)
> * Load Doubleword with Update Indexed (ldux)
> * Load Floating Single with Update (lfsu)
> * Load Floating Single with Update Indexed (lfsux)
> * Load Floating Double with Update (lfdu)
> * Load Floating Double with Update Indexed (lfdux)
> * Store Byte with Update (stbu)
> * Store Byte with Update Indexed (stbux)
> * Store Halfword with Update (sthu)
> * Store Halfword with Update Indexed (sthux)
> * Store Word with Update (stwu)
> * Store Word with Update Indexed (stwux)
> * Store Doubleword with Update (stdu)
> * Store Doubleword with Update Indexed (stdux)
> * Store Floating Single with Update (stfsu)
> * Store Floating Single with Update Indexed (stfsux)
> * Store Floating Double with Update (stfdu)
> * Store Floating Double with Update Indexed (stfdux)
>
> E.g. the following behaviour is observed for an invalid
> load and update instruction having RA = RT.
>
> While a userspace program having an instruction word like
> 0xe9ce0001, i.e. ldu r14, 0(r14), runs without getting
> receiving a SIGILL on a Power system (observed on P8 and
> P9), the outcome of executing that instruction word varies
> and its behaviour can be considered to be undefined.
>
> Attaching an uprobe at that instruction's address results
> in emulation which currently performs the load as well as
> writes the effective address back to the base register.
> This might not match the outcome from hardware.
>
> To remove any inconsistencies, this adds additional checks
> for the aforementioned instructions to make sure that the
> emulation infrastructure treats them as unknown. The kernel
> can then fallback to executing such instructions on hardware.
>
> Fixes: 0016a4cf5582 ("powerpc: Emulate most Book I instructions in emulate_step()")
> Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
> ---
> Previous versions can be found at:
> v3: https://lore.kernel.org/linuxppc-dev/20210204071432.116439-1-sandipan@linux.ibm.com/
> v2: https://lore.kernel.org/linuxppc-dev/20210203063841.431063-1-sandipan@linux.ibm.com/
> v1: https://lore.kernel.org/linuxppc-dev/20201119054139.244083-1-sandipan@linux.ibm.com/
>
> Changes in v4:
> - Fixed grammar and switch-case alignment.
>
> Changes in v3:
> - Consolidated the checks as suggested by Naveen.
> - Consolidated load/store changes into a single patch.
> - Included floating-point load/store and update instructions.
>
> Changes in v2:
> - Jump to unknown_opcode instead of returning -1 for invalid
> instruction forms.
>
> ---
> arch/powerpc/lib/sstep.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
For the series:
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
- Naveen
^ permalink raw reply
* Re: [PATCH RFC v1 5/6] xen-swiotlb: convert variables to arrays
From: Christoph Hellwig @ 2021-02-04 8:40 UTC (permalink / raw)
To: Dongli Zhang
Cc: ulf.hansson, airlied, joonas.lahtinen, dri-devel, linux-kernel,
bhelgaas, paulus, hpa, hch, m.szyprowski, sstabellini,
adrian.hunter, x86, joe.jin, mingo, peterz, mingo, bskeggs,
linux-pci, xen-devel, matthew.auld, thomas.lendacky, konrad.wilk,
intel-gfx, jani.nikula, bp, rodrigo.vivi, nouveau,
boris.ostrovsky, chris, jgross, tsbogend, robin.murphy, linux-mmc,
linux-mips, iommu, tglx, bauerman, daniel, akpm, linuxppc-dev,
rppt
In-Reply-To: <20210203233709.19819-6-dongli.zhang@oracle.com>
So one thing that has been on my mind for a while: I'd really like
to kill the separate dma ops in Xen swiotlb. If we compare xen-swiotlb
to swiotlb the main difference seems to be:
- additional reasons to bounce I/O vs the plain DMA capable
- the possibility to do a hypercall on arm/arm64
- an extra translation layer before doing the phys_to_dma and vice
versa
- an special memory allocator
I wonder if inbetween a few jump labels or other no overhead enablement
options and possibly better use of the dma_range_map we could kill
off most of swiotlb-xen instead of maintaining all this code duplication?
^ permalink raw reply
* Re: [PATCH v3 28/32] powerpc/64s: interrupt implement exit logic in C
From: Nicholas Piggin @ 2021-02-04 8:53 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Michal Suchanek
In-Reply-To: <65686b53-feb4-2788-88e1-76c3714d3e97@csgroup.eu>
Excerpts from Christophe Leroy's message of February 4, 2021 6:03 pm:
>
>
> Le 04/02/2021 à 04:27, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of February 4, 2021 2:25 am:
>>>
>>>
>>> Le 25/02/2020 à 18:35, Nicholas Piggin a écrit :
>>>> Implement the bulk of interrupt return logic in C. The asm return code
>>>> must handle a few cases: restoring full GPRs, and emulating stack store.
>>>>
>>>
>>>
>>>> +notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsigned long msr)
>>>> +{
>>>> + unsigned long *ti_flagsp = ¤t_thread_info()->flags;
>>>> + unsigned long flags;
>>>> +
>>>> + if (IS_ENABLED(CONFIG_PPC_BOOK3S) && unlikely(!(regs->msr & MSR_RI)))
>>>> + unrecoverable_exception(regs);
>>>> + BUG_ON(regs->msr & MSR_PR);
>>>> + BUG_ON(!FULL_REGS(regs));
>>>> +
>>>> + local_irq_save(flags);
>>>> +
>>>> + if (regs->softe == IRQS_ENABLED) {
>>>> + /* Returning to a kernel context with local irqs enabled. */
>>>> + WARN_ON_ONCE(!(regs->msr & MSR_EE));
>>>> +again:
>>>> + if (IS_ENABLED(CONFIG_PREEMPT)) {
>>>> + /* Return to preemptible kernel context */
>>>> + if (unlikely(*ti_flagsp & _TIF_NEED_RESCHED)) {
>>>> + if (preempt_count() == 0)
>>>> + preempt_schedule_irq();
>>>> + }
>>>> + }
>>>> +
>>>> + trace_hardirqs_on();
>>>> + __hard_EE_RI_disable();
>>>> + if (unlikely(lazy_irq_pending())) {
>>>> + __hard_RI_enable();
>>>> + irq_soft_mask_set(IRQS_ALL_DISABLED);
>>>> + trace_hardirqs_off();
>>>> + local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
>>>> + /*
>>>> + * Can't local_irq_enable in case we are in interrupt
>>>> + * context. Must replay directly.
>>>> + */
>>>> + replay_soft_interrupts();
>>>> + irq_soft_mask_set(flags);
>>>> + /* Took an interrupt, may have more exit work to do. */
>>>> + goto again;
>>>> + }
>>>> + local_paca->irq_happened = 0;
>>>> + irq_soft_mask_set(IRQS_ENABLED);
>>>> + } else {
>>>> + /* Returning to a kernel context with local irqs disabled. */
>>>> + trace_hardirqs_on();
>>>> + __hard_EE_RI_disable();
>>>> + if (regs->msr & MSR_EE)
>>>> + local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>>>> + }
>>>> +
>>>> +
>>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>>> + local_paca->tm_scratch = regs->msr;
>>>> +#endif
>>>> +
>>>> + /*
>>>> + * We don't need to restore AMR on the way back to userspace for KUAP.
>>>> + * The value of AMR only matters while we're in the kernel.
>>>> + */
>>>> + kuap_restore_amr(regs);
>>>
>>> Is that correct to restore KUAP state here ? Shouldn't we have it at lower level in assembly ?
>>>
>>> Isn't there a risk that someone manages to call interrupt_exit_kernel_prepare() or the end of it in
>>> a way or another, and get the previous KUAP state restored by this way ?
>>
>> I'm not sure if there much more risk if it's here rather than the
>> instruction being in another place in the code.
>>
>> There's a lot of user access around the kernel too if you want to find a
>> gadget to unlock KUAP then I suppose there is a pretty large attack
>> surface.
>
> My understanding is that user access scope is strictly limited, for instance we enforce the
> begin/end of user access to be in the same function, and we refrain from calling any other function
> inside the user access window. x86 even have 'objtool' to enforce it at build time. So in theory
> there is no way to get out of the function while user access is open.
>
> Here with the interrupt exit function it is free beer. You have a place where you re-open user
> access and return with a simple blr. So that's open bar. If someone manages to just call the
> interrupt exit function, then user access remains open
Hmm okay maybe that's a good point.
>>> Also, it looks a bit strange to have kuap_save_amr_and_lock() done at lowest level in assembly, and
>>> kuap_restore_amr() done in upper level. That looks unbalanced.
>>
>> I'd like to bring the entry assembly into C.
>>
>
> I really think it's not a good idea. We'll get better control and readability by keeping it at the
> lowest possible level in assembly.
>
> x86 only save and restore SMAC state in assembly.
Okay we could go the other way and move the unlock into asm then.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc64/idle: Fix SP offsets when saving GPRs
From: Nicholas Piggin @ 2021-02-04 9:05 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev, Michael Ellerman
In-Reply-To: <C90JVYFOGWU0.1C2DRATSDH0FM@geist>
Excerpts from Christopher M. Riedl's message of February 4, 2021 4:59 pm:
> On Sat Jan 30, 2021 at 7:44 AM CST, Nicholas Piggin wrote:
>> Excerpts from Michael Ellerman's message of January 30, 2021 9:32 pm:
>> > "Christopher M. Riedl" <cmr@codefail.de> writes:
>> >> The idle entry/exit code saves/restores GPRs in the stack "red zone"
>> >> (Protected Zone according to PowerPC64 ELF ABI v2). However, the offset
>> >> used for the first GPR is incorrect and overwrites the back chain - the
>> >> Protected Zone actually starts below the current SP. In practice this is
>> >> probably not an issue, but it's still incorrect so fix it.
>> >
>> > Nice catch.
>> >
>> > Corrupting the back chain means you can't backtrace from there, which
>> > could be confusing for debugging one day.
>>
>> Yeah, we seem to have got away without noticing because the CPU will
>> wake up and return out of here before it tries to unwind the stack,
>> but if you tried to walk it by hand if the CPU got stuck in idle or
>> something, then we'd get confused.
>>
>> > It does make me wonder why we don't just create a stack frame and use
>> > the normal macros? It would use a bit more stack space, but we shouldn't
>> > be short of stack space when going idle.
>> >
>> > Nick, was there a particular reason for using the red zone?
>>
>> I don't recall a particular reason, I think a normal stack frame is
>> probably a good idea.
>
> I'll send a version using STACKFRAMESIZE - I assume that's the "normal"
> stack frame :)
>
I think STACKFRAMESIZE is STACK_FRAME_OVERHEAD + NVGPRs. LR and CR can
be saved in the caller's frame so that should be okay.
> I admit I am a bit confused when I saw the similar but much smaller
> STACK_FRAME_OVERHEAD which is also used in _some_ cases to save/restore
> a few registers.
Yeah if you don't need to save all nvgprs you can use caller's frame
plus a few bytes in the minimum frame as volatile storage.
STACK_FRAME_OVERHEAD should be 32 on LE, but I think the problem is a
lot of asm uses it and hasn't necessarily been audited to make sure it's
not assuming it's bigger. You could actually use STACK_FRAME_MIN_SIZE
for new code, maybe we add a STACK_FRAME_MIN_NVGPR_SIZE to match and
use that.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v7 39/42] powerpc: move NMI entry/exit code into wrapper
From: Michael Ellerman @ 2021-02-04 10:15 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Athira Rajeev, Nicholas Piggin
In-Reply-To: <20210130130852.2952424-40-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> This moves the common NMI entry and exit code into the interrupt handler
> wrappers.
>
> This changes the behaviour of soft-NMI (watchdog) and HMI interrupts, and
> also MCE interrupts on 64e, by adding missing parts of the NMI entry to
> them.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/interrupt.h | 28 ++++++++++++++++++++++
> arch/powerpc/kernel/mce.c | 11 ---------
> arch/powerpc/kernel/traps.c | 35 +++++-----------------------
> arch/powerpc/kernel/watchdog.c | 10 ++++----
> 4 files changed, 38 insertions(+), 46 deletions(-)
This is unhappy when injecting SLB multi-hits:
root@p86-2:~# echo PPC_SLB_MULTIHIT > /sys/kernel/debug/provoke-crash/DIRECT
[ 312.496026][ T1344] kernel BUG at arch/powerpc/include/asm/interrupt.h:152!
[ 312.496037][ T1344] Oops: Exception in kernel mode, sig: 5 [#1]
[ 312.496045][ T1344] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
[ 312.496053][ T1344] Modules linked in: squashfs dm_multipath scsi_dh_rdac scsi_dh_alua pseries_rng rng_core vmx_crypto gf128mul crc32c_vpmsum ip_tables x_tables autofs4
[ 312.496085][ T1344] CPU: 19 PID: 1344 Comm: bash Not tainted 5.11.0-rc2-gcc-8.2.0-00123-g3fadced17474-dirty #638
[ 312.496096][ T1344] NIP: c000000000ef1618 LR: c000000000ef1600 CTR: 0000000000000000
[ 312.496104][ T1344] REGS: c00000001eb4ba00 TRAP: 0700 Not tainted (5.11.0-rc2-gcc-8.2.0-00123-g3fadced17474-dirty)
[ 312.496113][ T1344] MSR: 8000000000021033 <SF,ME,IR,DR,RI,LE> CR: 48428284 XER: 00000000
[ 312.496132][ T1344] CFAR: c000000000ef28b8 IRQMASK: 3
[ 312.496132][ T1344] GPR00: c000000000ef15e4 c00000001eb4bca0 c000000001a53900 0000000000000001
[ 312.496132][ T1344] GPR04: c0000000017e7230 4000000000000000 3ffffffffffffffe 0000000000006d66
[ 312.496132][ T1344] GPR08: c00000001ec6fe80 0000000000000001 c00000000e72d380 0000000000001001
[ 312.496132][ T1344] GPR12: 0000000000000010 c00000001ec6fe80 00000100235ad1d0 000000013c2fb738
[ 312.496132][ T1344] GPR16: 000000013c210ae0 0000000000000000 0000000022000000 00000100235af740
[ 312.496132][ T1344] GPR20: 0000000000000000 0000000000000001 000000013c2a3ca0 c000000033c50040
[ 312.496132][ T1344] GPR24: c0000000100fbd80 0000000000000000 c000000001a7dc78 0000000000000001
[ 312.496132][ T1344] GPR28: c00000001eb4bd60 0000000000000001 0000000000000000 0000000000000001
[ 312.496229][ T1344] NIP [c000000000ef1618] machine_check_early+0x138/0x1f0
[ 312.496241][ T1344] LR [c000000000ef1600] machine_check_early+0x120/0x1f0
[ 312.496249][ T1344] Call Trace:
[ 312.496254][ T1344] [c00000001eb4bca0] [c000000000ef15e4] machine_check_early+0x104/0x1f0 (unreliable)
[ 312.496267][ T1344] [c00000001eb4bcf0] [c000000000008394] machine_check_early_common+0x134/0x1f0
[ 312.496279][ T1344] --- interrupt: 200 at lkdtm_PPC_SLB_MULTIHIT+0x128/0x138
[ 312.496290][ T1344] NIP: c0000000009cfea8 LR: c0000000009cfea0 CTR: 0000000000000000
[ 312.496298][ T1344] REGS: c00000001eb4bd60 TRAP: 0200 Not tainted (5.11.0-rc2-gcc-8.2.0-00123-g3fadced17474-dirty)
[ 312.496307][ T1344] MSR: 8000000000209033 <SF,EE,ME,IR,DR,RI,LE> CR: 24428284 XER: 00000000
[ 312.496326][ T1344] CFAR: 000000000000021c DAR: c008000003880000 DSISR: 00000080 IRQMASK: 0
[ 312.496326][ T1344] GPR00: c0000000009cfea0 c0000000100fbb80 c000000001a53900 c008000003880000
[ 312.496326][ T1344] GPR04: 00000000000000b0 c000000401023440 8e01c533000000c0 0000000000bf50d9
[ 312.496326][ T1344] GPR08: 000000000fffffff 0000000000000021 000000000000001c c008000004880000
[ 312.496326][ T1344] GPR12: 0000000048428222 c00000001ec6fe80 00000100235ad1d0 000000013c2fb738
[ 312.496326][ T1344] GPR16: 000000013c210ae0 0000000000000000 0000000022000000 00000100235af740
[ 312.496326][ T1344] GPR20: 0000000000000000 0000000000000001 000000013c2a3ca0 c000000033c50040
[ 312.496326][ T1344] GPR24: c0000000100fbd80 0000000000000000 0000000000000011 c000000001a2ffb8
[ 312.496326][ T1344] GPR28: 00000000000004b0 c000000033c50000 c000000001105298 c008000003880000
[ 312.496427][ T1344] NIP [c0000000009cfea8] lkdtm_PPC_SLB_MULTIHIT+0x128/0x138
[ 312.496437][ T1344] LR [c0000000009cfea0] lkdtm_PPC_SLB_MULTIHIT+0x120/0x138
[ 312.496446][ T1344] --- interrupt: 200
[ 312.496451][ T1344] [c0000000100fbbf0] [c0000000009cb530] lkdtm_do_action+0x40/0x80
[ 312.496462][ T1344] [c0000000100fbc10] [c0000000009cbdfc] direct_entry+0x16c/0x350
[ 312.496473][ T1344] [c0000000100fbcc0] [c0000000007a7590] full_proxy_write+0x90/0xe0
[ 312.496484][ T1344] [c0000000100fbd10] [c00000000046b090] vfs_write+0xf0/0x310
[ 312.496496][ T1344] [c0000000100fbd60] [c00000000046b48c] ksys_write+0x7c/0x140
[ 312.496507][ T1344] [c0000000100fbdb0] [c000000000036340] system_call_exception+0x1a0/0x2e0
[ 312.496518][ T1344] [c0000000100fbe10] [c00000000000d360] system_call_common+0xf0/0x27c
[ 312.496528][ T1344] Instruction dump:
[ 312.496534][ T1344] 7c7b1b78 e93a0000 75290040 41820008 480000a8 4800125d 60000000 e94d0968
[ 312.496552][ T1344] 812a0000 55290216 7d290034 5529d97e <0b090000> 812a0000 3d29ffef 912a0000
[ 312.496571][ T1344] ---[ end trace 1cd2275de93cc3c3 ]---
[ 312.500581][ T1344]
[ 312.500705][ C19] RTAS: event: 1, Type: Unknown (0), Severity: 3
[ 312.501068][ T0] ------------[ cut here ]------------
[ 312.501081][ T0] WARNING: CPU: 19 PID: 0 at kernel/rcu/tree.c:632 rcu_eqs_enter.isra.59+0x140/0x160
[ 312.501103][ T0] Modules linked in: squashfs dm_multipath scsi_dh_rdac scsi_dh_alua pseries_rng rng_core vmx_crypto gf128mul crc32c_vpmsum ip_tables x_tables autofs4
[ 312.501152][ T0] CPU: 19 PID: 0 Comm: swapper/19 Tainted: G D 5.11.0-rc2-gcc-8.2.0-00123-g3fadced17474-dirty #638
[ 312.501173][ T0] NIP: c000000000ef2830 LR: c000000000c287c0 CTR: 0000000000000000
[ 312.501187][ T0] REGS: c000000008d5fa50 TRAP: 0700 Tainted: G D (5.11.0-rc2-gcc-8.2.0-00123-g3fadced17474-dirty)
[ 312.501203][ T0] MSR: 800000000282b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 28000248 XER: 20000009
[ 312.501248][ T0] CFAR: c000000000ef2728 IRQMASK: 1
[ 312.501248][ T0] GPR00: c000000000c287c0 c000000008d5fcf0 c000000001a53900 00000048c283bf7e
[ 312.501248][ T0] GPR04: c00000000199fae0 0000000000000001 0000000000000800 4000000000000000
[ 312.501248][ T0] GPR08: c00000001ec6fe80 c0000003fdb93980 3ffffffffffffffe 000000000098967f
[ 312.501248][ T0] GPR12: 00000000ffffffff c00000001ec6fe80 0000000000000000 000000001ef2fa60
[ 312.501248][ T0] GPR16: 0000000000000000 0000000000000000 c0000007fffebff8 c000000000051ca0
[ 312.501248][ T0] GPR20: c0000007fffec628 c00000000199fae0 0000000000000001 c0000003fdb917c8
[ 312.501248][ T0] GPR24: 0000000000000001 00000048c283bf7e 0000000000000000 0000000000000001
[ 312.501248][ T0] GPR28: c0000003fdb917c8 c00000000199fae0 0000000000000001 c0000000013d3980
[ 312.501428][ T0] NIP [c000000000ef2830] rcu_eqs_enter.isra.59+0x140/0x160
[ 312.501445][ T0] LR [c000000000c287c0] cpuidle_enter_state+0x2f0/0x540
[ 312.501469][ T0] Call Trace:
[ 312.501481][ T0] [c000000008d5fcf0] [c000000008d5fd40] 0xc000000008d5fd40 (unreliable)
[ 312.501508][ T0] [c000000008d5fd20] [c0000003fdb917c8] 0xc0000003fdb917c8
[ 312.501524][ T0] [c000000008d5fd80] [c000000000c28ab0] cpuidle_enter+0x50/0x70
[ 312.501544][ T0] [c000000008d5fdc0] [c00000000019000c] call_cpuidle+0x4c/0x80
[ 312.501564][ T0] [c000000008d5fde0] [c0000000001906a0] do_idle+0x390/0x3e0
[ 312.501581][ T0] [c000000008d5fe70] [c00000000019096c] cpu_startup_entry+0x3c/0x40
[ 312.501602][ T0] [c000000008d5fea0] [c000000000054838] start_secondary+0x5b8/0x9b0
[ 312.501619][ T0] [c000000008d5ff90] [c00000000000c654] start_secondary_prolog+0x10/0x14
[ 312.501642][ T0] Instruction dump:
[ 312.501657][ T0] 60000000 e8010040 7c0803a6 e94d0030 39200000 38210030 7fff5214 f93f00f0
[ 312.501692][ T0] ebe1fff8 4bfffd14 60000000 60000000 <0fe00000> 4bfffef8 60000000 60000000
[ 312.501721][ T0] ---[ end trace 1cd2275de93cc3c4 ]---
147 static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct interrupt_nmi_state *state)
148 {
149 if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64) ||
150 !firmware_has_feature(FW_FEATURE_LPAR) ||
151 radix_enabled() || (mfmsr() & MSR_DR))
152 nmi_exit();
So presumably it's:
#define __nmi_exit() \
do { \
BUG_ON(!in_nmi()); \
cheers
^ permalink raw reply
* RE: [PATCH v2 1/3] powerpc: sstep: Fix load and update emulation
From: David Laight @ 2021-02-04 10:29 UTC (permalink / raw)
To: 'Segher Boessenkool', Naveen N. Rao
Cc: ravi.bangoria@linux.ibm.com, ananth@linux.ibm.com,
jniethe5@gmail.com, paulus@samba.org, Sandipan Das,
linuxppc-dev@lists.ozlabs.org, dja@axtens.net
In-Reply-To: <20210203211732.GD30983@gate.crashing.org>
From: Segher Boessenkool
> Sent: 03 February 2021 21:18
...
> Power9 does:
>
> Load with Update Instructions (RA = 0)
> EA is placed into R0.
Does that change the value of 0?
Rather reminds me of some 1960s era systems that had the small integers
at fixed (global) addresses.
FORTRAN always passes by reference, pass 0 and the address of the global
zero location was passed, the called function could change 0 to 1 for
the entire computer!
> Load with Update Instructions (RA = RT)
> The storage operand addressed by EA is accessed. The displacement
> field is added to the data returned by the load and placed into RT.
Shame that isn't standard - could be used to optimise some code.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 01/13] powerpc/powernv: remove get_cxl_module
From: Michael Ellerman @ 2021-02-04 10:44 UTC (permalink / raw)
To: Christoph Hellwig, Frederic Barrat, Andrew Donnellan,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Daniel Vetter, Jessica Yu, Josh Poimboeuf, Jiri Kosina,
Miroslav Benes, Petr Mladek, Joe Lawrence
Cc: Michal Marek, linux-kbuild, Masahiro Yamada, linux-kernel,
dri-devel, live-patching, linuxppc-dev
In-Reply-To: <20210202121334.1361503-2-hch@lst.de>
Christoph Hellwig <hch@lst.de> writes:
> The static inline get_cxl_module function is entirely unused since commit
> 8bf6b91a5125a ("Revert "powerpc/powernv: Add support for the cxl kernel
> api on the real phb"), so remove it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
> ---
> arch/powerpc/platforms/powernv/pci-cxl.c | 22 ----------------------
> 1 file changed, 22 deletions(-)
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
^ permalink raw reply
* [PATCH v2] powerpc/uprobes: Validation for prefixed instruction
From: Ravi Bangoria @ 2021-02-04 10:47 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, jniethe5, oleg, rostedt, linux-kernel, paulus,
sandipan, naveen.n.rao, linuxppc-dev
Don't allow Uprobe on 2nd word of a prefixed instruction. As per
ISA 3.1, prefixed instruction should not cross 64-byte boundary.
So don't allow Uprobe on such prefixed instruction as well.
There are two ways probed instruction is changed in mapped pages.
First, when Uprobe is activated, it searches for all the relevant
pages and replace instruction in them. In this case, if we notice
that probe is on the 2nd word of prefixed instruction, error out
directly. Second, when Uprobe is already active and user maps a
relevant page via mmap(), instruction is replaced via mmap() code
path. But because Uprobe is invalid, entire mmap() operation can
not be stopped. In this case just print an error and continue.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
v1: http://lore.kernel.org/r/20210119091234.76317-1-ravi.bangoria@linux.ibm.com
v1->v2:
- Instead of introducing new arch hook from verify_opcode(), use
existing hook arch_uprobe_analyze_insn().
- Add explicit check for prefixed instruction crossing 64-byte
boundary. If probe is on such instruction, throw an error.
arch/powerpc/kernel/uprobes.c | 66 ++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index e8a63713e655..485d19a2a31f 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -7,6 +7,7 @@
* Adapted from the x86 port by Ananth N Mavinakayanahalli <ananth@in.ibm.com>
*/
#include <linux/kernel.h>
+#include <linux/highmem.h>
#include <linux/sched.h>
#include <linux/ptrace.h>
#include <linux/uprobes.h>
@@ -28,6 +29,69 @@ bool is_trap_insn(uprobe_opcode_t *insn)
return (is_trap(*insn));
}
+#ifdef CONFIG_PPC64
+static int get_instr(struct mm_struct *mm, unsigned long addr, u32 *instr)
+{
+ struct page *page;
+ struct vm_area_struct *vma;
+ void *kaddr;
+ unsigned int gup_flags = FOLL_FORCE | FOLL_SPLIT_PMD;
+
+ if (get_user_pages_remote(mm, addr, 1, gup_flags, &page, &vma, NULL) <= 0)
+ return -EINVAL;
+
+ kaddr = kmap_atomic(page);
+ *instr = *((u32 *)(kaddr + (addr & ~PAGE_MASK)));
+ kunmap_atomic(kaddr);
+ put_page(page);
+ return 0;
+}
+
+static int validate_prefixed_instr(struct mm_struct *mm, unsigned long addr)
+{
+ struct ppc_inst inst;
+ u32 prefix, suffix;
+
+ /*
+ * No need to check if addr is pointing to beginning of the
+ * page. Even if probe is on a suffix of page-unaligned
+ * prefixed instruction, hw will raise exception and kernel
+ * will send SIGBUS.
+ */
+ if (!(addr & ~PAGE_MASK))
+ return 0;
+
+ if (get_instr(mm, addr, &prefix) < 0)
+ return -EINVAL;
+ if (get_instr(mm, addr + 4, &suffix) < 0)
+ return -EINVAL;
+
+ inst = ppc_inst_prefix(prefix, suffix);
+ if (ppc_inst_prefixed(inst) && (addr & 0x3F) == 0x3C) {
+ printk_ratelimited("Cannot register a uprobe on 64 byte "
+ "unaligned prefixed instruction\n");
+ return -EINVAL;
+ }
+
+ suffix = prefix;
+ if (get_instr(mm, addr - 4, &prefix) < 0)
+ return -EINVAL;
+
+ inst = ppc_inst_prefix(prefix, suffix);
+ if (ppc_inst_prefixed(inst)) {
+ printk_ratelimited("Cannot register a uprobe on the second "
+ "word of prefixed instruction\n");
+ return -EINVAL;
+ }
+ return 0;
+}
+#else
+static int validate_prefixed_instr(struct mm_struct *mm, unsigned long addr)
+{
+ return 0;
+}
+#endif
+
/**
* arch_uprobe_analyze_insn
* @mm: the probed address space.
@@ -41,7 +105,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
if (addr & 0x03)
return -EINVAL;
- return 0;
+ return validate_prefixed_instr(mm, addr);
}
/*
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction
From: Ravi Bangoria @ 2021-02-04 10:49 UTC (permalink / raw)
To: mpe
Cc: Ravi Bangoria, jniethe5, oleg, rostedt, linux-kernel, paulus,
sandipan, naveen.n.rao, linuxppc-dev
In-Reply-To: <20210204104703.273429-1-ravi.bangoria@linux.ibm.com>
On 2/4/21 4:17 PM, Ravi Bangoria wrote:
> Don't allow Uprobe on 2nd word of a prefixed instruction. As per
> ISA 3.1, prefixed instruction should not cross 64-byte boundary.
> So don't allow Uprobe on such prefixed instruction as well.
>
> There are two ways probed instruction is changed in mapped pages.
> First, when Uprobe is activated, it searches for all the relevant
> pages and replace instruction in them. In this case, if we notice
> that probe is on the 2nd word of prefixed instruction, error out
> directly. Second, when Uprobe is already active and user maps a
> relevant page via mmap(), instruction is replaced via mmap() code
> path. But because Uprobe is invalid, entire mmap() operation can
> not be stopped. In this case just print an error and continue.
@mpe,
arch_uprobe_analyze_insn() can return early if
cpu_has_feature(CPU_FTR_ARCH_31) is not set. But that will
miss out a rare scenario of user running binary with prefixed
instruction on p10 predecessors. Please let me know if I
should add cpu_has_feature(CPU_FTR_ARCH_31) or not.
- Ravi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox