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 175D7C64EC4 for ; Wed, 8 Mar 2023 16:16:46 +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=ghnMo2UMcBmUNuX98+4vNIMH5yIPyvI1CZnwuNaq7Mw=; b=CWGKqprUL58UDf9akQGo5ystlc 3orqhPMp9z3K3Wpg6Lxb80I7ZTVpoI3IiDXyTIRK4OsXjGXkuXdbruOec5d67sm5yaAFMeh8WKzqb DJaDkvjpUpMsXpbxlY2pplppIMkVLCFuu//JKhUbKs0PwlO5nc7WMBiFI8+Jk88xOnLkxnmsDq73A WPNjwm58bHy0IP9j2Flzl+O3dvFUqcj30lblvwQnb98tZDq/Kgqs4QnGkF5JAeAyRkANI17ukxLq1 oCRlzx3dlzJI8DLI9Tw+PNyt7g25SWKc32KnXSWSXcnfo0OV1/csFOPtLspQZ3C6rYGRzZIZoxaly egyvHtLQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pZwSA-005tOj-Ju; Wed, 08 Mar 2023 16:15:46 +0000 Received: from mail-yb1-xb49.google.com ([2607:f8b0:4864:20::b49]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pZwS6-005tJK-Pp for linux-arm-kernel@lists.infradead.org; Wed, 08 Mar 2023 16:15:44 +0000 Received: by mail-yb1-xb49.google.com with SMTP id m6-20020a056902118600b00aeb1e3dbd1bso17790405ybu.9 for ; Wed, 08 Mar 2023 08:15:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1678292125; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=VX0arJfywVqX35g0k9tTR96olhe6Rkzh63HEalsQBTw=; b=aPIOdJt1pRbjHSKwY3bsDGW706qA4XO7N4boM4lbWxK/PBtA403Fu/YqwfRqcYvv2n kq6jNmJLvSU5VespFOiix83xyP/407O41I7oIka7d6Mb+Jf43Wcwoy9SY7H+NLzpzNhX IVm/j0rghG7cBIW3fw4KQrMwV5FkD1IGVM9HUQAv8/qHRFe3Ev0HfvKe6MtzsUn8nujz ifbuoFe4Wb/h3+yTKYa1uF1SIaLvM9DyLUW9NNF9qi61wMvp3JskFcXqicGtCWxzmhCU N3jyuSPJesCD5FjipZbFa4RD7uFXmi5utkwlJkeD3RD4sVuY3vd8ZnJ0vKKIvtvc25N0 NAkg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678292125; 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=VX0arJfywVqX35g0k9tTR96olhe6Rkzh63HEalsQBTw=; b=YdlUQR18kwXhVxKXeVLNyUDlPZDR+Q4W0hNVFjXN0K55g7fQCZv5MDxLCGV2Ok7GIz u1WyJ3+pk0MMDz92ZEZLmpw0dutmaAG+/dqWxiwa1BJFctWERJov1D+oozIS6TuchfHM SCtTbXNw6TSdHgKzQlHGMGr+BCgbMZF7nFVYZ8p/ys3XdV5pc7pLYYRu6yRvAZ5oKKYR dCM1IcOyPZQGKG9X+Y0aumZkYKrrht3BIQnBbc1aMEjFtqtvu3iQtCisUF6YgFu14kl9 q02vZiq/CuMOgSqDDCcJ0dahTM50HKz7DL6UYcnclb5Ep0UniE0NhZpWrCC9+WrvIu0g dYWg== X-Gm-Message-State: AO0yUKWsU1EhqScOPKm7Md/K54Rn9e1BHPBU+znMUuWfRxV9lGYBTheJ oFbMZIG9T0njjwldvpfiMjFTw1jx8X0= X-Google-Smtp-Source: AK7set9UJuVbluBf/SIC/ORw1wCMPIiECgOe6v4cF0q4qKiigQ1h9x3MzmAdArjQ38PeP5zmQYwjniK1pqQ= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a05:6902:524:b0:9f1:7c28:4925 with SMTP id y4-20020a056902052400b009f17c284925mr9235518ybs.9.1678292125779; Wed, 08 Mar 2023 08:15:25 -0800 (PST) Date: Wed, 8 Mar 2023 08:15:24 -0800 In-Reply-To: <20230308083947.3760066-1-oliver.upton@linux.dev> Mime-Version: 1.0 References: <20230308083947.3760066-1-oliver.upton@linux.dev> Message-ID: Subject: Re: [PATCH] KVM: arm64: Avoid inverted vcpu->mutex v. kvm->lock ordering From: Sean Christopherson To: Oliver Upton Cc: Marc Zyngier , James Morse , Suzuki K Poulose , kvmarm@lists.linux.dev, Zenghui Yu , linux-arm-kernel@lists.infradead.org, Jeremy Linton X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230308_081542_882244_2737D950 X-CRM114-Status: GOOD ( 30.67 ) 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 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? 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. > [*] https://lore.kernel.org/kvmarm/20230224191640.3396734-1-maz@kernel.org/ > > arch/arm64/include/asm/kvm_host.h | 3 +++ > arch/arm64/kvm/arm.c | 6 ++++-- > arch/arm64/kvm/hypercalls.c | 4 ++-- > arch/arm64/kvm/pmu-emul.c | 18 +++++++++--------- > arch/arm64/kvm/psci.c | 8 ++++---- > arch/arm64/kvm/reset.c | 6 +++--- > arch/arm64/kvm/vgic/vgic-init.c | 20 ++++++++++---------- > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 5 +++-- > arch/arm64/kvm/vgic/vgic-mmio.c | 12 ++++++------ > 9 files changed, 44 insertions(+), 38 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index a1892a8f6032..2b1070a526de 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -185,6 +186,8 @@ struct kvm_protected_vm { > }; > > struct kvm_arch { > + struct mutex lock; A comment explaining exactly what this protects would be very helpful for folks that aren't familiar with KVM ARM. > @@ -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 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? > 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel