* [PATCH v2 2/4] powerpc/va: Add a __va() variant that doesn't do input validation
2020-05-24 9:38 [PATCH v2 1/4] powerpc/instruction_dump: Fix kernel crash with show_instructions Aneesh Kumar K.V
@ 2020-05-24 9:38 ` Aneesh Kumar K.V
2020-05-24 9:38 ` [PATCH v2 3/4] powerpc: Fix instruction dumping to use address value correctly Aneesh Kumar K.V
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Aneesh Kumar K.V @ 2020-05-24 9:38 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V
On ppc64, __va(x) do check for input argument to be less than PAGE_OFFSET.
In certain code paths, we want to skip that check. Add a variant ___va(x)
to be used in such cases.
Switch the #define to static inline. __pa() still doesn't benefit from this. But
a static inline done in this patch is better than multi-line #define.
For __va() we get the type checking benefit. We still have to keep the
macro __pa(x) to avoid a large number of compilation errors with the change.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/page.h | 38 ++++++++++++++--------
arch/powerpc/mm/nohash/book3e_pgtable.c | 2 +-
arch/powerpc/platforms/powernv/opal-core.c | 4 +--
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index a63fe6f3a0ff..8e8ffde0aef8 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -9,6 +9,7 @@
#ifndef __ASSEMBLY__
#include <linux/types.h>
#include <linux/kernel.h>
+#include <linux/mmdebug.h>
#else
#include <asm/types.h>
#endif
@@ -208,30 +209,41 @@ static inline bool pfn_valid(unsigned long pfn)
* the other definitions for __va & __pa.
*/
#if defined(CONFIG_PPC32) && defined(CONFIG_BOOKE)
-#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + VIRT_PHYS_OFFSET))
+#define ___va(x) ((void *)(unsigned long)((phys_addr_t)(x) + VIRT_PHYS_OFFSET))
#define __pa(x) ((phys_addr_t)(unsigned long)(x) - VIRT_PHYS_OFFSET)
+#define __va(x) ___va(x)
#else
#ifdef CONFIG_PPC64
+
+#ifndef __ASSEMBLY__
/*
* gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET
* with -mcmodel=medium, so we use & and | instead of - and + on 64-bit.
* This also results in better code generation.
*/
-#define __va(x) \
-({ \
- VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \
- (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET); \
-})
-
-#define __pa(x) \
-({ \
- VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET); \
- (unsigned long)(x) & 0x0fffffffffffffffUL; \
-})
+static inline void *___va(phys_addr_t addr)
+{
+ return (void *)(addr | PAGE_OFFSET);
+}
+
+static inline void *__va(phys_addr_t addr)
+{
+ VIRTUAL_BUG_ON((unsigned long)(addr) >= PAGE_OFFSET);
+ return ___va(addr);
+}
+
+static inline phys_addr_t ___pa(void *addr)
+{
+ VIRTUAL_BUG_ON((unsigned long)(addr) < PAGE_OFFSET);
+ return (phys_addr_t)((unsigned long)addr & 0x0fffffffffffffffUL);
+}
+#define __pa(x) ___pa((void *)(x))
+#endif /* __ASSEMBLY__ */
#else /* 32-bit, non book E */
-#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + PAGE_OFFSET - MEMORY_START))
+#define ___va(x) ((void *)(unsigned long)((phys_addr_t)(x) + PAGE_OFFSET - MEMORY_START))
#define __pa(x) ((unsigned long)(x) - PAGE_OFFSET + MEMORY_START)
+#define __va(x) ___va(x)
#endif
#endif
diff --git a/arch/powerpc/mm/nohash/book3e_pgtable.c b/arch/powerpc/mm/nohash/book3e_pgtable.c
index 4637fdd469cf..a8ce309ce740 100644
--- a/arch/powerpc/mm/nohash/book3e_pgtable.c
+++ b/arch/powerpc/mm/nohash/book3e_pgtable.c
@@ -60,7 +60,7 @@ static void __init *early_alloc_pgtable(unsigned long size)
if (!ptr)
panic("%s: Failed to allocate %lu bytes align=0x%lx max_addr=%lx\n",
- __func__, size, size, __pa(MAX_DMA_ADDRESS));
+ __func__, size, size, (unsigned long)__pa(MAX_DMA_ADDRESS));
return ptr;
}
diff --git a/arch/powerpc/platforms/powernv/opal-core.c b/arch/powerpc/platforms/powernv/opal-core.c
index 6dba3b62269f..9a993db88212 100644
--- a/arch/powerpc/platforms/powernv/opal-core.c
+++ b/arch/powerpc/platforms/powernv/opal-core.c
@@ -192,10 +192,10 @@ static ssize_t read_opalcore(struct file *file, struct kobject *kobj,
break;
if (tpos < m->offset + m->size) {
- void *addr;
+ phys_addr_t addr;
tsz = min_t(size_t, m->offset + m->size - tpos, count);
- addr = (void *)(m->paddr + tpos - m->offset);
+ addr = m->paddr + tpos - m->offset;
memcpy(to, __va(addr), tsz);
to += tsz;
tpos += tsz;
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 3/4] powerpc: Fix instruction dumping to use address value correctly
2020-05-24 9:38 [PATCH v2 1/4] powerpc/instruction_dump: Fix kernel crash with show_instructions Aneesh Kumar K.V
2020-05-24 9:38 ` [PATCH v2 2/4] powerpc/va: Add a __va() variant that doesn't do input validation Aneesh Kumar K.V
@ 2020-05-24 9:38 ` Aneesh Kumar K.V
2020-05-24 9:38 ` [PATCH v2 4/4] powerpc: Avoid opencoding fixup_real_addr Aneesh Kumar K.V
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Aneesh Kumar K.V @ 2020-05-24 9:38 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V
Use ___va() to convert the real address that will skip the input
validation. We can get interrupts with IR=0 and with NIP value > PAGE_OFFSET.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/processor.h | 9 +++++++++
arch/powerpc/kernel/process.c | 7 ++-----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 4e53df163b92..8bf7921dceca 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -441,6 +441,15 @@ extern void cvt_fd(float *from, double *to);
extern void cvt_df(double *from, float *to);
extern void _nmask_and_or_msr(unsigned long nmask, unsigned long or_val);
+static inline unsigned long fixup_real_addr(struct pt_regs *regs,
+ unsigned long addr)
+{
+ if (!(regs->msr & MSR_IR))
+ return (unsigned long)___va(addr);
+
+ return addr;
+}
+
#ifdef CONFIG_PPC64
/*
* We handle most unaligned accesses in hardware. On the other hand
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 93bf4a766707..de07b796222d 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1262,12 +1262,9 @@ static void show_instructions(struct pt_regs *regs)
/* If executing with the IMMU off, adjust pc rather
* than print XXXXXXXX.
*/
- if (!(regs->msr & MSR_IR)) {
- pc = (unsigned long)phys_to_virt(pc);
- nip = (unsigned long)phys_to_virt(regs->nip);
- }
+ pc = fixup_real_addr(regs, pc);
+ nip = fixup_real_addr(regs, nip);
#endif
-
for (i = 0; i < NR_INSN_TO_PRINT; i++) {
int instr;
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 4/4] powerpc: Avoid opencoding fixup_real_addr
2020-05-24 9:38 [PATCH v2 1/4] powerpc/instruction_dump: Fix kernel crash with show_instructions Aneesh Kumar K.V
2020-05-24 9:38 ` [PATCH v2 2/4] powerpc/va: Add a __va() variant that doesn't do input validation Aneesh Kumar K.V
2020-05-24 9:38 ` [PATCH v2 3/4] powerpc: Fix instruction dumping to use address value correctly Aneesh Kumar K.V
@ 2020-05-24 9:38 ` Aneesh Kumar K.V
2020-06-18 12:37 ` [PATCH v2 1/4] powerpc/instruction_dump: Fix kernel crash with show_instructions Michael Ellerman
2022-03-09 17:25 ` Christophe Leroy
4 siblings, 0 replies; 6+ messages in thread
From: Aneesh Kumar K.V @ 2020-05-24 9:38 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V
Use the newly added helper.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/kernel/traps.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 051d7028e71f..1d58d88a7be1 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1508,12 +1508,10 @@ void program_check_exception(struct pt_regs *regs)
== NOTIFY_STOP)
goto bail;
- bugaddr = regs->nip;
/*
* Fixup bugaddr for BUG_ON() in real mode
*/
- if (!is_kernel_addr(bugaddr) && !(regs->msr & MSR_IR))
- bugaddr += PAGE_OFFSET;
+ bugaddr = fixup_real_addr(regs, regs->nip);
if (!(regs->msr & MSR_PR) && /* not user-mode */
report_bug(bugaddr, regs) == BUG_TRAP_TYPE_WARN) {
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 1/4] powerpc/instruction_dump: Fix kernel crash with show_instructions
2020-05-24 9:38 [PATCH v2 1/4] powerpc/instruction_dump: Fix kernel crash with show_instructions Aneesh Kumar K.V
` (2 preceding siblings ...)
2020-05-24 9:38 ` [PATCH v2 4/4] powerpc: Avoid opencoding fixup_real_addr Aneesh Kumar K.V
@ 2020-06-18 12:37 ` Michael Ellerman
2022-03-09 17:25 ` Christophe Leroy
4 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2020-06-18 12:37 UTC (permalink / raw)
To: linuxppc-dev, mpe, Aneesh Kumar K.V
On Sun, 24 May 2020 15:08:19 +0530, Aneesh Kumar K.V wrote:
> With Hard Lockup watchdog, we can hit a BUG() if we take a watchdog
> interrupt when in OPAL mode. This happens in show_instructions()
> where the kernel takes the watchdog NMI IPI with MSR_IR == 0.
> With that show_instructions() updates the variable pc in the loop
> and the second iterations will result in BUG().
>
> We hit the BUG_ON due the below check in __va()
>
> [...]
Patch 1 applied to powerpc/fixes.
[1/4] powerpc: Fix kernel crash in show_instructions() w/DEBUG_VIRTUAL
https://git.kernel.org/powerpc/c/a6e2c226c3d51fd93636320e47cabc8a8f0824c5
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 1/4] powerpc/instruction_dump: Fix kernel crash with show_instructions
2020-05-24 9:38 [PATCH v2 1/4] powerpc/instruction_dump: Fix kernel crash with show_instructions Aneesh Kumar K.V
` (3 preceding siblings ...)
2020-06-18 12:37 ` [PATCH v2 1/4] powerpc/instruction_dump: Fix kernel crash with show_instructions Michael Ellerman
@ 2022-03-09 17:25 ` Christophe Leroy
4 siblings, 0 replies; 6+ messages in thread
From: Christophe Leroy @ 2022-03-09 17:25 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe
Le 24/05/2020 à 11:38, Aneesh Kumar K.V a écrit :
> With Hard Lockup watchdog, we can hit a BUG() if we take a watchdog
> interrupt when in OPAL mode. This happens in show_instructions()
> where the kernel takes the watchdog NMI IPI with MSR_IR == 0.
> With that show_instructions() updates the variable pc in the loop
> and the second iterations will result in BUG().
>
> We hit the BUG_ON due the below check in __va()
>
> #define __va(x) \
> ({ \
> VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \
> (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET); \
> })
>
> Fixes: 4dd7554a6456 ("powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses")
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
If still relevant, this series needs rebase.
Patch 1 was applied with changes, at least patch 3 won't apply
Thanks
Christophe
> ---
> arch/powerpc/kernel/process.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 048d64c4e115..93bf4a766707 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1253,29 +1253,32 @@ struct task_struct *__switch_to(struct task_struct *prev,
> static void show_instructions(struct pt_regs *regs)
> {
> int i;
> + unsigned long nip = regs->nip;
> unsigned long pc = regs->nip - (NR_INSN_TO_PRINT * 3 / 4 * sizeof(int));
>
> printk("Instruction dump:");
>
> +#if !defined(CONFIG_BOOKE)
> + /* If executing with the IMMU off, adjust pc rather
> + * than print XXXXXXXX.
> + */
> + if (!(regs->msr & MSR_IR)) {
> + pc = (unsigned long)phys_to_virt(pc);
> + nip = (unsigned long)phys_to_virt(regs->nip);
> + }
> +#endif
> +
> for (i = 0; i < NR_INSN_TO_PRINT; i++) {
> int instr;
>
> if (!(i % 8))
> pr_cont("\n");
>
> -#if !defined(CONFIG_BOOKE)
> - /* If executing with the IMMU off, adjust pc rather
> - * than print XXXXXXXX.
> - */
> - if (!(regs->msr & MSR_IR))
> - pc = (unsigned long)phys_to_virt(pc);
> -#endif
> -
> if (!__kernel_text_address(pc) ||
> probe_kernel_address((const void *)pc, instr)) {
> pr_cont("XXXXXXXX ");
> } else {
> - if (regs->nip == pc)
> + if (nip == pc)
> pr_cont("<%08x> ", instr);
> else
> pr_cont("%08x ", instr);
^ permalink raw reply [flat|nested] 6+ messages in thread