From patchwork Tue Jan 23 14:48:06 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Vivier X-Patchwork-Id: 125526 Delivered-To: patch@linaro.org Received: by 10.46.66.141 with SMTP id h13csp1803692ljf; Tue, 23 Jan 2018 06:55:53 -0800 (PST) X-Google-Smtp-Source: AH8x224zAWa9L0437PEUDK/i0oMfCdsvVBToBso4EcPS4C2Tr+v/VVwZ+Xw8lcp0lwYruDnyiB5W X-Received: by 10.37.4.10 with SMTP id 10mr2714987ybe.473.1516719353548; Tue, 23 Jan 2018 06:55:53 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1516719353; cv=none; d=google.com; s=arc-20160816; b=Rdj4nr7u1bRfVC+ndDBaXxOwgN8UnrhC02M5JydVDmpE0goVUS84COMC3ggb4r52nM 49y485f8bbCzZIz7Hcpa6VwoHNh+Avock0WMGf4hyUiLS7PgOhk8rl1sah4ug2Bluker Tqvm2qHdfJ+N8y7o66Uc2wCCyStEmDcZDHcH4HtYJ13Lk+A7F8gGfMAkPLwX8OAY0rL8 mBUKjj43yf2ayCLb3w1+4MLZbnyiHGjMYH2EI0ZhLp96xU5hl/0z3DGdDC07Ut12sHXL +1GRaDgFwhKfoP0m4+XyaI654Uwi+dlwj520H/R6hehPEBCZIS05E1dXr5N4KiOwb8sB SxVg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:cc:list-subscribe:list-help:list-post:list-archive :list-unsubscribe:list-id:precedence:subject:references:in-reply-to :message-id:date:to:from:arc-authentication-results; bh=VTErDZaA6Oz3Gsqef1iFeEj39LcBVyN556A97mcBN0Y=; b=qMNwR0Fb2mgVz2ehLKAAUcXqefJjUtXIz8p7oylQ6NaADag6REkVA4ZBCfbwfxwMu3 udR9wKdWzNogNV0dnxg/3NvRKA4P6PTRKYf3asbq7g5CMBi/YBG5PgfZWeFwroCli7Zv FLkIaTRCO90oy1PkEIwTtEahw1jvW3Waq2bX3I4Zee/gCCl5e5dKi9nurqHS58cDaS+z u2/lPrZ8JVbw9/btb32/WTOGyIcygXaLTtvDFAgmSKZmH3Wnw4kHE2KINYadvKRebRC4 ZTrIIiOp9FpdWH6szDHnYSU/dTxwwV/6l8fbcjOr2cfxHDd9dAXHmgqZhdw5fWQXfja4 ryWg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom=qemu-devel-bounces+patch=linaro.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id z193si601707yba.164.2018.01.23.06.55.53 for (version=TLS1 cipher=AES128-SHA bits=128/128); Tue, 23 Jan 2018 06:55:53 -0800 (PST) Received-SPF: pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Authentication-Results: mx.google.com; spf=pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom=qemu-devel-bounces+patch=linaro.org@nongnu.org Received: from localhost ([::1]:33639 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1edzzM-0004xW-Th for patch@linaro.org; Tue, 23 Jan 2018 09:55:52 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43522) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1edzs6-00078R-1Y for qemu-devel@nongnu.org; Tue, 23 Jan 2018 09:48:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1edzs1-0002Rg-M0 for qemu-devel@nongnu.org; Tue, 23 Jan 2018 09:48:22 -0500 Received: from mout.kundenserver.de ([212.227.17.10]:63310) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1edzs1-0002Qa-Ba for qemu-devel@nongnu.org; Tue, 23 Jan 2018 09:48:17 -0500 Received: from localhost.localdomain ([78.238.229.36]) by mrelayeu.kundenserver.de (mreue102 [212.227.15.183]) with ESMTPSA (Nemesis) id 0MfHZq-1eSNyH22ck-00OoaS; Tue, 23 Jan 2018 15:48:15 +0100 From: Laurent Vivier To: qemu-devel@nongnu.org Date: Tue, 23 Jan 2018 15:48:06 +0100 Message-Id: <20180123144807.5618-13-laurent@vivier.eu> X-Mailer: git-send-email 2.14.3 In-Reply-To: <20180123144807.5618-1-laurent@vivier.eu> References: <20180123144807.5618-1-laurent@vivier.eu> X-Provags-ID: V03:K0:t7jvgJY2Pzd3rNu8ICtaAYMPJwotmFxCJyksQJOUjRo9WtHi1/Z 4adLVJ9J1ysZfITQaa8BaZ5fhpDdSsoCMcuikzFZI5sZMo9+lnEfEInLHnE9en4uSIiniXS b5g5wCxZdwtsTstuVD2crQV89/kSMb6ZnV80SpII/1+jZcCVhFx+ZcDHe/AvQOTFA+hbwjo TBj4iTSy5T+XO/jNsZvrw== X-UI-Out-Filterresults: notjunk:1; V01:K0:NydL/eYLj2E=:ydFoFm3lt6wpMMpn/FWtYV xKBbnrViYhdlOZvpEWPnjObL440+G7S0r8wHH7JV34KdDVtgySwAVc0X13tlWxxk4cvVyWsD/ en7kX14H2N+R2kueQb/UNvbf/nwukmIB+JK53JimVeymG4gb6z2E8DXKpABXrmnojvatWqt3S eS4czXSQjRMoDH41r1dFsXe24cM+pA4gR9H9kaYxMIgH97elZ8hBJo7D+0q7em+8oYMgZuRZJ aV6Omm+ZKzpzGOlSLVK9yEuf9h9axXFIDjgNBvKX3bU/kTerHZgvGAkZ79nM8qsAjn2H+Wk0/ gFTe++kIvzOCQFsSftNKSeWM5oNHbVO5CPSZTC6yRWDlPCRVkt2XQDryMcECka4bKzsX8A8Vc HfQA4paMXjLiIBz2Afvvh8ln1q3CT5DtGHDUNgt0jBouQ7aRtiVMiqQUzPCR7wn7JY/NJ019A VgnSgo5e0eonZ5DVkkKhh4SCS60MkRG79dll/lleXUE/HYDmqKhmLXqotBgZFT0sJQlZC6wj/ 46wMUFwPB5/dmfwUwPnXxNORjhm7zA3pDu6rak+lV8oOB75ndb0VVP77+2xbnur2703HBsJmo 52uq2SUBG3d06B1v4ScFXKrdLhFPbiWH8/VtXWfUVo18sJg/j4EO0QyUx7y+mAkqydp7CDIL6 m0J3qCqXDnpSPq21Ar3Ebfp1yQVFGqXfuVX6IzCbb7ianR/IVlgvVCpVY7kpVuBiA32p76Nnl auE8I1pZzIeHUR0GlEcoM0nqLbVW2d/qNniaUQ== X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 212.227.17.10 Subject: [Qemu-devel] [PULL 12/13] page_unprotect(): handle calls to pages that are PAGE_WRITE X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , Laurent Vivier Errors-To: qemu-devel-bounces+patch=linaro.org@nongnu.org Sender: "Qemu-devel" From: Peter Maydell 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(-) -- 2.14.3 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