Re: [PATCH 1/2] target/s390x: Fix MVC not always invalidating translation blocks

[David Hildenbrand]

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>

--
Cheers,

David / dhildenb