Re: [Qemu-devel] [PATCH 2/5] tcg: Introduce set/clear_helper_retaddr

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> At present we have a potential error in that helper_retaddr contains
> data for handle_cpu_signal, but we have not ensured that those stores
> will be scheduled properly before the operation that may fault.
>
> It might be that these races are not in practice observable, due to
> our use of -fno-strict-aliasing, but better safe than sorry.
>
> Adjust all of the setters of helper_retaddr.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  include/exec/cpu_ldst.h                   | 20 +++++++++++
>  include/exec/cpu_ldst_useronly_template.h | 12 +++----
>  accel/tcg/user-exec.c                     | 11 +++---
>  target/arm/helper-a64.c                   |  8 ++---
>  target/arm/sve_helper.c                   | 43 +++++++++++------------
>  5 files changed, 57 insertions(+), 37 deletions(-)
>
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index a08b11bd2c..9de8c93303 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -89,6 +89,26 @@ typedef target_ulong abi_ptr;
>
>  extern __thread uintptr_t helper_retaddr;
>
> +static inline void set_helper_retaddr(uintptr_t ra)
> +{
> +    helper_retaddr = ra;
> +    /*
> +     * Ensure that this write is visible to the SIGSEGV handler that
> +     * may be invoked due to a subsequent invalid memory operation.
> +     */
> +    signal_barrier();
> +}
> +
> +static inline void clear_helper_retaddr(void)
> +{
> +    /*
> +     * Ensure that previous memory operations have succeeded before
> +     * removing the data visible to the signal handler.
> +     */
> +    signal_barrier();
> +    helper_retaddr = 0;
> +}
> +
>  /* In user-only mode we provide only the _code and _data accessors. */
>
>  #define MEMSUFFIX _data
> diff --git a/include/exec/cpu_ldst_useronly_template.h 
> b/include/exec/cpu_ldst_useronly_template.h
> index bc45e2b8d4..e65733f7e2 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -78,9 +78,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), 
> _ra)(CPUArchState *env,
>                                                    uintptr_t retaddr)
>  {
>      RES_TYPE ret;
> -    helper_retaddr = retaddr;
> +    set_helper_retaddr(retaddr);
>      ret = glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>      return ret;
>  }
>
> @@ -102,9 +102,9 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), 
> _ra)(CPUArchState *env,
>                                                    uintptr_t retaddr)
>  {
>      int ret;
> -    helper_retaddr = retaddr;
> +    set_helper_retaddr(retaddr);
>      ret = glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>      return ret;
>  }
>  #endif
> @@ -128,9 +128,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), 
> _ra)(CPUArchState *env,
>                                                    RES_TYPE v,
>                                                    uintptr_t retaddr)
>  {
> -    helper_retaddr = retaddr;
> +    set_helper_retaddr(retaddr);
>      glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(env, ptr, v);
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>  }
>  #endif
>
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index cb5f4b19c5..4384b59a4d 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -134,7 +134,7 @@ static inline int handle_cpu_signal(uintptr_t pc, 
> siginfo_t *info,
>               * currently executing TB was modified and must be exited
>               * immediately.  Clear helper_retaddr for next execution.
>               */
> -            helper_retaddr = 0;
> +            clear_helper_retaddr();
>              cpu_exit_tb_from_sighandler(cpu, old_set);
>              /* NORETURN */
>
> @@ -152,7 +152,7 @@ static inline int handle_cpu_signal(uintptr_t pc, 
> siginfo_t *info,
>       * an exception.  Undo signal and retaddr state prior to longjmp.
>       */
>      sigprocmask(SIG_SETMASK, old_set, NULL);
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>
>      cc = CPU_GET_CLASS(cpu);
>      access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
> @@ -682,14 +682,15 @@ static void *atomic_mmu_lookup(CPUArchState *env, 
> target_ulong addr,
>      if (unlikely(addr & (size - 1))) {
>          cpu_loop_exit_atomic(env_cpu(env), retaddr);
>      }
> -    helper_retaddr = retaddr;
> -    return g2h(addr);
> +    void *ret = g2h(addr);
> +    set_helper_retaddr(retaddr);
> +    return ret;
>  }
>
>  /* Macro to call the above, with local variables from the use context.  */
>  #define ATOMIC_MMU_DECLS do {} while (0)
>  #define ATOMIC_MMU_LOOKUP  atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
> -#define ATOMIC_MMU_CLEANUP do { helper_retaddr = 0; } while (0)
> +#define ATOMIC_MMU_CLEANUP do { clear_helper_retaddr(); } while (0)
>
>  #define ATOMIC_NAME(X)   HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
>  #define EXTRA_ARGS
> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
> index 44e45a8037..060699b901 100644
> --- a/target/arm/helper-a64.c
> +++ b/target/arm/helper-a64.c
> @@ -554,7 +554,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, 
> uint64_t addr,
>      /* ??? Enforce alignment.  */
>      uint64_t *haddr = g2h(addr);
>
> -    helper_retaddr = ra;
> +    set_helper_retaddr(ra);
>      o0 = ldq_le_p(haddr + 0);
>      o1 = ldq_le_p(haddr + 1);
>      oldv = int128_make128(o0, o1);
> @@ -564,7 +564,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, 
> uint64_t addr,
>          stq_le_p(haddr + 0, int128_getlo(newv));
>          stq_le_p(haddr + 1, int128_gethi(newv));
>      }
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>  #else
>      int mem_idx = cpu_mmu_index(env, false);
>      TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
> @@ -624,7 +624,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, 
> uint64_t addr,
>      /* ??? Enforce alignment.  */
>      uint64_t *haddr = g2h(addr);
>
> -    helper_retaddr = ra;
> +    set_helper_retaddr(ra);
>      o1 = ldq_be_p(haddr + 0);
>      o0 = ldq_be_p(haddr + 1);
>      oldv = int128_make128(o0, o1);
> @@ -634,7 +634,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, 
> uint64_t addr,
>          stq_be_p(haddr + 0, int128_gethi(newv));
>          stq_be_p(haddr + 1, int128_getlo(newv));
>      }
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>  #else
>      int mem_idx = cpu_mmu_index(env, false);
>      TCGMemOpIdx oi0 = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx);
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index fd434c66ea..fc0c1755d2 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -4125,12 +4125,11 @@ static intptr_t max_for_page(target_ulong base, 
> intptr_t mem_off,
>      return MIN(split, mem_max - mem_off) + mem_off;
>  }
>
> -static inline void set_helper_retaddr(uintptr_t ra)
> -{
> -#ifdef CONFIG_USER_ONLY
> -    helper_retaddr = ra;
> +#ifndef CONFIG_USER_ONLY
> +/* These are normally defined only for CONFIG_USER_ONLY in <exec/cpu_ldst.h> 
> */
> +static inline void set_helper_retaddr(uintptr_t ra) { }
> +static inline void clear_helper_retaddr(void) { }

Why aren't these stubs in the #else leg of cpu_ldst.h?

With that:

Reviewed-by: Alex Bennée <address@hidden>

--
Alex Bennée