Richard Henderson <richard.hender...@linaro.org> writes:
> 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.
Fair enough, keep the r-b.
--
Alex Bennée
