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 6C1FAC61DA4 for ; Thu, 9 Mar 2023 08:54:23 +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=LmAlTRZ/5plktYHslZoDMYEo9nzAlSPL1dzxii115wM=; b=1B9Kl2fMXTpO4w DYuONx1mdo0dImb8reCnpLxQEh/B58WWWvMeBACFprp96npSP2X6rY0gugwsMgzDD8DiMZSY3F187 U4oDsPaue6RlYg+a9XWz4FVS6tRG272ArmBpfISZjncviyuWIpga7j+5CH0sTRiXYf9/xKir+NZwk nePT73dfSXZoH2csRiz5K3jHlseBaK6/9XeB5aR6tbCDky2Y3mFncnCwqei9wopo4IU4zkQKIQ0A+ K6vY9o59FTdObDAdLy5mE3Kizx/RjutASxf+XYbWoszGRN/8y6QG3ZVjXxc2R4YKxuEnMlg4jczpH Qealh0MqQQEpXCrSLZBQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1paC1a-008eIO-3d; Thu, 09 Mar 2023 08:53:22 +0000 Received: from out-37.mta1.migadu.com ([95.215.58.37]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1paC1V-008eF0-P0 for linux-arm-kernel@lists.infradead.org; Thu, 09 Mar 2023 08:53:20 +0000 Date: Thu, 9 Mar 2023 08:53:05 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1678351988; 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=i51WF518w1JT78ILCAznI/9c0YZ/t6rmLavMpyMR6Ig=; b=fbrjrJoe2PMEFPIRtAU8PI83RS4oDNe9Is4Sb+h3nSENK3sUcVhsOoCSypFnUWdHe2kDbX 6Q8M2IO89iDbCt9qDfzN86Laz9Qstw1FZ20Hocm+C6IpBFSB4XzRtabL7FjbzRU2DwCD1L BgAY3pDEM57ILg5Pjaoy+sdjK8yhtc4= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Oliver Upton To: Sean Christopherson Cc: Marc Zyngier , James Morse , Suzuki K Poulose , kvmarm@lists.linux.dev, Zenghui Yu , linux-arm-kernel@lists.infradead.org, Jeremy Linton Subject: Re: [PATCH] KVM: arm64: Avoid inverted vcpu->mutex v. kvm->lock ordering Message-ID: References: <20230308083947.3760066-1-oliver.upton@linux.dev> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230309_005318_418756_72BB8A30 X-CRM114-Status: GOOD ( 35.40 ) 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 Hey Sean, On Wed, Mar 08, 2023 at 08:15:24AM -0800, Sean Christopherson wrote: > On Wed, Mar 08, 2023, Oliver Upton wrote: > > I'm somewhat annoyed with the fix here, but annoyance alone isn't enough > > to justify significantly reworking our locking scheme, and especially > > not to address an existing bug. > > > > I believe all of the required mutual exclusion is preserved, but another > > set of eyes looking at this would be greatly appreciated. Note that the > > issues in arch_timer were separately addressed by a patch from Marc [*]. > > With both patches applied I no longer see any lock inversion warnings w/ > > selftests nor kvmtool. > > Oof. Would it make sense to split this into ~3 patches, one for each logical > area? E.g. PMU, PSCI, and vGIC? So much sense that I had done so originally! I wanted to keep all the surrounding context from lockdep together with the change, but that made a bit more of a mess. So yes. > That would help reviewers and would give you > the opportunity to elaborate on the safety of the change. Or maye even go a step > further and add multiple locks? The PMU mess in particular would benefit from > a dedicated lock. The PMU is the exact reason I didn't want to retool the locking, especially considering the serialization we have around KVM_ARCH_FLAG_HAS_RAN_ONCE. Opinions on the current state of affairs be damned, the locking we currently have ensures the PMU configuration is visible before any vCPU has had the chance to run. Correctly nesting the respective dedicated locks would be fine, but that's yet another layer for us to get wrong elsewhere :) > > @@ -961,17 +961,17 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) > > filter.action != KVM_PMU_EVENT_DENY)) > > return -EINVAL; > > > > - mutex_lock(&kvm->lock); > > + mutex_lock(&kvm->arch.lock); > > > > if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &kvm->arch.flags)) { > > - mutex_unlock(&kvm->lock); > > + mutex_unlock(&kvm->arch.lock); > > return -EBUSY; > > } > > > > if (!kvm->arch.pmu_filter) { > > kvm->arch.pmu_filter = bitmap_alloc(nr_events, GFP_KERNEL_ACCOUNT); > > I believe there's an existing bug here. nr_events is grabbed from kvm_pmu_event_mask(), > i.e. which depends on kvm->arch.arm_pmu->pmuver, outside of the lock. > > kvm_arm_pmu_v3_set_pmu() disallows changing the PMU type after a filter has been > set, but the ordering means nr_events can be computed on a stale PMU. E.g. if > userspace does KVM_ARM_VCPU_PMU_V3_SET_PMU and KVM_ARM_VCPU_PMU_V3_FILTER > concurrently on two different tasks. > > KVM_ARM_VCPU_PMU_V3_IRQ is similarly sketchy. pmu_irq_is_valid() iterates over > all vCPUs without holding any locks, which in and of itself is safe, but then it > it checks vcpu->arch.pmu.irq_num for every vCPU. I believe concurrent calls to > KVM_ARM_VCPU_PMU_V3_IRQ would potentially result in pmu_irq_is_valid() returning > a false postive. > > I don't see anything that would break by holding a lock for the entire function, > e.g. ending up with something like this Yeah, that'd certainly be the cleaner thing to do. > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 07444fa22888..2394c598e429 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -957,7 +957,9 @@ int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > > switch (attr->group) { > case KVM_ARM_VCPU_PMU_V3_CTRL: > + mutex_lock(&vcpu->kvm->arch.pmu_lock); > ret = kvm_arm_pmu_v3_set_attr(vcpu, attr); > + mutex_unlock(&vcpu->kvm->arch.pmu_lock); > break; > case KVM_ARM_VCPU_TIMER_CTRL: > ret = kvm_arm_timer_set_attr(vcpu, attr); > > > if (!kvm->arch.pmu_filter) { > > - mutex_unlock(&kvm->lock); > > + mutex_unlock(&kvm->arch.lock); > > return -ENOMEM; > > } > > > > @@ -373,7 +373,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu) > > vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF; > > } > > > > -/* To be called with kvm->lock held */ > > +/* To be called with kvm->arch.lock held */ > > Opportunistically convert to lockdep? Agreed (there's a few stragglers I didn't touch as well). > > static void __kvm_vgic_destroy(struct kvm *kvm) > > { > > struct kvm_vcpu *vcpu; > > ... > > > @@ -441,7 +441,7 @@ int kvm_vgic_map_resources(struct kvm *kvm) > > if (likely(vgic_ready(kvm))) > > return 0; > > > > - mutex_lock(&kvm->lock); > > + mutex_lock(&kvm->arch.lock); > > if (vgic_ready(kvm)) > > goto out; > > This is buggy. KVM_CREATE_IRQCHIP and KVM_CREATE_DEVICE protect vGIC creation > with kvm->lock, whereas this (obviously) now takes only kvm->arch.lock. > kvm_vgic_create() sets > > kvm->arch.vgic.in_kernel = true; > > before it has fully initialized "vgic", and so all of these flows that are being > converted can race with the final setup of the vGIC. Oops -- the intention was to have a lock that can nest within both vcpu->mutex and kvm->lock, but the latter part of this is missing. -- Thanks, Oliver _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel