Re: [PATCH] powerpc/code-patching: Flush the patch area TLB entry locally

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

"Christophe Leroy (CS GROUP)" <chleroy@kernel.org> writes:

> Le 03/11/2026 à 10:12, Ritesh Harjani a écrit :
>> "Christophe Leroy (CS GROUP)" <chleroy@kernel.org> writes:
>> 
>>> Le 03/11/2026 à 03:54, Ritesh Harjani (IBM) a écrit :
>>>> patching functions like patch_mem() and patch_instructions(), maps the
>>>> target page at the per-cpu text_poke_area, write it, clear the PTE and
>>>> then flushes the tlb using flush_tlb_kernel_range(), with interrupts
>>>> disabled. flush_tlb_kernel_range() on SMP nohash (e500) does an IPI
>>>> based TLB invalidation i.e -
>>>>
>>>> 	smp_call_function(do_flush_tlb_mm_ipi, NULL, 1);
>>>>
>>>> ...which will wait for other CPUs but with interrupts off. This can be
>>>> caught by lockdep with the following warning:
>>>>
>>>>     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
>>>>
>>>> Josef confirmed that this was causing a hang (deadlock):
>>>>       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.
>>>>
>>>> ...which is possible if both cpus are doing similar operations with interrupts
>>>> off and doing TLB invalidation using IPI.
>>>>
>>>> Broadcast TLB invalidation is not needed here since text_poke_area is
>>>> a per-cpu area. It is mapped and unmapped with interrupts disabled by
>>>> it's own cpu. The only reason I think, if we would need a broadcast tlb
>>>> invalidate is, when we want to prevent a stray or malicious write to
>>>> this cpu text_poke_area by another CPU. This generally gets solved with
>>>> temporary-mm path like how Radix and even x86 implements. However, booke
>>>> doesn't implement that, so for now let's ensure we use local tlb
>>>> flushing for correctness.
>>>
>>> This problem was already discussed one year ago, see
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2FPA3P192MB2865C07D634E3B775D003C7F8CD7A%40PA3P192MB2865.EURP192.PROD.OUTLOOK.COM%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C94ba9bffb5d647f9790208df398f6ccb%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C639292960305489629%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=RgVxRJOlFSWgNsFSKwLDRinx3kaF4LpSiCaTey8xtic%3D&reserved=0
>>>
>>> See response from Arnd, a stop_machine() should be done for completeness.
>>>
>> 
>> I will have to take sometime to go over what ARM does here with
>> stop_machine(). However, either ways I think, we still need this fix,
>> since otherwise P2020 boards gets hung during boot. Thoughts?
>
> That's right, it must be fixed. The global TLB flush must likely be 
> changed to a local TLB flush, but that's not enough to get a safe system.
>

Ok then, will spend sometime looking at how other archs handles this.

Although IIUC - even with TLB invalidation using IPI there is a window
where the remote cpu could use a stale tlb. But the window is too small,
however, with only local tlb invalidate, if in case the remote cpu has a
stale TLB, it will never get flushed.

>> 
>> 
>>>>
>>>> Fixes: 49e3d8ea6248 ("powerpc/fsl_booke: Enable STRICT_KERNEL_RWX")
>>>> Cc: stable@vger.kernel.org
>>>> Reported-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
>>>> Closes: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20261026195644.16384-1-pepe.schlehofer%40gmail.com%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C94ba9bffb5d647f9790208df398f6ccb%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C639292960305519321%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=A50lEHBZBaX70jGTu38GylmnQvlxVzLalMVXYj2kti0%3D&reserved=0
>>>> Tested-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
>>>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>>>> ---
>>>>    arch/powerpc/lib/code-patching.c | 21 +++++++++++++++++++--
>>>>    1 file changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> 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))
>>>
>>> CONFIG_PPC_BOOK3S not consistant with commit message. Are you adressing
>>> a problem on e500 only ? on all not book3s ? What about book3s/32 ? What
>>> about 8xx ? Even for book3s/64, is it correct at all when
>>> mm_patch_enabled() returns false ?
>>>
>> 
>> No we are addressing the problem on all NOHASH platforms. IIUC, all
>> !BOOK3S are "nohash". Although some of them maybe UP, so it doesn't
>> matter whether a flush_tlb_kernel_range() is called or
>> local_flush_tlb_page() - it wouldn't matter. And on others which are SMP
>> but do not have STRICT_KERNEL_RWX enabled, they will anyway not go
>> through this path - but if they ever do - then we should ensure they too
>> do a local_flush_tlb_range() to avoid the same deadlock.
>> 
>> However, I think, maybe we could use CONFIG_PPC_MMU_NOHASH instead of using
>> CONFIG_PPC_BOOK3S, thoughts?
>
> Well they are equivalent, so it doesn't solve the issue I think:
>
> config PPC_MMU_NOHASH
> 	def_bool y
> 	depends on !PPC_BOOK3S
>
> What about book3s/64 in hash mode ? What about book3s/32 ? None of them 
> implement temporary_mm, so are they covered ?
>

They both use TLBIE (global/broadcast TLB invalidation). With that we
don't need to use IPI for TLB invalidation.

> Christophe
>

Thanks for the review comments. Let me check on the stop_machine() or
other alternative to fix the security gap as well. At the moment, I have
my hands full with couple of other things, but hopefully I can come back
on this once they are somewhat taken care of.

-ritesh

>> 
>> 
>> -ritesh
>> 
>>>> +		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;
>>>>    }
>>>> --
>>>> 2.39.5
>>>>