Re: [PATCH for-7.2 00/21] accel/tcg: minimize tlb lookups during translate + user-only PROT_EXEC fixes

[Ilya Leoshkevich]

On Fri, 2022-08-12 at 11:07 -0700, Richard Henderson wrote:
> This is part of a larger body of work, but in the process of
> reorganizing I was reminded that PROT_EXEC wasn't being enforced
> properly for user-only.  As this has come up in the context of
> some of Ilya's patches, I thought I'd go ahead and post this part.
> 
> 
> r~
> 
> 
> Ilya Leoshkevich (1):
>   accel/tcg: Introduce is_same_page()
> 
> Richard Henderson (20):
>   linux-user/arm: Mark the commpage executable
>   linux-user/hppa: Allocate page zero as a commpage
>   linux-user/x86_64: Allocate vsyscall page as a commpage
>   linux-user: Honor PT_GNU_STACK
>   tests/tcg/i386: Move smc_code2 to an executable section
>   accel/tcg: Remove PageDesc code_bitmap
>   accel/tcg: Use bool for page_find_alloc
>   accel/tcg: Merge tb_htable_lookup into caller
>   accel/tcg: Move qemu_ram_addr_from_host_nofail to physmem.c
>   accel/tcg: Properly implement get_page_addr_code for user-only
>   accel/tcg: Use probe_access_internal for softmmu
>     get_page_addr_code_hostp
>   accel/tcg: Add nofault parameter to get_page_addr_code_hostp
>   accel/tcg: Unlock mmap_lock after longjmp
>   accel/tcg: Hoist get_page_addr_code out of tb_lookup
>   accel/tcg: Hoist get_page_addr_code out of tb_gen_code
>   accel/tcg: Raise PROT_EXEC exception early
>   accel/tcg: Remove translator_ldsw
>   accel/tcg: Add pc and host_pc params to gen_intermediate_code
>   accel/tcg: Add fast path for translator_ld*
>   accel/tcg: Use DisasContextBase in plugin_gen_tb_start
> 
>  accel/tcg/internal.h          |   7 +-
>  include/elf.h                 |   1 +
>  include/exec/cpu-common.h     |   1 +
>  include/exec/exec-all.h       |  87 +++++-----------
>  include/exec/plugin-gen.h     |   7 +-
>  include/exec/translator.h     |  85 ++++++++++++----
>  linux-user/arm/target_cpu.h   |   4 +-
>  linux-user/qemu.h             |   1 +
>  accel/tcg/cpu-exec.c          | 184 ++++++++++++++++++--------------
> --
>  accel/tcg/cputlb.c            |  93 +++++------------
>  accel/tcg/plugin-gen.c        |  23 +++--
>  accel/tcg/translate-all.c     | 120 ++++------------------
>  accel/tcg/translator.c        | 122 +++++++++++++++++-----
>  accel/tcg/user-exec.c         |  15 +++
>  linux-user/elfload.c          |  80 ++++++++++++++-
>  softmmu/physmem.c             |  12 +++
>  target/alpha/translate.c      |   5 +-
>  target/arm/translate.c        |   5 +-
>  target/avr/translate.c        |   5 +-
>  target/cris/translate.c       |   5 +-
>  target/hexagon/translate.c    |   6 +-
>  target/hppa/translate.c       |   5 +-
>  target/i386/tcg/translate.c   |   7 +-
>  target/loongarch/translate.c  |   6 +-
>  target/m68k/translate.c       |   5 +-
>  target/microblaze/translate.c |   5 +-
>  target/mips/tcg/translate.c   |   5 +-
>  target/nios2/translate.c      |   5 +-
>  target/openrisc/translate.c   |   6 +-
>  target/ppc/translate.c        |   5 +-
>  target/riscv/translate.c      |   5 +-
>  target/rx/translate.c         |   5 +-
>  target/s390x/tcg/translate.c  |   5 +-
>  target/sh4/translate.c        |   5 +-
>  target/sparc/translate.c      |   5 +-
>  target/tricore/translate.c    |   6 +-
>  target/xtensa/translate.c     |   6 +-
>  tests/tcg/i386/test-i386.c    |   2 +-
>  38 files changed, 532 insertions(+), 424 deletions(-)
> 

Hi,

I need the following fixup to make my noexec tests pass with v1:

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 6a3ca8224f..cc6a43a3bc 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -386,6 +386,10 @@ const void *HELPER(lookup_tb_ptr)(CPUArchState
*env)
         return tcg_code_gen_epilogue;
     }
 
+    if (tb->page_addr[1] != -1) {
+        get_page_addr_code_hostp(env, tb->page_addr[1], false, NULL);
+    }
+
     log_cpu_exec(pc, cpu, tb);
 
     return tb->tc.ptr;
@@ -997,6 +1001,9 @@ int cpu_exec(CPUState *cpu)
                  * for the fast lookup
                  */
                 qatomic_set(&cpu-
>tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
+            } else if (tb->page_addr[1] != -1) {
+                get_page_addr_code_hostp(cpu->env_ptr, tb-
>page_addr[1], false,
+                                         NULL);
             }
             mmap_unlock();

With v2, the exception after mprotect(PROT_NONE) is not happening
again. I have not figured out what the problem is yet.

Also, wasmtime tests trigger this assertion:

static void pgb_dynamic(const char *image_name, long align)
{
    /*
     * The executable is dynamic and does not require a fixed address.
     * All we need is a commpage that satisfies align.
     * If we do not need a commpage, leave guest_base == 0.
     */
    if (HI_COMMPAGE) {
        uintptr_t addr, commpage;

        /* 64-bit hosts should have used reserved_va. */
        assert(sizeof(uintptr_t) == 4);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Likewise, I also have not figured out why this is happening.

Best regards,
Ilya