| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87cy9b441j.fsf@gmail.com>
Date: Mon, 04 Aug 2025 18:50:08 +0530
From: Ritesh Harjani (IBM) <ritesh.list@...il.com>
To: Nicholas Piggin <npiggin@...il.com>, Donet Tom <donettom@...ux.ibm.com>, Madhavan Srinivasan <maddy@...ux.ibm.com>, Christophe Leroy <christophe.leroy@...roup.eu>, linuxppc-dev@...ts.ozlabs.org
Cc: linux-kernel@...r.kernel.org, Michael Ellerman <mpe@...erman.id.au>, Vishal Chourasia <vishalc@...ux.ibm.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2] powerpc/mm: Fix SLB multihit issue during SLB preload
"Nicholas Piggin" <npiggin@...il.com> writes:
> Hmm, interesting bug. Impressive work to track it down.
>
Thanks Nick for taking a look at it.
> On Fri Aug 1, 2025 at 8:37 PM AEST, Donet Tom wrote:
>> On systems using the hash MMU, there is a software SLB preload cache that
>> mirrors the entries loaded into the hardware SLB buffer. This preload
>> cache is subject to periodic eviction — typically after every 256 context
>> switches — to remove old entry.
>>
>> To optimize performance, the kernel skips switch_mmu_context() in
>> switch_mm_irqs_off() when the prev and next mm_struct are the same.
>> However, on hash MMU systems, this can lead to inconsistencies between
>> the hardware SLB and the software preload cache.
>>
>> If an SLB entry for a process is evicted from the software cache on one
>> CPU, and the same process later runs on another CPU without executing
>> switch_mmu_context(), the hardware SLB may retain stale entries. If the
>> kernel then attempts to reload that entry, it can trigger an SLB
>> multi-hit error.
>>
>> The following timeline shows how stale SLB entries are created and can
>> cause a multi-hit error when a process moves between CPUs without a
>> MMU context switch.
>>
>> CPU 0 CPU 1
>> ----- -----
>> Process P
>> exec swapper/1
>> load_elf_binary
>> begin_new_exc
>> activate_mm
>> switch_mm_irqs_off
>> switch_mmu_context
>> switch_slb
>> /*
>> * This invalidates all
>> * the entries in the HW
>> * and setup the new HW
>> * SLB entries as per the
>> * preload cache.
>> */
>> context_switch
>> sched_migrate_task migrates process P to cpu-1
>>
>> Process swapper/0 context switch (to process P)
>> (uses mm_struct of Process P) switch_mm_irqs_off()
>> switch_slb
>> load_slb++
>> /*
>> * load_slb becomes 0 here
>> * and we evict an entry from
>> * the preload cache with
>> * preload_age(). We still
>> * keep HW SLB and preload
>> * cache in sync, that is
>> * because all HW SLB entries
>> * anyways gets evicted in
>> * switch_slb during SLBIA.
>> * We then only add those
>> * entries back in HW SLB,
>> * which are currently
>> * present in preload_cache
>> * (after eviction).
>> */
>> load_elf_binary continues...
>> setup_new_exec()
>> slb_setup_new_exec()
>>
>> sched_switch event
>> sched_migrate_task migrates
>> process P to cpu-0
>>
>> context_switch from swapper/0 to Process P
>> switch_mm_irqs_off()
>> /*
>> * Since both prev and next mm struct are same we don't call
>> * switch_mmu_context(). This will cause the HW SLB and SW preload
>> * cache to go out of sync in preload_new_slb_context. Because there
>> * was an SLB entry which was evicted from both HW and preload cache
>> * on cpu-1. Now later in preload_new_slb_context(), when we will try
>> * to add the same preload entry again, we will add this to the SW
>> * preload cache and then will add it to the HW SLB. Since on cpu-0
>> * this entry was never invalidated, hence adding this entry to the HW
>> * SLB will cause a SLB multi-hit error.
>> */
>> load_elf_binary continues...
>> START_THREAD
>> start_thread
>> preload_new_slb_context
>> /*
>> * This tries to add a new EA to preload cache which was earlier
>> * evicted from both cpu-1 HW SLB and preload cache. This caused the
>> * HW SLB of cpu-0 to go out of sync with the SW preload cache. The
>> * reason for this was, that when we context switched back on CPU-0,
>> * we should have ideally called switch_mmu_context() which will
>> * bring the HW SLB entries on CPU-0 in sync with SW preload cache
>> * entries by setting up the mmu context properly. But we didn't do
>> * that since the prev mm_struct running on cpu-0 was same as the
>> * next mm_struct (which is true for swapper / kernel threads). So
>> * now when we try to add this new entry into the HW SLB of cpu-0,
>> * we hit a SLB multi-hit error.
>> */
>
> Okay, so what happens is CPU0 has SLB entries remaining from when
> P last ran on there, and the preload aging happens on CPU1 at a
> time when that CPU does clear its SLB. That slb aging step doesn't
> account for the fact CPU0 SLB entries still exist.
Yes, that is right.
>>
>> WARNING: CPU: 0 PID: 1810970 at arch/powerpc/mm/book3s64/slb.c:62
>> assert_slb_presence+0x2c/0x50(48 results) 02:47:29 [20157/42149]
>> Modules linked in:
>> CPU: 0 UID: 0 PID: 1810970 Comm: dd Not tainted 6.16.0-rc3-dirty #12
>> VOLUNTARY
>> Hardware name: IBM pSeries (emulated by qemu) POWER8 (architected)
>> 0x4d0200 0xf000004 of:SLOF,HEAD hv:linux,kvm pSeries
>> NIP: c00000000015426c LR: c0000000001543b4 CTR: 0000000000000000
>> REGS: c0000000497c77e0 TRAP: 0700 Not tainted (6.16.0-rc3-dirty)
>> MSR: 8000000002823033 <SF,VEC,VSX,FP,ME,IR,DR,RI,LE> CR: 28888482 XER: 00000000
>> CFAR: c0000000001543b0 IRQMASK: 3
>> <...>
>> NIP [c00000000015426c] assert_slb_presence+0x2c/0x50
>> LR [c0000000001543b4] slb_insert_entry+0x124/0x390
>> Call Trace:
>> 0x7fffceb5ffff (unreliable)
>> preload_new_slb_context+0x100/0x1a0
>> start_thread+0x26c/0x420
>> load_elf_binary+0x1b04/0x1c40
>> bprm_execve+0x358/0x680
>> do_execveat_common+0x1f8/0x240
>> sys_execve+0x58/0x70
>> system_call_exception+0x114/0x300
>> system_call_common+0x160/0x2c4
>>
>> To fix this issue, we add a code change to always switch the MMU context on
>> hash MMU if the SLB preload cache has aged. With this change, the
>> SLB multi-hit error no longer occurs.
>>
>> cc: Christophe Leroy <christophe.leroy@...roup.eu>
>> cc: Ritesh Harjani (IBM) <ritesh.list@...il.com>
>> cc: Michael Ellerman <mpe@...erman.id.au>
>> cc: Nicholas Piggin <npiggin@...il.com>
>> Fixes: 5434ae74629a ("powerpc/64s/hash: Add a SLB preload cache")
>> cc: stable@...r.kernel.org
>> Suggested-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
>> Signed-off-by: Donet Tom <donettom@...ux.ibm.com>
>> ---
>>
>> v1 -> v2 : Changed commit message and added a comment in
>> switch_mm_irqs_off()
>>
>> v1 - https://lore.kernel.org/all/20250731161027.966196-1-donettom@linux.ibm.com/
>> ---
>> arch/powerpc/mm/book3s64/slb.c | 2 +-
>> arch/powerpc/mm/mmu_context.c | 7 +++++--
>> 2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/book3s64/slb.c b/arch/powerpc/mm/book3s64/slb.c
>> index 6b783552403c..08daac3f978c 100644
>> --- a/arch/powerpc/mm/book3s64/slb.c
>> +++ b/arch/powerpc/mm/book3s64/slb.c
>> @@ -509,7 +509,7 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
>> * SLB preload cache.
>> */
>> tsk->thread.load_slb++;
>> - if (!tsk->thread.load_slb) {
>> + if (tsk->thread.load_slb == U8_MAX) {
>> unsigned long pc = KSTK_EIP(tsk);
>>
>> preload_age(ti);
>> diff --git a/arch/powerpc/mm/mmu_context.c b/arch/powerpc/mm/mmu_context.c
>> index 3e3af29b4523..95455d787288 100644
>> --- a/arch/powerpc/mm/mmu_context.c
>> +++ b/arch/powerpc/mm/mmu_context.c
>> @@ -83,8 +83,11 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>> /* Some subarchs need to track the PGD elsewhere */
>> switch_mm_pgdir(tsk, next);
>>
>> - /* Nothing else to do if we aren't actually switching */
>> - if (prev == next)
>> + /*
>> + * Nothing else to do if we aren't actually switching and
>> + * the preload slb cache has not aged
>> + */
>> + if ((prev == next) && (tsk->thread.load_slb != U8_MAX))
>> return;
>>
>> /*
>
> I see couple of issues with this fix. First of all, it's a bit wrong to
> call switch subsequent switch_mm functions if prev == next, they are not
> all powerpc specific. We could work around that somehow with some hash
> specific knowledge. But worse I think is that load_slb could be
> incremented again if we context switched P again before migrating back
> here, then we would miss it.
Aah right. Got too involved in the issue, missed to see that coming.
You are right, if the process is context switched twice before coming
back on the original cpu (while still in load_elf_binary() path), we can
still hit the same issue. Though it's probablity of hitting must be very
low, but it is still possible.
>
> How about removing preload_new_slb_context() and slb_setup_new_exec()
> entirely? Then slb preload is a much simpler thing that is only loaded
> after the SLB has been cleared. Those functions were always a bit
> janky and for performance, context switch is the most improtant I think,
> new thread/proc creation less so.
Thanks for the suggestion. Right the above changes should not affect
context switch which is more performance critical.
I agree, removing these two functions should solve the reported problem,
since these two are the only callers where we don't invalidate the HW
SLB before preloading. switch_slb() on the other hand takes care of
that (which gets called during context switch).
I see in the original commit - you had measured context_switch benchmark
which showed 27% performance improvement. Was this context_switch in
will-it-scale?
Is there any workload which you think we could run to confirm that no
regression should be observed with these changes (including for
proc/thread creation?)
>
> Thanks,
> Nick
Thanks again for taking a look. Appreciate your help!
-ritesh
Powered by blists - more mailing lists