From a78b1299f1bbb9608e3e3a36a7f16cf700a2789d Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 28 Nov 2017 14:35:24 +0000 Subject: linux-user: Propagate siginfo_t through to handle_cpu_signal() Currently all the architecture/OS specific cpu_signal_handler() functions call handle_cpu_signal() without passing it the siginfo_t. We're going to want that so we can look at the si_code to determine whether this is a SEGV_ACCERR access violation or some other kind of fault, so change the functions to pass through the pointer to the siginfo_t rather than just the si_addr value. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-Id: <1511879725-9576-2-git-send-email-peter.maydell@linaro.org> Signed-off-by: Laurent Vivier --- accel/tcg/user-exec.c | 39 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 25 deletions(-) (limited to 'accel') diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index f42285ea1c..e8f26ff0cb 100644 --- a/accel/tcg/user-exec.c +++ b/accel/tcg/user-exec.c @@ -57,12 +57,13 @@ static void cpu_exit_tb_from_sighandler(CPUState *cpu, sigset_t *old_set) the effective address of the memory exception. 'is_write' is 1 if a write caused the exception and otherwise 0'. 'old_set' is the signal set which should be restored */ -static inline int handle_cpu_signal(uintptr_t pc, unsigned long address, +static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info, int is_write, sigset_t *old_set) { CPUState *cpu = current_cpu; CPUClass *cc; int ret; + unsigned long address = (unsigned long)info->si_addr; /* We must handle PC addresses from two different sources: * a call return address and a signal frame address. @@ -215,9 +216,8 @@ int cpu_signal_handler(int host_signum, void *pinfo, #endif pc = EIP_sig(uc); trapno = TRAP_sig(uc); - return handle_cpu_signal(pc, (unsigned long)info->si_addr, - trapno == 0xe ? - (ERROR_sig(uc) >> 1) & 1 : 0, + return handle_cpu_signal(pc, info, + trapno == 0xe ? (ERROR_sig(uc) >> 1) & 1 : 0, &MASK_sig(uc)); } @@ -261,9 +261,8 @@ int cpu_signal_handler(int host_signum, void *pinfo, #endif pc = PC_sig(uc); - return handle_cpu_signal(pc, (unsigned long)info->si_addr, - TRAP_sig(uc) == 0xe ? - (ERROR_sig(uc) >> 1) & 1 : 0, + return handle_cpu_signal(pc, info, + TRAP_sig(uc) == 0xe ? (ERROR_sig(uc) >> 1) & 1 : 0, &MASK_sig(uc)); } @@ -341,8 +340,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, is_write = 1; } #endif - return handle_cpu_signal(pc, (unsigned long)info->si_addr, - is_write, &uc->uc_sigmask); + return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask); } #elif defined(__alpha__) @@ -372,8 +370,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, is_write = 1; } - return handle_cpu_signal(pc, (unsigned long)info->si_addr, - is_write, &uc->uc_sigmask); + return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask); } #elif defined(__sparc__) @@ -432,8 +429,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, break; } } - return handle_cpu_signal(pc, (unsigned long)info->si_addr, - is_write, sigmask); + return handle_cpu_signal(pc, info, is_write, sigmask); } #elif defined(__arm__) @@ -466,9 +462,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, * later processor; on v5 we will always report this as a read). */ is_write = extract32(uc->uc_mcontext.error_code, 11, 1); - return handle_cpu_signal(pc, (unsigned long)info->si_addr, - is_write, - &uc->uc_sigmask); + return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask); } #elif defined(__aarch64__) @@ -495,8 +489,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, void *puc) /* Ignore bits 23 & 24, controlling indexing. */ || (insn & 0x3a400000) == 0x28000000); /* C3.3.7,14-16 */ - return handle_cpu_signal(pc, (uintptr_t)info->si_addr, - is_write, &uc->uc_sigmask); + return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask); } #elif defined(__ia64) @@ -529,9 +522,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, void *puc) default: break; } - return handle_cpu_signal(ip, (unsigned long)info->si_addr, - is_write, - (sigset_t *)&uc->uc_sigmask); + return handle_cpu_signal(ip, info, is_write, (sigset_t *)&uc->uc_sigmask); } #elif defined(__s390__) @@ -583,8 +574,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, } break; } - return handle_cpu_signal(pc, (unsigned long)info->si_addr, - is_write, &uc->uc_sigmask); + return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask); } #elif defined(__mips__) @@ -599,8 +589,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, /* XXX: compute is_write */ is_write = 0; - return handle_cpu_signal(pc, (unsigned long)info->si_addr, - is_write, &uc->uc_sigmask); + return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask); } #else -- cgit v1.2.3-55-g7522 From 9c4bbee9e3b83544257e82566342c29e15a88637 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 28 Nov 2017 14:35:25 +0000 Subject: page_unprotect(): handle calls to pages that are PAGE_WRITE If multiple guest threads in user-mode emulation write to a page which QEMU has marked read-only because of cached TCG translations, the threads can race in page_unprotect: * threads A & B both try to do a write to a page with code in it at the same time (ie which we've made non-writeable, so SEGV) * they race into the signal handler with this faulting address * thread A happens to get to page_unprotect() first and takes the mmap lock, so thread B sits waiting for it to be done * A then finds the page, marks it PAGE_WRITE and mprotect()s it writable * A can then continue OK (returns from signal handler to retry the memory access) * ...but when B gets the mmap lock it finds that the page is already PAGE_WRITE, and so it exits page_unprotect() via the "not due to protected translation" code path, and wrongly delivers the signal to the guest rather than just retrying the access In particular, this meant that trying to run 'javac' in user-mode emulation would fail with a spurious guest SIGSEGV. Handle this by making page_unprotect() assume that a call for a page which is already PAGE_WRITE is due to a race of this sort and return a "fault handled" indication. Since this would cause an infinite loop if we ever called page_unprotect() for some other kind of fault than "write failed due to bad access permissions", tighten the condition in handle_cpu_signal() to check the signal number and si_code, and add a comment so that if somebody does ever find themselves debugging an infinite loop of faults they have some clue about why. (The trick for identifying the correct setting for current_tb_invalidated for thread B (needed to handle the precise-SMC case) is due to Richard Henderson. Paolo Bonzini suggested just relying on si_code rather than trying anything more complicated.) Signed-off-by: Peter Maydell Message-Id: <1511879725-9576-3-git-send-email-peter.maydell@linaro.org> Signed-off-by: Laurent Vivier --- accel/tcg/translate-all.c | 50 +++++++++++++++++++++++++++++------------------ accel/tcg/user-exec.c | 13 +++++++++++- 2 files changed, 43 insertions(+), 20 deletions(-) (limited to 'accel') diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 7736257085..67795cd78c 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -2181,29 +2181,41 @@ int page_unprotect(target_ulong address, uintptr_t pc) /* if the page was really writable, then we change its protection back to writable */ - if ((p->flags & PAGE_WRITE_ORG) && !(p->flags & PAGE_WRITE)) { - host_start = address & qemu_host_page_mask; - host_end = host_start + qemu_host_page_size; - - prot = 0; + if (p->flags & PAGE_WRITE_ORG) { current_tb_invalidated = false; - for (addr = host_start ; addr < host_end ; addr += TARGET_PAGE_SIZE) { - p = page_find(addr >> TARGET_PAGE_BITS); - p->flags |= PAGE_WRITE; - prot |= p->flags; - - /* and since the content will be modified, we must invalidate - the corresponding translated code. */ - current_tb_invalidated |= tb_invalidate_phys_page(addr, pc); -#ifdef CONFIG_USER_ONLY - if (DEBUG_TB_CHECK_GATE) { - tb_invalidate_check(addr); + if (p->flags & PAGE_WRITE) { + /* If the page is actually marked WRITE then assume this is because + * this thread raced with another one which got here first and + * set the page to PAGE_WRITE and did the TB invalidate for us. + */ +#ifdef TARGET_HAS_PRECISE_SMC + TranslationBlock *current_tb = tb_find_pc(pc); + if (current_tb) { + current_tb_invalidated = tb_cflags(current_tb) & CF_INVALID; } #endif + } else { + host_start = address & qemu_host_page_mask; + host_end = host_start + qemu_host_page_size; + + prot = 0; + for (addr = host_start; addr < host_end; addr += TARGET_PAGE_SIZE) { + p = page_find(addr >> TARGET_PAGE_BITS); + p->flags |= PAGE_WRITE; + prot |= p->flags; + + /* and since the content will be modified, we must invalidate + the corresponding translated code. */ + current_tb_invalidated |= tb_invalidate_phys_page(addr, pc); +#ifdef CONFIG_USER_ONLY + if (DEBUG_TB_CHECK_GATE) { + tb_invalidate_check(addr); + } +#endif + } + mprotect((void *)g2h(host_start), qemu_host_page_size, + prot & PAGE_BITS); } - mprotect((void *)g2h(host_start), qemu_host_page_size, - prot & PAGE_BITS); - mmap_unlock(); /* If current TB was invalidated return to main loop */ return current_tb_invalidated ? 2 : 1; diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index e8f26ff0cb..c973752562 100644 --- a/accel/tcg/user-exec.c +++ b/accel/tcg/user-exec.c @@ -104,7 +104,18 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info, pc, address, is_write, *(unsigned long *)old_set); #endif /* XXX: locking issue */ - if (is_write && h2g_valid(address)) { + /* Note that it is important that we don't call page_unprotect() unless + * this is really a "write to nonwriteable page" fault, because + * page_unprotect() assumes that if it is called for an access to + * a page that's writeable this means we had two threads racing and + * another thread got there first and already made the page writeable; + * so we will retry the access. If we were to call page_unprotect() + * for some other kind of fault that should really be passed to the + * guest, we'd end up in an infinite loop of retrying the faulting + * access. + */ + if (is_write && info->si_signo == SIGSEGV && info->si_code == SEGV_ACCERR && + h2g_valid(address)) { switch (page_unprotect(h2g(address), pc)) { case 0: /* Fault not caused by a page marked unwritable to protect -- cgit v1.2.3-55-g7522