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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 61D9CC4332F for ; Wed, 23 Nov 2022 12:08:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236423AbiKWMIO (ORCPT ); Wed, 23 Nov 2022 07:08:14 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46162 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235733AbiKWMIN (ORCPT ); Wed, 23 Nov 2022 07:08:13 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2254ABF61 for ; Wed, 23 Nov 2022 04:08:12 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id D7266B81F1B for ; Wed, 23 Nov 2022 12:08:10 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9072BC433C1; Wed, 23 Nov 2022 12:08:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1669205289; bh=zc1JAJ1qABLUREShjyvcqOP2+eRNq/1hNKrS8QmOzvY=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=GA/EV/BE4t6W+yoyfl0llIaHnFVECwHAS7GVpd5ba7Nvh/zcIKbwCgOYt47YzPDrj 1W6Dxrc8IYfxqb/DX7nDyVTYELV9Ee8XXLEr4olNTFq6RKKMzNp6tE6JxvA4qVnEAv DvDVrFpOvSFHFAyb2M/EHUxXqtqDhbbwKRekglZyPrXm7cvouy9Cd13FTQGTVu3HRD Bj3QqBZBRzkBwGjcG3TZo6EdKEHjtbu9qbxYDAWsfaLyx8qVPUU1OSlVNfUenIm0pV j2jwpks4SStv3Id/77Xkmz3l1W0nTBIXW33AK66JkVDva86bOX8H0hPZyxKaO+/4fe R79RhAjejfaiQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oxoXu-0086iQ-07; Wed, 23 Nov 2022 12:08:07 +0000 Date: Wed, 23 Nov 2022 12:08:05 +0000 Message-ID: <86k03loouy.wl-maz@kernel.org> From: Marc Zyngier To: chenxiang Cc: , , , Subject: Re: [PATCH v2] vfio/pci: Verify each MSI vector to avoid invalid MSI vectors In-Reply-To: <1669167756-196788-1-git-send-email-chenxiang66@hisilicon.com> References: <1669167756-196788-1-git-send-email-chenxiang66@hisilicon.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: chenxiang66@hisilicon.com, alex.williamson@redhat.com, kvm@vger.kernel.org, qemu-devel@nongnu.org, linuxarm@huawei.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Wed, 23 Nov 2022 01:42:36 +0000, chenxiang wrote: > > From: Xiang Chen > > Currently the number of MSI vectors comes from register PCI_MSI_FLAGS > which should be power-of-2 in qemu, in some scenaries it is not the same as > the number that driver requires in guest, for example, a PCI driver wants > to allocate 6 MSI vecotrs in guest, but as the limitation, it will allocate > 8 MSI vectors. So it requires 8 MSI vectors in qemu while the driver in > guest only wants to allocate 6 MSI vectors. > > When GICv4.1 is enabled, it iterates over all possible MSIs and enable the > forwarding while the guest has only created some of mappings in the virtual > ITS, so some calls fail. The exception print is as following: > vfio-pci 0000:3a:00.1: irq bypass producer (token 000000008f08224d) registration > fails:66311 > > To avoid the issue, verify each MSI vector, skip some operations such as > request_irq() and irq_bypass_register_producer() for those invalid MSI vectors. > > Signed-off-by: Xiang Chen > --- > I reported the issue at the link: > https://lkml.kernel.org/lkml/87cze9lcut.wl-maz@kernel.org/T/ > > Change Log: > v1 -> v2: > Verify each MSI vector in kernel instead of adding systemcall according to > Mar's suggestion > --- > arch/arm64/kvm/vgic/vgic-irqfd.c | 13 +++++++++++++ > arch/arm64/kvm/vgic/vgic-its.c | 36 ++++++++++++++++++++++++++++++++++++ > arch/arm64/kvm/vgic/vgic.h | 1 + > drivers/vfio/pci/vfio_pci_intrs.c | 33 +++++++++++++++++++++++++++++++++ > include/linux/kvm_host.h | 2 ++ > 5 files changed, 85 insertions(+) > > diff --git a/arch/arm64/kvm/vgic/vgic-irqfd.c b/arch/arm64/kvm/vgic/vgic-irqfd.c > index 475059b..71f6af57 100644 > --- a/arch/arm64/kvm/vgic/vgic-irqfd.c > +++ b/arch/arm64/kvm/vgic/vgic-irqfd.c > @@ -98,6 +98,19 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, > return vgic_its_inject_msi(kvm, &msi); > } > > +int kvm_verify_msi(struct kvm *kvm, > + struct kvm_kernel_irq_routing_entry *irq_entry) > +{ > + struct kvm_msi msi; > + > + if (!vgic_has_its(kvm)) > + return -ENODEV; > + > + kvm_populate_msi(irq_entry, &msi); > + > + return vgic_its_verify_msi(kvm, &msi); > +} > + > /** > * kvm_arch_set_irq_inatomic: fast-path for irqfd injection > */ > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > index 94a666d..8312a4a 100644 > --- a/arch/arm64/kvm/vgic/vgic-its.c > +++ b/arch/arm64/kvm/vgic/vgic-its.c > @@ -767,6 +767,42 @@ int vgic_its_inject_cached_translation(struct kvm *kvm, struct kvm_msi *msi) > return 0; > } > > +int vgic_its_verify_msi(struct kvm *kvm, struct kvm_msi *msi) > +{ > + struct vgic_its *its; > + struct its_ite *ite; > + struct kvm_vcpu *vcpu; > + int ret = 0; > + > + if (!irqchip_in_kernel(kvm) || (msi->flags & ~KVM_MSI_VALID_DEVID)) > + return -EINVAL; > + > + if (!vgic_has_its(kvm)) > + return -ENODEV; > + > + its = vgic_msi_to_its(kvm, msi); > + if (IS_ERR(its)) > + return PTR_ERR(its); > + > + mutex_lock(&its->its_lock); > + if (!its->enabled) { > + ret = -EBUSY; > + goto unlock; > + } > + ite = find_ite(its, msi->devid, msi->data); > + if (!ite || !its_is_collection_mapped(ite->collection)) { > + ret = E_ITS_INT_UNMAPPED_INTERRUPT; > + goto unlock; > + } > + > + vcpu = kvm_get_vcpu(kvm, ite->collection->target_addr); > + if (!vcpu) > + ret = E_ITS_INT_UNMAPPED_INTERRUPT; I'm sorry, but what does this mean to the caller? This should never leak outside of the ITS code. > +unlock: > + mutex_unlock(&its->its_lock); > + return ret; > +} > + > /* > * Queries the KVM IO bus framework to get the ITS pointer from the given > * doorbell address. > diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h > index 0c8da72..d452150 100644 > --- a/arch/arm64/kvm/vgic/vgic.h > +++ b/arch/arm64/kvm/vgic/vgic.h > @@ -240,6 +240,7 @@ int kvm_vgic_register_its_device(void); > void vgic_enable_lpis(struct kvm_vcpu *vcpu); > void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu); > int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi); > +int vgic_its_verify_msi(struct kvm *kvm, struct kvm_msi *msi); > int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr); > int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write, > int offset, u32 *val); > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > index 40c3d7c..3027805 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > > #include "vfio_pci_priv.h" > > @@ -315,6 +316,28 @@ static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msi > return 0; > } > > +static int vfio_pci_verify_msi_entry(struct vfio_pci_core_device *vdev, > + struct eventfd_ctx *trigger) > +{ > + struct kvm *kvm = vdev->vdev.kvm; > + struct kvm_kernel_irqfd *tmp; > + struct kvm_kernel_irq_routing_entry irq_entry; > + int ret = -ENODEV; > + > + spin_lock_irq(&kvm->irqfds.lock); > + list_for_each_entry(tmp, &kvm->irqfds.items, list) { > + if (trigger == tmp->eventfd) { > + ret = 0; > + break; > + } > + } > + spin_unlock_irq(&kvm->irqfds.lock); > + if (ret) > + return ret; > + irq_entry = tmp->irq_entry; > + return kvm_verify_msi(kvm, &irq_entry); How does this work on !arm64? Why do we need an on-stack version of tmp->irq_entry? > +} > + > static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, > int vector, int fd, bool msix) > { > @@ -355,6 +378,16 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, > return PTR_ERR(trigger); > } > > + if (!msix) { > + ret = vfio_pci_verify_msi_entry(vdev, trigger); > + if (ret) { > + kfree(vdev->ctx[vector].name); > + eventfd_ctx_put(trigger); > + if (ret > 0) > + ret = 0; > + return ret; > + } > + } Honestly, the whole things seems really complicated to avoid something that is only a harmless warning . How about just toning down the message instead? M. -- Without deviation from the norm, progress is not possible.