Skip to contentSkip to conversation

MCE robustness fixes and LKDTM powerpc enhancements

PATCH Superseded Sayali Patil 3 participants 8 messages May 6 – May 13

Subsystems (4)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover MCE robustness fixes and LKDTM powerpc enhancements

Hi all,

This series fixes a panic observed while handling machine check exceptions (MCEs) from real mode. It also improves the reliability of the PPC_SLB_MULTIHIT test by adding isync instructions after slbmte operations, and introduces a new LKDTM trigger (PPC_RADIX_TLBIEL) to validate MCE behavior on radix MMU.

Please review the patches and provide any feedback or suggestions for improvement.

Thanks,
Sayali

Sayali Patil (3):
powerpc/time: remove preempt_disable/enable from arch_irq_work_raise() lkdtm/powerpc: add isync after slbmte to enforce SLB update ordering lkdtm/powerpc: add PPC_RADIX_TLBIEL test for radix MCE validation

 arch/powerpc/kernel/time.c              |  2 --
 drivers/misc/lkdtm/Makefile             |  2 +-
 drivers/misc/lkdtm/powerpc.c            | 44 +++++++++++++++++++++++++
 tools/testing/selftests/lkdtm/tests.txt |  1 +
 4 files changed, 46 insertions(+), 3 deletions(-)
Signature · 1 line
2.52.0
#1 lore raw

PATCH 1/3 powerpc/time: remove preempt_disable/enable from arch_irq_work_raise()

A kernel panic is observed when handling machine check exceptions from real mode.

  BUG: Unable to handle kernel data access on read at 0xc00000006be21300
  Oops: Kernel access of bad area, sig: 11 [#1]
  NIP [c000000000029e40] arch_irq_work_raise+0x10/0x70
  LR [c00000000003ffc8] machine_check_queue_event+0xa8/0x150
  Call Trace:
  [c0000000179d3c70] [c00000000003ff64] machine_check_queue_event+0x44/0x150
  [c0000000179d3d30] [c0000000000084e0] machine_check_early_common+0x1f0/0x2c0

The crash occurs because arch_irq_work_raise() calls preempt_disable() from machine check exception (MCE) handlers running in real mode. In this context, accessing the preempt_count can fault, leading to the panic.

The preempt_disable()/preempt_enable() pair in arch_irq_work_raise() was originally added by commit 0fe1ac48bef0 ("powerpc/perf_event: Fix oops due to perf_event_do_pending call") to avoid races while raising irq work from exception context.

Later, commit 471ba0e686cb ("irq_work: Do not raise an IPI when queueing work on the local CPU") added preemption protection in irq_work_queue() path, while commit 20b876918c06 ("irq_work: Use per cpu atomics instead of regular atomics") added equivalent protection in irq_work_queue_on() before reaching arch_irq_work_raise():

irq_work_queue() / irq_work_queue_on()

    -> preempt_disable()
      -> __irq_work_queue_local()
        -> irq_work_raise()
          -> arch_irq_work_raise()

As a result, callers other than mce_irq_work_raise() already execute with preemption disabled, making the additional preempt_disable()/preempt_enable() pair in arch_irq_work_raise() redundant.

Remove it to avoid accessing preempt_count from real mode context.

Fixes cc15ff327569 ("powerpc/mce: Avoid using irq_work_queue() in realmode")
---
 arch/powerpc/kernel/time.c | 2 --
 1 file changed, 2 deletions(-)
arch/powerpc/kernel/time.c +0 -2
index 4bbeb8644d3d..a99eb43f6ce9 100644
@@ -471,10 +471,8 @@ void arch_irq_work_raise(void)
 	 * which could get tangled up if we're messing with the same state
 	 * here.
 	 */
-	preempt_disable();
 	set_irq_work_pending_flag();
 	set_dec(1);
-	preempt_enable();
 }
 
 static void set_dec_or_work(u64 val)
Signature · 1 line
2.52.0
reviewer Acked-by on v1 1/3
#2 lore raw

Re: [PATCH 1/3] powerpc/time: remove preempt_disable/enable from arch_irq_work_raise()

Quoted 32 lines · Sayali Patil

A kernel panic is observed when handling machine check exceptions from real mode.

   BUG: Unable to handle kernel data access on read at 0xc00000006be21300
   Oops: Kernel access of bad area, sig: 11 [#1]
   NIP [c000000000029e40] arch_irq_work_raise+0x10/0x70
   LR [c00000000003ffc8] machine_check_queue_event+0xa8/0x150
   Call Trace:
   [c0000000179d3c70] [c00000000003ff64] machine_check_queue_event+0x44/0x150
   [c0000000179d3d30] [c0000000000084e0] machine_check_early_common+0x1f0/0x2c0

The crash occurs because arch_irq_work_raise() calls preempt_disable() from machine check exception (MCE) handlers running in real mode. In this context, accessing the preempt_count can fault, leading to the panic.

The preempt_disable()/preempt_enable() pair in arch_irq_work_raise() was originally added by commit 0fe1ac48bef0 ("powerpc/perf_event: Fix oops due to perf_event_do_pending call") to avoid races while raising irq work from exception context.

Later, commit 471ba0e686cb ("irq_work: Do not raise an IPI when queueing work on the local CPU") added preemption protection in irq_work_queue() path, while commit 20b876918c06 ("irq_work: Use per cpu atomics instead of regular atomics") added equivalent protection in irq_work_queue_on() before reaching arch_irq_work_raise():

   irq_work_queue() / irq_work_queue_on()
     -> preempt_disable()
       -> __irq_work_queue_local()
         -> irq_work_raise()
           -> arch_irq_work_raise()
Quoted 6 lines

As a result, callers other than mce_irq_work_raise() already execute with preemption disabled, making the additional preempt_disable()/preempt_enable() pair in arch_irq_work_raise() redundant.

Remove it to avoid accessing preempt_count from real mode context.

I assume interrupt is disabled here. So it should be functionally safe to remove it.

arch/powerpc/kernel/time.c void arch_irq_work_raise(void)

Quoted 13 lines
Fixes cc15ff327569 ("powerpc/mce: Avoid using irq_work_queue() in realmode")
---
  arch/powerpc/kernel/time.c | 2 --
  1 file changed, 2 deletions(-)
arch/powerpc/kernel/time.c +0 -0
index 4bbeb8644d3d..a99eb43f6ce9 100644
@@ -471,10 +471,8 @@ void arch_irq_work_raise(void)

Could you please add a comment for the function that it expects to be called with preemption_disabled?

arch/powerpc/kernel/time.c void arch_irq_work_raise(void) · line 478

Quoted 10 lines
arch/powerpc/kernel/time.c +0 -2
  	 * which could get tangled up if we're messing with the same state
  	 * here.
  	 */
-	preempt_disable();
  	set_irq_work_pending_flag();
  	set_dec(1);
-	preempt_enable();
  }
 
  static void set_dec_or_work(u64 val)
reviewer Reviewed-by on v1 1/3
#3 lore raw

Re: [PATCH 1/3] powerpc/time: remove preempt_disable/enable from arch_irq_work_raise()

In reply to Sayali Patil
Quoted 7 lines · Sayali Patil

A kernel panic is observed when handling machine check exceptions from real mode.

  BUG: Unable to handle kernel data access on read at 0xc00000006be21300
  Oops: Kernel access of bad area, sig: 11 [#1]
  NIP [c000000000029e40] arch_irq_work_raise+0x10/0x70
  LR [c00000000003ffc8] machine_check_queue_event+0xa8/0x150
[14626.841925] MSR:  8000000000001003 <SF,ME,RI,LE>  CR: 88222248  XER: 00000005
[14626.841939] CFAR: c00000000003ffc4 DAR: c00000006be21300 DSISR: 40000000 IRQMASK: 0

Let's also add the above MSR state along with the call stack showing MSR[EE] was 0 when this triggered. This also shows the DAR as 0xc.... while MSR[IR|DR] = 0.

Quoted 25 lines
  Call Trace:
  [c0000000179d3c70] [c00000000003ff64] machine_check_queue_event+0x44/0x150
  [c0000000179d3d30] [c0000000000084e0] machine_check_early_common+0x1f0/0x2c0

The crash occurs because arch_irq_work_raise() calls preempt_disable() from machine check exception (MCE) handlers running in real mode. In this context, accessing the preempt_count can fault, leading to the panic.

The preempt_disable()/preempt_enable() pair in arch_irq_work_raise() was originally added by commit 0fe1ac48bef0 ("powerpc/perf_event: Fix oops due to perf_event_do_pending call") to avoid races while raising irq work from exception context.

Later, commit 471ba0e686cb ("irq_work: Do not raise an IPI when queueing work on the local CPU") added preemption protection in irq_work_queue() path, while commit 20b876918c06 ("irq_work: Use per cpu atomics instead of regular atomics") added equivalent protection in irq_work_queue_on() before reaching arch_irq_work_raise():

irq_work_queue() / irq_work_queue_on()

    -> preempt_disable()
      -> __irq_work_queue_local()
        -> irq_work_raise()
          -> arch_irq_work_raise()
Quoted 8 lines

As a result, callers other than mce_irq_work_raise() already execute with preemption disabled, making the additional preempt_disable()/preempt_enable() pair in arch_irq_work_raise() redundant.

Remove it to avoid accessing preempt_count from real mode context.

Fixes cc15ff327569 ("powerpc/mce: Avoid using irq_work_queue() in realmode")

Agree with the Fixes tag. This patch actually moved mce to use arch_irq_work_raise(). It was ok until the CONFIG_PREEMPTION was disabled on powerpc since macros like preempt_enable|disable() were mostly a no-op. However, after lazy preemption got enabled, access to preempt_count while in real mode can cause the issue you described.

One more thing which we should add to the commit msg is:
The arch_irq_work_raise() function executes in NMI context when called from MCE handler, hence we won't be preempted or scheduled out since we are in NMI context with MSR[EE]=0, hence it is safe to remove preempt_disable|enable() call from here.

And let's change the commit subject to:
powerpc/time: Remove redundant preempt_disable|enable() calls from arch_irq_work_raise()

BTW, thanks for adding a nice commit msg with the sequence of events. With the above changes - pease feel free to add:

Quoted 23 lines
---
 arch/powerpc/kernel/time.c | 2 --
 1 file changed, 2 deletions(-)
arch/powerpc/kernel/time.c +0 -2
index 4bbeb8644d3d..a99eb43f6ce9 100644
@@ -471,10 +471,8 @@ void arch_irq_work_raise(void)
 	 * which could get tangled up if we're messing with the same state
 	 * here.
 	 */
-	preempt_disable();
 	set_irq_work_pending_flag();
 	set_dec(1);
-	preempt_enable();
 }
 
 static void set_dec_or_work(u64 val)
Signature · 1 line
2.52.0
#4 lore raw

Re: [PATCH 1/3] powerpc/time: remove preempt_disable/enable from arch_irq_work_raise()

Quoted 48 lines · Ritesh Harjani (IBM)
Quoted 7 lines · Sayali Patil

A kernel panic is observed when handling machine check exceptions from real mode.

   BUG: Unable to handle kernel data access on read at 0xc00000006be21300
   Oops: Kernel access of bad area, sig: 11 [#1]
   NIP [c000000000029e40] arch_irq_work_raise+0x10/0x70
   LR [c00000000003ffc8] machine_check_queue_event+0xa8/0x150
[14626.841925] MSR:  8000000000001003 <SF,ME,RI,LE>  CR: 88222248  XER: 00000005
[14626.841939] CFAR: c00000000003ffc4 DAR: c00000006be21300 DSISR: 40000000 IRQMASK: 0

Let's also add the above MSR state along with the call stack showing MSR[EE] was 0 when this triggered. This also shows the DAR as 0xc.... while MSR[IR|DR] = 0.

Quoted 30 lines
   Call Trace:
   [c0000000179d3c70] [c00000000003ff64] machine_check_queue_event+0x44/0x150
   [c0000000179d3d30] [c0000000000084e0] machine_check_early_common+0x1f0/0x2c0

The crash occurs because arch_irq_work_raise() calls preempt_disable() from machine check exception (MCE) handlers running in real mode. In this context, accessing the preempt_count can fault, leading to the panic.

The preempt_disable()/preempt_enable() pair in arch_irq_work_raise() was originally added by commit 0fe1ac48bef0 ("powerpc/perf_event: Fix oops due to perf_event_do_pending call") to avoid races while raising irq work from exception context.

Later, commit 471ba0e686cb ("irq_work: Do not raise an IPI when queueing work on the local CPU") added preemption protection in irq_work_queue() path, while commit 20b876918c06 ("irq_work: Use per cpu atomics instead of regular atomics") added equivalent protection in irq_work_queue_on() before reaching arch_irq_work_raise():

   irq_work_queue() / irq_work_queue_on()
     -> preempt_disable()
       -> __irq_work_queue_local()
         -> irq_work_raise()
           -> arch_irq_work_raise()

As a result, callers other than mce_irq_work_raise() already execute with preemption disabled, making the additional preempt_disable()/preempt_enable() pair in arch_irq_work_raise() redundant.

Quoted 8 lines
Quoted 3 lines

Remove it to avoid accessing preempt_count from real mode context.

Fixes cc15ff327569 ("powerpc/mce: Avoid using irq_work_queue() in realmode")

Agree with the Fixes tag. This patch actually moved mce to use arch_irq_work_raise(). It was ok until the CONFIG_PREEMPTION was disabled on powerpc since macros like preempt_enable|disable() were mostly a no-op. However, after lazy preemption got enabled, access to

Both full/lazy preemption. With upstream now, one can choose full or lazy only. Leading to issue being discovered.

Quoted 42 lines

preempt_count while in real mode can cause the issue you described.

One more thing which we should add to the commit msg is:
The arch_irq_work_raise() function executes in NMI context when called from MCE handler, hence we won't be preempted or scheduled out since we are in NMI context with MSR[EE]=0, hence it is safe to remove preempt_disable|enable() call from here.

And let's change the commit subject to:
powerpc/time: Remove redundant preempt_disable|enable() calls from arch_irq_work_raise()

BTW, thanks for adding a nice commit msg with the sequence of events. With the above changes - pease feel free to add:

Quoted 23 lines
---
  arch/powerpc/kernel/time.c | 2 --
  1 file changed, 2 deletions(-)
arch/powerpc/kernel/time.c +0 -2
index 4bbeb8644d3d..a99eb43f6ce9 100644
@@ -471,10 +471,8 @@ void arch_irq_work_raise(void)
  	 * which could get tangled up if we're messing with the same state
  	 * here.
  	 */
-	preempt_disable();
  	set_irq_work_pending_flag();
  	set_dec(1);
-	preempt_enable();
  }
 
  static void set_dec_or_work(u64 val)
Signature · 1 line
2.52.0
#5 lore raw

PATCH 2/3 lkdtm/powerpc: add isync after slbmte to enforce SLB update ordering

In reply to Sayali Patil

The slbmte instruction modifies the Segment Lookaside Buffer, but without a context synchronizing operation the CPU is not guaranteed to observe the updated SLB state for subsequent instructions. This can result in use of stale translation state when memory is accessed immediately after SLB modifications.

Add isync after each slbmte in the PPC_SLB_MULTIHIT test to ensure proper ordering of SLB updates before subsequent memory accesses.

This aligns with Power ISA context synchronization requirements for changes in address translation state and improves the reliability of SLB multihit injection tests in hash MMU mode.

---
 drivers/misc/lkdtm/powerpc.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/misc/lkdtm/powerpc.c +5 -0
index be385449911a..ef07e5201edf 100644
@@ -17,11 +17,14 @@ static void insert_slb_entry(unsigned long p, int ssize, int page_size)
 		     : "r" (mk_vsid_data(p, ssize, flags)),
 		       "r" (mk_esid_data(p, ssize, SLB_NUM_BOLTED))
 		     : "memory");
+	isync();
 
 	asm volatile("slbmte %0,%1" :
 			: "r" (mk_vsid_data(p, ssize, flags)),
 			  "r" (mk_esid_data(p, ssize, SLB_NUM_BOLTED + 1))
 			: "memory");
+	isync();
+
 	preempt_enable();
 }
 
@@ -84,6 +87,7 @@ static void insert_dup_slb_entry_0(void)
 			: "r" (vsid),
 			  "r" (esid | SLB_NUM_BOLTED)
 			: "memory");
+	isync();
 
 	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
 	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
@@ -93,6 +97,7 @@ static void insert_dup_slb_entry_0(void)
 			: "r" (vsid),
 			  "r" (esid | (SLB_NUM_BOLTED + 1))
 			: "memory");
+	isync();
 
 	pr_info("%s accessing test address 0x%lx: 0x%lx\n",
 		__func__, test_address, *test_ptr);
Signature · 1 line
2.52.0
#6 lore raw

PATCH 3/3 lkdtm/powerpc: add PPC_RADIX_TLBIEL test for radix MCE validation

In reply to Sayali Patil

Add a new LKDTM trigger (PPC_RADIX_TLBIEL) that executes a process-scoped radix TLBIEL instruction to exercise the radix MMU behaviour and associated machine check exception (MCE) handling paths.

This provides a way to validate MCE handling in radix mode. Currently, there is no dedicated LKDTM test that exercises this path or allows triggering radix-specific machine check behaviour for validation.

The test is only enabled on ppc64 systems with radix MMU support and If radix is not active, the trigger is skipped and reported as XFAIL.

---
 drivers/misc/lkdtm/Makefile             |  2 +-
 drivers/misc/lkdtm/powerpc.c            | 39 +++++++++++++++++++++++++
 tools/testing/selftests/lkdtm/tests.txt |  1 +
 3 files changed, 41 insertions(+), 1 deletion(-)
drivers/misc/lkdtm/Makefile +1 -1
index 03ebe33185f9..4e58d16fc01e 100644
@@ -11,7 +11,7 @@ lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
 lkdtm-$(CONFIG_LKDTM)		+= kstack_erase.o
 lkdtm-$(CONFIG_LKDTM)		+= cfi.o
 lkdtm-$(CONFIG_LKDTM)		+= fortify.o
-lkdtm-$(CONFIG_PPC_64S_HASH_MMU)	+= powerpc.o
+lkdtm-$(CONFIG_PPC_BOOK3S_64)	+= powerpc.o
 
 KASAN_SANITIZE_stackleak.o	:= n
 
drivers/misc/lkdtm/powerpc.c +39 -0
index ef07e5201edf..3d56b56f5e0c 100644
@@ -105,6 +105,31 @@ static void insert_dup_slb_entry_0(void)
 	preempt_enable();
 }
 
+static void tlbiel_va(unsigned long va,
+		      unsigned long pid,
+		      unsigned long ap,
+		      unsigned long ric)
+{
+	unsigned long rb, rs, prs, r;
+
+	rb = va & ~(PPC_BITMASK(52, 63));
+	rb |= ap << PPC_BITLSHIFT(58);
+	rs = pid << PPC_BITLSHIFT(31);
+
+	prs = 1; /* process scoped */
+	r = 1;   /* radix format */
+
+	/*
+	 * Trigger an MCE by issuing radix tlbiel with an invalid operand combination.
+	 * Using PRS=1 (process-scoped) with kernel address does not correspond to
+	 * any valid process-scoped translation.
+	 * This results in an invalid tlbiel operation, causing hardware to
+	 * raise a machine check.
+	 */
+	asm volatile(PPC_TLBIEL(%0, %4, %3, %2, %1)
+			: : "r"(rb), "i"(r), "i"(prs), "i"(ric), "r"(rs) : "memory");
+}
+
 static void lkdtm_PPC_SLB_MULTIHIT(void)
 {
 	if (!radix_enabled()) {
@@ -124,8 +149,22 @@ static void lkdtm_PPC_SLB_MULTIHIT(void)
 	}
 }
 
+static void lkdtm_PPC_RADIX_TLBIEL(void)
+{
+	unsigned long addr = PAGE_OFFSET;
+
+	if (radix_enabled()) {
+		pr_info("Injecting Radix TLB invalidation MCE\n");
+		tlbiel_va(addr, 0, 0, RIC_FLUSH_ALL);
+		pr_info("Recovered from radix tlbiel attempt\n");
+	} else {
+		pr_err("XFAIL: This test is for ppc64 and with radix mode MMU only\n");
+	}
+}
+
 static struct crashtype crashtypes[] = {
 	CRASHTYPE(PPC_SLB_MULTIHIT),
+	CRASHTYPE(PPC_RADIX_TLBIEL),
 };
 
 struct crashtype_category powerpc_crashtypes = {
tools/testing/selftests/lkdtm/tests.txt +1 -0
index 3245032db34d..d8180bbe31e8 100644
@@ -86,3 +86,4 @@ FORTIFY_STR_MEMBER detected buffer overflow
 FORTIFY_MEM_OBJECT detected buffer overflow
 FORTIFY_MEM_MEMBER detected field-spanning write
 PPC_SLB_MULTIHIT Recovered
+#PPC_RADIX_TLBIEL Triggers unrecoverable MCE
Signature · 1 line
2.52.0
#7 lore raw
In reply to Sayali Patil
Quoted 19 lines · Sayali Patil

Hi all,

This series fixes a panic observed while handling machine check exceptions (MCEs) from real mode. It also improves the reliability of the PPC_SLB_MULTIHIT test by adding isync instructions after slbmte operations, and introduces a new LKDTM trigger (PPC_RADIX_TLBIEL) to validate MCE behavior on radix MMU.

Please review the patches and provide any feedback or suggestions for improvement.

Thanks,
Sayali

Sayali Patil (3):
powerpc/time: remove preempt_disable/enable from arch_irq_work_raise() lkdtm/powerpc: add isync after slbmte to enforce SLB update ordering lkdtm/powerpc: add PPC_RADIX_TLBIEL test for radix MCE validation

Quoted 6 lines
  arch/powerpc/kernel/time.c              |  2 --
  drivers/misc/lkdtm/Makefile             |  2 +-
  drivers/misc/lkdtm/powerpc.c            | 44 +++++++++++++++++++++++++
  tools/testing/selftests/lkdtm/tests.txt |  1 +
  4 files changed, 46 insertions(+), 3 deletions(-)

Sashiko has provided some comments (https://sashiko.dev/#/patchset/cover.1778057685.git.sayalip%40linux.ibm.com), so I am planning to split this into two patch series. I will send v2 part 1 today with the proposed change for "powerpc/time: Remove preempt_disable/enable from arch_irq_work_raise()", and v2 part 2 with the remaining two patches later.

Thanks for the review, Ritesh and Shrikanth.

Regards,
Sayali