On 7/9/19 12:07 PM, Alex Bennée wrote:
>
> Richard Henderson <richard.hender...@linaro.org> 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 <richard.hender...@linaro.org>
>> ---
>> 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.
r~
