From patchwork Tue Nov 28 14:35:25 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Maydell X-Patchwork-Id: 119881 Delivered-To: patches@linaro.org Received: by 10.80.225.132 with SMTP id k4csp3165768edl; Tue, 28 Nov 2017 06:35:29 -0800 (PST) X-Google-Smtp-Source: AGs4zMZOxAYab8bLG+EREtJvbM78SQY6OINzOESCiV5yUjapQpaOzyR5qkSr48Qt8/jbRGgz1+Zu X-Received: by 10.80.219.11 with SMTP id o11mr2903004edk.259.1511879729754; Tue, 28 Nov 2017 06:35:29 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1511879729; cv=none; d=google.com; s=arc-20160816; b=1ASsYQ+PMDZYO2HCdfprVLFOTVOhX+n1JfcGQdqt+NcBr94sKtubl4DtPjGb9DNGe7 8TAVPkhVM8/Xz824i2RvpUZkyy2OhiZGZ8Q4z6AG0PrQch4O2kQzW87U6cZIgZ2G0/Xo t328qbqVQbKkdt6B5MKxvcp3WwyAZFI6syfKmaThQZRM+OTSzSlNMkMb5U41wfoGBe0L nLPHyesEDm9N95dAibjArzXbWGrcifiQDz293JBEwjcXNXsfVLZM/gTCnPXWxGdWKR6V cWl+fdSO3vWOnY8tlHrAnQ6uGvngp6Iyxe6UqFXF0xNdjWLNgFe6HMusKVvNvjJQUd6n +SRw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=references:in-reply-to:message-id:date:subject:cc:to:from :arc-authentication-results; bh=JjCfT3CPzm4bh6bls7SWWgNzkz+Dbkmfk7j9+mZfBhg=; b=IbTJMNTKWTfS6o1nI8V7g/T4VQLj2uz10A2TUjQcw2fFvw9idmgiee6Rdz+qi1g/T2 o2mFwpPVaxITLYd2QLOZg6Fakt/g3W8HOUwwDkIEcBd9SoyMmoBvYTBcBPAe5lhqnAT6 NP4pw62ZAcXJ1MFNfHBreOUUQFzFNT9/lqWSFmtV+uYZerIbdjDI4T3jpAIvQDuxJAUM I1cMWokyqfkowsFTLpFkAtOcp90UJayunqRWur345ho++8apS6nne27DVDtVRxkp1Po1 Qsr1uu+HROga/AksmFO+ScpQWyJqExIkYfUdQXV2DdH5nf2OJlyzxhBmG+kpxwa3yPrD 4f7A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of pm215@archaic.org.uk designates 2001:8b0:1d0::2 as permitted sender) smtp.mailfrom=pm215@archaic.org.uk; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from orth.archaic.org.uk (orth.archaic.org.uk. [2001:8b0:1d0::2]) by mx.google.com with ESMTPS id w58si2994186edb.269.2017.11.28.06.35.29 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 28 Nov 2017 06:35:29 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of pm215@archaic.org.uk designates 2001:8b0:1d0::2 as permitted sender) client-ip=2001:8b0:1d0::2; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of pm215@archaic.org.uk designates 2001:8b0:1d0::2 as permitted sender) smtp.mailfrom=pm215@archaic.org.uk; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from pm215 by orth.archaic.org.uk with local (Exim 4.89) (envelope-from ) id 1eJgyu-00020q-2p; Tue, 28 Nov 2017 14:35:28 +0000 From: Peter Maydell To: qemu-devel@nongnu.org Cc: patches@linaro.org, Paolo Bonzini , Richard Henderson , Riku Voipio , Laurent Vivier Subject: [PATCH 2/2] page_unprotect(): handle calls to pages that are PAGE_WRITE Date: Tue, 28 Nov 2017 14:35:25 +0000 Message-Id: <1511879725-9576-3-git-send-email-peter.maydell@linaro.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1511879725-9576-1-git-send-email-peter.maydell@linaro.org> References: <1511879725-9576-1-git-send-email-peter.maydell@linaro.org> 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 --- accel/tcg/translate-all.c | 50 +++++++++++++++++++++++++++++------------------ accel/tcg/user-exec.c | 13 +++++++++++- 2 files changed, 43 insertions(+), 20 deletions(-) -- 2.7.4 diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index e7f0329..96e68d5 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -2182,29 +2182,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 e8f26ff..c973752 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