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

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> On 7/9/19 12:07 PM, Alex Bennée wrote:
>>
>> 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?
>
> I'm not sure it makes sense to spread these around generically, since they are
> no-ops which require the extra help of the "host_fn" pointers within that 
> file.
>
> In particular, the softmmu host_fn continues to use ra, while the linux-user
> host_fn does not.  Indeed, the whole point of sve_helper.c using
> set_helper_retaddr is to hoist the setting of helper_retaddr that would be 
> done
> for each occurrence of cpu_ld_data_ra() et al.

Fair enough, keep the r-b.

--
Alex Bennée