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 E3F97C4829A for ; Tue, 13 Feb 2024 16:30:07 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=GGH6pXXW+PIDOwOkkxu6DALwhNAUKc/GL006o8PKQ6g=; b=eqzU4HY5uzobcs QljcS12ez/Yr3KR8og7OExYNv6ZoPPK/C3DKN3irro+7f2XcZo2HQLCO94VFFhLXK5sQqsQIwqsuB NVVfO4fpTGwQ9+OrX2sNsWuoh2/PiimlPCbNdNkWvwpkV+lHltIwP8vGZ1B2XOeWltwHz1IZP5ae+ Dksn2TSbQHf9zEthGPrrkCQ/Nx5xAWvmfr789jcZ/QHihjwOHthkkj53qguQ7Ubjnk7yc/aGSEBT5 q4zaelFd5RhKsX84IplCCRRZLONFqBvHYeHF0BXvPISFh0/bsRY3a/d4NahntmPDznR8phc2W70tf 3OoeSVY6MiHwSPe4Oy1w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rZvfP-00000009wtw-1BRQ; Tue, 13 Feb 2024 16:29:55 +0000 Received: from out-173.mta1.migadu.com ([2001:41d0:203:375::ad]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rZvfL-00000009wtM-49Bl for linux-arm-kernel@lists.infradead.org; Tue, 13 Feb 2024 16:29:54 +0000 Date: Tue, 13 Feb 2024 16:29:42 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1707841787; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=mF7J0g2Nxf+VBOHYwoPE53ZcMPEVUheZGZS5pd1pQoU=; b=GZTwqKcD2J+jk67hFJUI3xSvi3EfxyJevFW4Mm24S4k7iXdUVJDaHRgP+ME3UCJML/Srlw mz9++W2jHHizB72MeD3m/8UTo9wxGeZFTjbl4O28g8kSfkXpLD0yjGCESVeW9t/In3J4jm 1Yds7/qCgTwuF5j0aeLIT+dHrfTQyaA= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Oliver Upton To: Marc Zyngier Cc: Will Deacon , kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Ricardo Koller Subject: Re: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked() Message-ID: References: <20240212193052.27765-1-will@kernel.org> <86cyt062jh.wl-maz@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <86cyt062jh.wl-maz@kernel.org> X-Migadu-Flow: FLOW_OUT X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240213_082952_668120_D2906997 X-CRM114-Status: GOOD ( 34.34 ) 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 On Tue, Feb 13, 2024 at 11:12:34AM +0000, Marc Zyngier wrote: > On Mon, 12 Feb 2024 20:14:37 +0000, > Oliver Upton wrote: > > > > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote: > > > kvm_pgtable_stage2_free_unlinked() does the final put_page() on the > > > root page of the sub-tree before returning, so remove the additional > > > put_page() invocations in the callers. > > > > > > Cc: Marc Zyngier > > > Cc: Oliver Upton > > > Cc: Ricardo Koller > > > Signed-off-by: Will Deacon > > > --- > > > > > > Hi folks, > > > > > > Sending this as an RFC as I only spotted it from code inspection and I'm > > > surprised others aren't seeing fireworks if it's a genuine bug. I also > > > couldn't come up with a sensible Fixes tag, as all of: > > > > > > e7c05540c694b ("KVM: arm64: Add helper for creating unlinked stage2 subtrees") > > > 8f5a3eb7513fc ("KVM: arm64: Add kvm_pgtable_stage2_split()") > > > f6a27d6dc51b2 ("KVM: arm64: Drop last page ref in kvm_pgtable_stage2_free_removed()") > > I'd blame it on the last commit, as we really ought to have it if we > have the others. > > > > > > > are actually ok in isolation. Hrm. Please tell me I'm wrong? > > > > > > arch/arm64/kvm/hyp/pgtable.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > > index c651df904fe3..ab9d05fcf98b 100644 > > > --- a/arch/arm64/kvm/hyp/pgtable.c > > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > > @@ -1419,7 +1419,6 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, > > > level + 1); > > > if (ret) { > > > kvm_pgtable_stage2_free_unlinked(mm_ops, pgtable, level); > > > - mm_ops->put_page(pgtable); > > > return ERR_PTR(ret); > > > } > > > > AFAICT, this entire branch is effectively dead code, unless there's a > > KVM bug lurking behind the page table walk. The sub-tree isn't visible > > to other software or hardware walkers yet, so none of the PTE races > > could cause this to pop. > > > > So while this is very obviously a bug, it might be pure luck that folks > > haven't seen smoke here. Perhaps while fixing the bug we should take the > > opportunity to promote the condition to WARN_ON_ONCE(). > > Can't you construct a case where an allocation fails during the walk > (memcache empty), and we end up on this exact path? Possibly, but AFAICT that can only happen if there was a bug in KVM. We don't start the walk at all if userspace set the split chunk size to 0, and otherwise we expect it to be at least PMD_SIZE, which will top up the cache to 1 every pass. stage2_split_walker() will 'do the right thing' if there aren't enough preallocated pages to get down to level 3. It really doesn't matter either way, I'm just trying to convince myself of the reasons why we haven't seen this explode yet :) > > > > > @@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx, > > > > > > if (!stage2_try_break_pte(ctx, mmu)) { > > > kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level); > > > - mm_ops->put_page(childp); > > > return -EAGAIN; > > > } > > > > This, on the other hand, seems possible. There exists a race where an > > old block PTE could have the AF set on it and the underlying cmpxchg() > > could fail. There shouldn't be a race with any software walkers, as we > > hold the MMU lock for write here. > > AF update is indeed a likely candidate. > > In any case, this patch looks good to me as it is, and we can always > have a separate tweak to adjust the severity of the first case as > required. Unless anyone objects, I'd like to queue it shortly. Agreed, happy with the way this looks and should've added: Reviewed-by: Oliver Upton the first time around. -- Thanks, Oliver _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel