From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f201.google.com (mail-pg1-f201.google.com [209.85.215.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1F9EE28F2 for ; Mon, 13 Mar 2023 15:53:39 +0000 (UTC) Received: by mail-pg1-f201.google.com with SMTP id az5-20020a056a02004500b004fb64e929f2so1511909pgb.7 for ; Mon, 13 Mar 2023 08:53:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1678722819; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=Ou4eTHfchaovdyTcvsOJz1A/GQo+JKYaMd0j8j6QHK8=; b=RqXh4bxfWB+D0kxLETg5U2ZiLZMoAh0XWbDL0HXjMPHKUBEeKXZCJohVf4RzW3rVv5 8E+1HTtAEIVvoyiAcfqSBcklqe0g5oNyXazRlXz2BJttEoIFQdpT7ow8bz2aLzGxReTR OUBj8yx1eFJRKJCgHg3oyU0XE48Xpd7PqTYQfgrZYpnSIO3/YNrkD5HR7enqiBGPEHHs t+Rtb43jLjOG3gF0NBUb7u23o+hBeaIR5BI6ViYiMKbanT2WPKiTbbabKSpys4/obFBm izJOOdKlEZPrbw7aWNzahXLXygf59Pod28WLA/vDuvvwitqaKJ4h0NQPzNnfBy/2kYLd Hwjg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678722819; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Ou4eTHfchaovdyTcvsOJz1A/GQo+JKYaMd0j8j6QHK8=; b=BCfVoxPc3i0u+X0xr4EH+SLeemXdk+BQFHYbtlmWIzvEvHX8p3k6Q/Z9Mu/EFQx1iF 7/IdrJ91xrcuXoqKGrnzG+HIBhS2wpYDuvMPTZ6sx5FcLdLU2D8fqo7FYKd5ntmxVU7Z OCJQeDyALbzIxVx36mX8VNM/GzAi901O/vCcBTeKLFt0vbmlosJDTzDuxu+OL3u3fhED Cyh64u106I5sbMNjaBi1KYffQkqdUN3gJDCrojNLnsr1rBgWE9Qh2XmiT0jyo7vs/6LV Wfw5jAakw6IUUrrXk4jvd8BgAjp04GATDOwHxuPiOezpt84sUi8+cmenCXg2pJkaiV1P hlgQ== X-Gm-Message-State: AO0yUKWnDnHuzZYN9AX9RCpcQubj8xlilz0FekJhL/lAHPQi+qhjcc0G oIEsEA8s3xyfB+a+ypubrb92IulNVhA= X-Google-Smtp-Source: AK7set/B3AXUsWJi39YJ5IRScPm11y6++Mvi/PFxtBwCwD2hD3aoPc9yCn2f2bYzX1HNyskG85ynwYN2s2k= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a17:903:33cb:b0:199:6e3:187a with SMTP id kc11-20020a17090333cb00b0019906e3187amr4475804plb.6.1678722819539; Mon, 13 Mar 2023 08:53:39 -0700 (PDT) Date: Mon, 13 Mar 2023 08:53:37 -0700 In-Reply-To: <20230313091425.1962708-2-maz@kernel.org> Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20230313091425.1962708-1-maz@kernel.org> <20230313091425.1962708-2-maz@kernel.org> Message-ID: Subject: Re: [PATCH 1/2] KVM: arm64: Disable interrupts while walking userspace PTs From: Sean Christopherson To: Marc Zyngier Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, James Morse , Suzuki K Poulose , Oliver Upton , Zenghui Yu , Ard Biesheuvel , Will Deacon , Quentin Perret , stable@vger.kernel.org, David Matlack Content-Type: text/plain; charset="us-ascii" +David On Mon, Mar 13, 2023, Marc Zyngier wrote: > We walk the userspace PTs to discover what mapping size was > used there. However, this can race against the userspace tables > being freed, and we end-up in the weeds. > > Thankfully, the mm code is being generous and will IPI us when > doing so. So let's implement our part of the bargain and disable > interrupts around the walk. This ensures that nothing terrible > happens during that time. > > We still need to handle the removal of the page tables before > the walk. For that, allow get_user_mapping_size() to return an > error, and make sure this error can be propagated all the way > to the the exit handler. > > Signed-off-by: Marc Zyngier > Cc: stable@vger.kernel.org > --- > arch/arm64/kvm/mmu.c | 35 ++++++++++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 7113587222ff..d7b8b25942df 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -666,14 +666,23 @@ static int get_user_mapping_size(struct kvm *kvm, u64 addr) > CONFIG_PGTABLE_LEVELS), > .mm_ops = &kvm_user_mm_ops, > }; > + unsigned long flags; > kvm_pte_t pte = 0; /* Keep GCC quiet... */ > u32 level = ~0; > int ret; > > + /* > + * Disable IRQs so that we hazard against a concurrent > + * teardown of the userspace page tables (which relies on > + * IPI-ing threads). > + */ > + local_irq_save(flags); > ret = kvm_pgtable_get_leaf(&pgt, addr, &pte, &level); > - VM_BUG_ON(ret); > - VM_BUG_ON(level >= KVM_PGTABLE_MAX_LEVELS); > - VM_BUG_ON(!(pte & PTE_VALID)); > + local_irq_restore(flags); > + > + /* Oops, the userspace PTs are gone... */ > + if (ret || level >= KVM_PGTABLE_MAX_LEVELS || !(pte & PTE_VALID)) > + return -EFAULT; I don't think this should return -EFAULT all the way out to userspace. Unless arm64 differs from x86 in terms of how the userspace page tables are managed, not having a valid translation _right now_ doesn't mean that one can't be created in the future, e.g. by way of a subsequent hva_to_pfn(). FWIW, the approach x86 takes is to install a 4KiB (smallest granuale) translation, which is safe since there _was_ a valid translation when mmu_lock was acquired and mmu_invalidate_retry() was checked. It's the primary MMU's responsibility to ensure all secondary MMUs are purged before freeing memory, i.e. worst case should be that KVMs stage-2 translation will be immediately zapped via mmu_notifier. KVM ARM also has a bug that might be related: the mmu_seq snapshot needs to be taken _before_ mmap_read_unlock(), otherwise vma_shift may be stale by the time it's consumed. I believe David is going to submit a patch (I found and "reported" the bug when doing an internal review of "common MMU" stuff). From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4D9D0C6FD19 for ; Mon, 13 Mar 2023 16:12:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:From:Subject:Message-ID: References:Mime-Version:In-Reply-To:Date:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=nnxMKVoiqBdj96AVNRh18kTRUun7A2/8ar+fj0ZotGc=; b=fc0HpYY7G7b8eEMSpepaR5KfDI vKdTy+rLWDqNUAJAZh8JePjwZXXm/bor6DXhJSe1F6Wfuy2NIabBMUqE46ChCctFzYf2OxyUKwv0A +ElSo1vfQx2Xxa4M7inFWNV7kaebkvEI1VgoSnoKHSaPYA1SA+fsBr3pPuWpjm0N1vbyKkbvlqow0 BtdxvodTjvreFwCvVPgTBUC0dofJiHS0O4P9taVXNf8rfhqbhYIAFqqtsjX2nsc8vH5LzqA65N4LO /NLe6AFlBtWA9NzYaeic7Yh1ake2nui7jJZuWtiTIEoNV8ndlI9NwhfU9Wf1p8hQSLlM18NAi1q1c mtTqoIuw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pbklj-006efH-6W; Mon, 13 Mar 2023 16:11:28 +0000 Received: from mail-pg1-x549.google.com ([2607:f8b0:4864:20::549]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pbkUY-006WVB-AJ for linux-arm-kernel@lists.infradead.org; Mon, 13 Mar 2023 15:53:43 +0000 Received: by mail-pg1-x549.google.com with SMTP id t2-20020a632d02000000b005075b896422so2753062pgt.19 for ; Mon, 13 Mar 2023 08:53:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1678722819; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=Ou4eTHfchaovdyTcvsOJz1A/GQo+JKYaMd0j8j6QHK8=; b=RqXh4bxfWB+D0kxLETg5U2ZiLZMoAh0XWbDL0HXjMPHKUBEeKXZCJohVf4RzW3rVv5 8E+1HTtAEIVvoyiAcfqSBcklqe0g5oNyXazRlXz2BJttEoIFQdpT7ow8bz2aLzGxReTR OUBj8yx1eFJRKJCgHg3oyU0XE48Xpd7PqTYQfgrZYpnSIO3/YNrkD5HR7enqiBGPEHHs t+Rtb43jLjOG3gF0NBUb7u23o+hBeaIR5BI6ViYiMKbanT2WPKiTbbabKSpys4/obFBm izJOOdKlEZPrbw7aWNzahXLXygf59Pod28WLA/vDuvvwitqaKJ4h0NQPzNnfBy/2kYLd Hwjg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678722819; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Ou4eTHfchaovdyTcvsOJz1A/GQo+JKYaMd0j8j6QHK8=; b=B2XVHbC0/VH0n/pRBpdUUyV953EuFdcAynxKrtZa2V0faawWWhJt+9xZJqACU7+L7n PCamlEs51QtgSPPDtnHtl49m3MgRNAXdRKAYEfVFnfzk2i7ev9M/Rf+qV+UUS0touut3 dhXi9jGEC/l/mL03VRa93OlNTa6BxeR4bhWUS2NkgQa6b0jp99t4Alx6yGKkCTL1nY0k Iv/81O6xqp16i654RP9xVkU68z8/dRXFBC47+I4UCZizSAf2YWQmJIXxKfuvfn32+GxN LvgPcYFGifnNZZWNq107WJoUPAWnW36j6b1OShiH9WmN5+z6K4fzmQQZKufCIlWPDEjy be7g== X-Gm-Message-State: AO0yUKXHwIvSx7p/9sf1A6b0qzh6thFHWhvLXtodfOieo9r/DJL2CjWt gZdEQa9vsDVWa7RtAhj18F6ylSbVxOI= X-Google-Smtp-Source: AK7set/B3AXUsWJi39YJ5IRScPm11y6++Mvi/PFxtBwCwD2hD3aoPc9yCn2f2bYzX1HNyskG85ynwYN2s2k= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a17:903:33cb:b0:199:6e3:187a with SMTP id kc11-20020a17090333cb00b0019906e3187amr4475804plb.6.1678722819539; Mon, 13 Mar 2023 08:53:39 -0700 (PDT) Date: Mon, 13 Mar 2023 08:53:37 -0700 In-Reply-To: <20230313091425.1962708-2-maz@kernel.org> Mime-Version: 1.0 References: <20230313091425.1962708-1-maz@kernel.org> <20230313091425.1962708-2-maz@kernel.org> Message-ID: Subject: Re: [PATCH 1/2] KVM: arm64: Disable interrupts while walking userspace PTs From: Sean Christopherson To: Marc Zyngier Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, James Morse , Suzuki K Poulose , Oliver Upton , Zenghui Yu , Ard Biesheuvel , Will Deacon , Quentin Perret , stable@vger.kernel.org, David Matlack X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230313_085342_414186_A48CEB64 X-CRM114-Status: GOOD ( 29.46 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org +David On Mon, Mar 13, 2023, Marc Zyngier wrote: > We walk the userspace PTs to discover what mapping size was > used there. However, this can race against the userspace tables > being freed, and we end-up in the weeds. > > Thankfully, the mm code is being generous and will IPI us when > doing so. So let's implement our part of the bargain and disable > interrupts around the walk. This ensures that nothing terrible > happens during that time. > > We still need to handle the removal of the page tables before > the walk. For that, allow get_user_mapping_size() to return an > error, and make sure this error can be propagated all the way > to the the exit handler. > > Signed-off-by: Marc Zyngier > Cc: stable@vger.kernel.org > --- > arch/arm64/kvm/mmu.c | 35 ++++++++++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 7113587222ff..d7b8b25942df 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -666,14 +666,23 @@ static int get_user_mapping_size(struct kvm *kvm, u64 addr) > CONFIG_PGTABLE_LEVELS), > .mm_ops = &kvm_user_mm_ops, > }; > + unsigned long flags; > kvm_pte_t pte = 0; /* Keep GCC quiet... */ > u32 level = ~0; > int ret; > > + /* > + * Disable IRQs so that we hazard against a concurrent > + * teardown of the userspace page tables (which relies on > + * IPI-ing threads). > + */ > + local_irq_save(flags); > ret = kvm_pgtable_get_leaf(&pgt, addr, &pte, &level); > - VM_BUG_ON(ret); > - VM_BUG_ON(level >= KVM_PGTABLE_MAX_LEVELS); > - VM_BUG_ON(!(pte & PTE_VALID)); > + local_irq_restore(flags); > + > + /* Oops, the userspace PTs are gone... */ > + if (ret || level >= KVM_PGTABLE_MAX_LEVELS || !(pte & PTE_VALID)) > + return -EFAULT; I don't think this should return -EFAULT all the way out to userspace. Unless arm64 differs from x86 in terms of how the userspace page tables are managed, not having a valid translation _right now_ doesn't mean that one can't be created in the future, e.g. by way of a subsequent hva_to_pfn(). FWIW, the approach x86 takes is to install a 4KiB (smallest granuale) translation, which is safe since there _was_ a valid translation when mmu_lock was acquired and mmu_invalidate_retry() was checked. It's the primary MMU's responsibility to ensure all secondary MMUs are purged before freeing memory, i.e. worst case should be that KVMs stage-2 translation will be immediately zapped via mmu_notifier. KVM ARM also has a bug that might be related: the mmu_seq snapshot needs to be taken _before_ mmap_read_unlock(), otherwise vma_shift may be stale by the time it's consumed. I believe David is going to submit a patch (I found and "reported" the bug when doing an internal review of "common MMU" stuff). _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel