[BUG] powerpc/code-patching: IPI with interrupts disabled on e500 SMP

[BUG] powerpc/code-patching: IPI with interrupts disabled on e500 SMP

[Josef Schlehofer]

Hi,

On a P2020 (e500v2, two CPUs) running 7.3-rc1 with
CONFIG_PROVE_LOCKING and CONFIG_STRICT_KERNEL_RWX, the BPF JIT
triggers this warning at boot:

  WARNING: kernel/smp.c:917 at smp_call_function_many_cond+0x430/0x818, CPU#1: swapper/0/1
  CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 7.3.0-rc1-leds-v4 #1 PREEMPT
  Hardware name: Turris 1.1 e500v2 0x80211051 Freescale P2020
  Call Trace:
  [c1915bc0] [c001be04] flush_tlb_kernel_range+0x2c/0x74
  [c1915bd0] [c001ce2c] patch_instructions+0x1b0/0x318
  [c1915c20] [c003113c] bpf_arch_text_copy+0x5c/0xb4
  [c1915c40] [c01c3398] bpf_jit_binary_pack_finalize+0x28/0x8c
  [c1915c60] [c0031770] bpf_int_jit_compile+0x5dc/0x758
  ...
  [c1915dd0] [c102e114] ptp_classifier_init+0x30/0x4c
  [c1915df0] [c102d3a8] sock_init+0xc8/0xe4
  ...
  hardirqs last disabled at (3076): [<c001ceb4>] patch_instructions+0x238/0x318

patch_mem() and patch_instructions() disable interrupts and then call
flush_tlb_kernel_range(). On nohash SMP that is
smp_call_function(do_flush_tlb_mm_ipi, NULL, 1), which waits for the
other CPU with interrupts off. If the other CPU is doing the same, both
wait for each other.

I think this is why the CZ.NIC Turris 1.1 hangs early in boot with
CONFIG_JUMP_LABEL=y. On 6.18.44 it hung in 19 of 19 boots and booted
fine 9 of 9 times with jump labels off. The hang comes while static
keys are patched and modules are loaded, nothing is printed and the
watchdog resets the board. OpenWrt keeps jump labels off on these
boards for now.

The same problem in change_page_attr() was fixed in 2017 by commit
7c6a4f3b1641 ("powerpc/mm: Call flush_tlb_kernel_range with interrupts
enabled").

I can test patches on the board.

Josef

Re: [BUG] powerpc/code-patching: IPI with interrupts disabled on e500 SMP

[Ritesh Harjani]

Hi Josef,

Josef Schlehofer <pepe.schlehofer@gmail.com> writes:

> Hi,
>
> On a P2020 (e500v2, two CPUs) running 7.3-rc1 with
> CONFIG_PROVE_LOCKING and CONFIG_STRICT_KERNEL_RWX, the BPF JIT
> triggers this warning at boot:
>
>   WARNING: kernel/smp.c:917 at smp_call_function_many_cond+0x430/0x818, CPU#1: swapper/0/1
>   CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 7.3.0-rc1-leds-v4 #1 PREEMPT
>   Hardware name: Turris 1.1 e500v2 0x80211051 Freescale P2020
>   Call Trace:
>   [c1915bc0] [c001be04] flush_tlb_kernel_range+0x2c/0x74
>   [c1915bd0] [c001ce2c] patch_instructions+0x1b0/0x318
>   [c1915c20] [c003113c] bpf_arch_text_copy+0x5c/0xb4
>   [c1915c40] [c01c3398] bpf_jit_binary_pack_finalize+0x28/0x8c
>   [c1915c60] [c0031770] bpf_int_jit_compile+0x5dc/0x758
>   ...
>   [c1915dd0] [c102e114] ptp_classifier_init+0x30/0x4c
>   [c1915df0] [c102d3a8] sock_init+0xc8/0xe4
>   ...
>   hardirqs last disabled at (3076): [<c001ceb4>] patch_instructions+0x238/0x318
>
> patch_mem() and patch_instructions() disable interrupts and then call
> flush_tlb_kernel_range(). On nohash SMP that is
> smp_call_function(do_flush_tlb_mm_ipi, NULL, 1), which waits for the
> other CPU with interrupts off. If the other CPU is doing the same, both
> wait for each other.
>
> I think this is why the CZ.NIC Turris 1.1 hangs early in boot with
> CONFIG_JUMP_LABEL=y. On 6.18.44 it hung in 19 of 19 boots and booted
> fine 9 of 9 times with jump labels off. The hang comes while static
> keys are patched and modules are loaded, nothing is printed and the
> watchdog resets the board. OpenWrt keeps jump labels off on these
> boards for now.

Thanks for the report and the analysis.

Looks like the problem existed from quite some time, but it must have
got exposed after you enabled the above configs. I guess some of these
configs are not default on - hence this might not have been caught so
far.

>
> The same problem in change_page_attr() was fixed in 2017 by commit
> 7c6a4f3b1641 ("powerpc/mm: Call flush_tlb_kernel_range with interrupts
> enabled").
>

I am not sure whether flushing the tlb after enabling interrupts is the
right fix here. Because once interrupts are restored, an interrupt or
preemption can run another patch instruction on the same cpu, where it
could use the previous stale TLB, which isn't flushed yet. Note that the
text_poke_area is per-cpu and the same address is reused by every patch
on that cpu.

So, maybe we can do local tlb flush since the text_poke_area is anyway
per-cpu and interrupts are also kept disabled for patching.

> I can test patches on the board.
>

Thanks! that will help. Can you give this diff a try please?


diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 44ff9f684bef..6351342fa5c3 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -276,6 +276,23 @@ static int __do_patch_mem_mm(void *addr, unsigned long val, bool is_dword)
        return err;
 }

+/*
+ * The patch area is per-CPU and only used by its own CPU, with IRQs
+ * disabled, so only the local TLB can hold an entry for it.
+ *
+ * On nohash SMP, flush_tlb_kernel_range() IPIs the other CPUs and waits, which
+ * must not be done with IRQs disabled (see smp_call_function_many_cond()), so
+ * flush just the local kernel (PID 0) entry.
+ * Book3S keeps flush_tlb_kernel_range() which doesn't IPI here.
+ */
+static void flush_patch_area(unsigned long addr)
+{
+       if (IS_ENABLED(CONFIG_PPC_BOOK3S))
+               flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+       else
+               local_flush_tlb_page(NULL, addr);
+}
+
 static int __do_patch_mem(void *addr, unsigned long val, bool is_dword)
 {
        int err;
@@ -296,7 +313,7 @@ static int __do_patch_mem(void *addr, unsigned long val, bool is_dword)
        err = __patch_mem(addr, val, patch_addr, is_dword);

        pte_clear(&init_mm, text_poke_addr, pte);
-       flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
+       flush_patch_area(text_poke_addr);

        return err;
 }
@@ -488,7 +505,7 @@ static int __do_patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat
        err = __patch_instructions(patch_addr, code, len, repeat_instr);

        pte_clear(&init_mm, text_poke_addr, pte);
-       flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
+       flush_patch_area(text_poke_addr);

        return err;
 }


-ritesh