| Message ID | 20250128001338.11474-1-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] target/s390x: Fix MVC not always invalidating translation blocks | expand |
On 1/27/25 16:12, Ilya Leoshkevich wrote: > Node.js crashes in qemu-system-s390x with random SIGSEGVs / SIGILLs. > > The v8 JIT used by Node.js can garbage collect and overwrite unused > code. Overwriting is performed by WritableJitAllocation::CopyCode(), > which ultimately calls memcpy(). For certain sizes, memcpy() uses the > MVC instruction. > > QEMU implements MVC and other similar instructions using helpers. While > TCG store ops invalidate affected translation blocks automatically, > helpers must do this manually by calling probe_access_flags(). The MVC > helper does this using the access_prepare() -> access_prepare_nf() -> > s390_probe_access() -> probe_access_flags() call chain. > > At the last step of this chain, the store size is replaced with 0. This > causes the probe_access_flags() -> notdirty_write() -> > tb_invalidate_phys_range_fast() chain to miss some translation blocks. > > When this happens, QEMU executes a mix of old and new code. This > quickly leads to either a SIGSEGV or a SIGILL in case the old code > ends in the middle of a new instruction. > > Fix by passing the true size. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~ > > Reported-by: Berthold Gunreben <azouhr@opensuse.org> > Cc: Sarah Kriesch <ada.lovelace@gmx.de> > Cc: qemu-stable@nongnu.org > Closes: https://bugzilla.opensuse.org/show_bug.cgi?id=1235709 > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > target/s390x/tcg/mem_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c > index 32717acb7d1..c6ab2901e5a 100644 > --- a/target/s390x/tcg/mem_helper.c > +++ b/target/s390x/tcg/mem_helper.c > @@ -149,7 +149,7 @@ static inline int s390_probe_access(CPUArchState *env, target_ulong addr, > int mmu_idx, bool nonfault, > void **phost, uintptr_t ra) > { > - int flags = probe_access_flags(env, addr, 0, access_type, mmu_idx, > + int flags = probe_access_flags(env, addr, size, access_type, mmu_idx, > nonfault, phost, ra); > > if (unlikely(flags & TLB_INVALID_MASK)) {
On 28.01.25 01:12, Ilya Leoshkevich wrote: > Node.js crashes in qemu-system-s390x with random SIGSEGVs / SIGILLs. > > The v8 JIT used by Node.js can garbage collect and overwrite unused > code. Overwriting is performed by WritableJitAllocation::CopyCode(), > which ultimately calls memcpy(). For certain sizes, memcpy() uses the > MVC instruction. > > QEMU implements MVC and other similar instructions using helpers. While > TCG store ops invalidate affected translation blocks automatically, > helpers must do this manually by calling probe_access_flags(). The MVC > helper does this using the access_prepare() -> access_prepare_nf() -> > s390_probe_access() -> probe_access_flags() call chain. > > At the last step of this chain, the store size is replaced with 0. This > causes the probe_access_flags() -> notdirty_write() -> > tb_invalidate_phys_range_fast() chain to miss some translation blocks. We added the size parameter in: commit 1770b2f2d3d6fe8f1e2d61692692264cac44340d Author: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Date: Thu Feb 23 20:44:24 2023 -0300 accel/tcg: Add 'size' param to probe_access_flags() probe_access_flags() as it is today uses probe_access_full(), which in turn uses probe_access_internal() with size = 0. probe_access_internal() then uses the size to call the tlb_fill() callback for the given CPU. This size param ('fault_size' as probe_access_internal() calls it) is ignored by most existing .tlb_fill callback implementations, e.g. arm_cpu_tlb_fill(), ppc_cpu_tlb_fill(), x86_cpu_tlb_fill() and mips_cpu_tlb_fill() to name a few. And added support for more than one byte for the notdirty case in commit e2faabee78ff127848f59892747d4c07c56de033 Author: Jessica Clarke <jrtc27@jrtc27.com> Date: Fri Nov 10 21:43:03 2023 -0800 accel/tcg: Forward probe size on to notdirty_write Without this, we just dirty a single byte, and so if the caller writes more than one byte to the host memory then we won't have invalidated any translation blocks that start after the first byte and overlap those writes. So I guess it's rather hard to find a "Fixes:" tag, because likely it's been broken forever? > > When this happens, QEMU executes a mix of old and new code. This > quickly leads to either a SIGSEGV or a SIGILL in case the old code > ends in the middle of a new instruction. > > Fix by passing the true size. Reviewed-by: David Hildenbrand <david@redhat.com>
On Tue, 2025-01-28 at 10:56 +0100, David Hildenbrand wrote: > On 28.01.25 01:12, Ilya Leoshkevich wrote: > > Node.js crashes in qemu-system-s390x with random SIGSEGVs / > > SIGILLs. > > > > The v8 JIT used by Node.js can garbage collect and overwrite unused > > code. Overwriting is performed by > > WritableJitAllocation::CopyCode(), > > which ultimately calls memcpy(). For certain sizes, memcpy() uses > > the > > MVC instruction. > > > > QEMU implements MVC and other similar instructions using helpers. > > While > > TCG store ops invalidate affected translation blocks automatically, > > helpers must do this manually by calling probe_access_flags(). The > > MVC > > helper does this using the access_prepare() -> access_prepare_nf() > > -> > > s390_probe_access() -> probe_access_flags() call chain. > > > > At the last step of this chain, the store size is replaced with 0. > > This > > causes the probe_access_flags() -> notdirty_write() -> > > tb_invalidate_phys_range_fast() chain to miss some translation > > blocks. > > We added the size parameter in: > > commit 1770b2f2d3d6fe8f1e2d61692692264cac44340d > Author: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > Date: Thu Feb 23 20:44:24 2023 -0300 > > accel/tcg: Add 'size' param to probe_access_flags() > > probe_access_flags() as it is today uses probe_access_full(), > which in > turn uses probe_access_internal() with size = 0. > probe_access_internal() > then uses the size to call the tlb_fill() callback for the given > CPU. > This size param ('fault_size' as probe_access_internal() calls > it) is > ignored by most existing .tlb_fill callback implementations, > e.g. > arm_cpu_tlb_fill(), ppc_cpu_tlb_fill(), x86_cpu_tlb_fill() and > mips_cpu_tlb_fill() to name a few. > > > And added support for more than one byte for the notdirty case in > > commit e2faabee78ff127848f59892747d4c07c56de033 > Author: Jessica Clarke <jrtc27@jrtc27.com> > Date: Fri Nov 10 21:43:03 2023 -0800 > > accel/tcg: Forward probe size on to notdirty_write > > Without this, we just dirty a single byte, and so if the caller > writes > more than one byte to the host memory then we won't have > invalidated any > translation blocks that start after the first byte and overlap > those > writes. > > So I guess it's rather hard to find a "Fixes:" tag, because likely > it's been > broken forever? Yes, I thought about this too and gave up. However, I now wonder if we still need one for non-philosophical, but rather practical backporting reasons? Then Fixes: e2faabee78ff ("accel/tcg: Forward probe size on to notdirty_write") (v8.2.0+) should be the right one I think? > > When this happens, QEMU executes a mix of old and new code. This > > quickly leads to either a SIGSEGV or a SIGILL in case the old code > > ends in the middle of a new instruction. > > > > Fix by passing the true size. > > > Reviewed-by: David Hildenbrand <david@redhat.com>
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c index 32717acb7d1..c6ab2901e5a 100644 --- a/target/s390x/tcg/mem_helper.c +++ b/target/s390x/tcg/mem_helper.c @@ -149,7 +149,7 @@ static inline int s390_probe_access(CPUArchState *env, target_ulong addr, int mmu_idx, bool nonfault, void **phost, uintptr_t ra) { - int flags = probe_access_flags(env, addr, 0, access_type, mmu_idx, + int flags = probe_access_flags(env, addr, size, access_type, mmu_idx, nonfault, phost, ra); if (unlikely(flags & TLB_INVALID_MASK)) {
Node.js crashes in qemu-system-s390x with random SIGSEGVs / SIGILLs. The v8 JIT used by Node.js can garbage collect and overwrite unused code. Overwriting is performed by WritableJitAllocation::CopyCode(), which ultimately calls memcpy(). For certain sizes, memcpy() uses the MVC instruction. QEMU implements MVC and other similar instructions using helpers. While TCG store ops invalidate affected translation blocks automatically, helpers must do this manually by calling probe_access_flags(). The MVC helper does this using the access_prepare() -> access_prepare_nf() -> s390_probe_access() -> probe_access_flags() call chain. At the last step of this chain, the store size is replaced with 0. This causes the probe_access_flags() -> notdirty_write() -> tb_invalidate_phys_range_fast() chain to miss some translation blocks. When this happens, QEMU executes a mix of old and new code. This quickly leads to either a SIGSEGV or a SIGILL in case the old code ends in the middle of a new instruction. Fix by passing the true size. Reported-by: Berthold Gunreben <azouhr@opensuse.org> Cc: Sarah Kriesch <ada.lovelace@gmx.de> Cc: qemu-stable@nongnu.org Closes: https://bugzilla.opensuse.org/show_bug.cgi?id=1235709 Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- target/s390x/tcg/mem_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)