From 8cf0449067e18bbb1e57da27399bcbd3b91a6505 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 9 Feb 2016 16:20:48 +0100 Subject: [PATCH] Apply pciback/pcifront patches fixing MSI-X enabled devices handling Fixes QubesOS/qubes-issues#1734 --- ...k-PF-instead-of-VF-for-PCI_COMMAND_M.patch | 46 +++++++ ...-the-number-of-MSI-X-entries-to-be-c.patch | 56 +++++++++ ...en-pcifront-Report-the-errors-better.patch | 54 ++++++++ ...-mysterious-crashes-when-NUMA-locali.patch | 117 ++++++++++++++++++ patches.xen/pci_op-cleanup.patch | 104 ++++++++++++++++ ...rn-error-on-XEN_PCI_OP_enable_msi-wh.patch | 61 +++++++++ ...rn-error-on-XEN_PCI_OP_enable_msix-w.patch | 63 ++++++++++ ...ot-install-an-IRQ-handler-for-MSI-in.patch | 81 ++++++++++++ ...XEN_PCI_OP_disable_msi-x-only-disabl.patch | 105 ++++++++++++++++ ...t-allow-MSI-X-ops-if-PCI_COMMAND_MEM.patch | 64 ++++++++++ series.conf | 14 +++ 11 files changed, 765 insertions(+) create mode 100644 patches.xen/0001-xen-pciback-Check-PF-instead-of-VF-for-PCI_COMMAND_M.patch create mode 100644 patches.xen/0002-xen-pciback-Save-the-number-of-MSI-X-entries-to-be-c.patch create mode 100644 patches.xen/0003-xen-pcifront-Report-the-errors-better.patch create mode 100644 patches.xen/0004-xen-pcifront-Fix-mysterious-crashes-when-NUMA-locali.patch create mode 100644 patches.xen/pci_op-cleanup.patch create mode 100644 patches.xen/xsa157-0001-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msi-wh.patch create mode 100644 patches.xen/xsa157-0002-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msix-w.patch create mode 100644 patches.xen/xsa157-0003-xen-pciback-Do-not-install-an-IRQ-handler-for-MSI-in.patch create mode 100644 patches.xen/xsa157-0004-xen-pciback-For-XEN_PCI_OP_disable_msi-x-only-disabl.patch create mode 100644 patches.xen/xsa157-0005-xen-pciback-Don-t-allow-MSI-X-ops-if-PCI_COMMAND_MEM.patch diff --git a/patches.xen/0001-xen-pciback-Check-PF-instead-of-VF-for-PCI_COMMAND_M.patch b/patches.xen/0001-xen-pciback-Check-PF-instead-of-VF-for-PCI_COMMAND_M.patch new file mode 100644 index 0000000..efbcd80 --- /dev/null +++ b/patches.xen/0001-xen-pciback-Check-PF-instead-of-VF-for-PCI_COMMAND_M.patch @@ -0,0 +1,46 @@ +From 9b8c92f9535ca9af672facd05360570730a33e05 Mon Sep 17 00:00:00 2001 +From: Konrad Rzeszutek Wilk +Date: Wed, 3 Feb 2016 10:18:18 -0500 +Subject: [PATCH 1/4] xen/pciback: Check PF instead of VF for + PCI_COMMAND_MEMORY + +c/s 408fb0e5aa7fda0059db282ff58c3b2a4278baa0 +"xen/pciback: Don't allow MSI-X ops if PCI_COMMAND_MEMORY is not set." +would check the device for PCI_COMMAND_MEMORY which is great. +Except that VF devices are unique - for example they have no +legacy interrupts, and also any writes to PCI_COMMAND_MEMORY +are silently ignored (by the hardware). + +CC: stable@vger.kernel.org +Signed-off-by: Konrad Rzeszutek Wilk +--- + drivers/xen/xen-pciback/pciback_ops.c | 5 ++++- + 1 file changed, 4 insertions(+), 1 deletion(-) + +diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c +index 73dafdc..8c86a53 100644 +--- a/drivers/xen/xen-pciback/pciback_ops.c ++++ b/drivers/xen/xen-pciback/pciback_ops.c +@@ -213,6 +213,7 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev, + int i, result; + struct msix_entry *entries; + u16 cmd; ++ struct pci_dev *phys_dev; + + if (unlikely(verbose_request)) + printk(KERN_DEBUG DRV_NAME ": %s: enable MSI-X\n", +@@ -227,8 +228,10 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev, + /* + * PCI_COMMAND_MEMORY must be enabled, otherwise we may not be able + * to access the BARs where the MSI-X entries reside. ++ * But VF devices are unique in which the PF needs to be checked. + */ +- pci_read_config_word(dev, PCI_COMMAND, &cmd); ++ phys_dev = pci_physfn(dev); ++ pci_read_config_word(phys_dev, PCI_COMMAND, &cmd); + if (dev->msi_enabled || !(cmd & PCI_COMMAND_MEMORY)) + return -ENXIO; + +-- +2.1.0 + diff --git a/patches.xen/0002-xen-pciback-Save-the-number-of-MSI-X-entries-to-be-c.patch b/patches.xen/0002-xen-pciback-Save-the-number-of-MSI-X-entries-to-be-c.patch new file mode 100644 index 0000000..72d3df8 --- /dev/null +++ b/patches.xen/0002-xen-pciback-Save-the-number-of-MSI-X-entries-to-be-c.patch @@ -0,0 +1,56 @@ +From 0f8901117800fc2f1a87cc5468f1ab7e4288cc5f Mon Sep 17 00:00:00 2001 +From: Konrad Rzeszutek Wilk +Date: Wed, 3 Feb 2016 10:22:02 -0500 +Subject: [PATCH 2/4] xen/pciback: Save the number of MSI-X entries to be + copied later. + +c/s 8135cf8b092723dbfcc611fe6fdcb3a36c9951c5 +"xen/pciback: Save xen_pci_op commands before processing it" +would copyback the processed values - which was great. + +However it missed the case that xen_pcibk_enable_msix - when +completing would overwrite op->value (which had the number +of MSI-X vectors requested) with the return value (which for +success was zero). Hence the copy-back routine (which would use +op->value) would copy exactly zero MSI-X vectors back. + +Signed-off-by: Konrad Rzeszutek Wilk +--- + drivers/xen/xen-pciback/pciback_ops.c | 7 +++++-- + 1 file changed, 5 insertions(+), 2 deletions(-) + +diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c +index 8c86a53..2fc5880 100644 +--- a/drivers/xen/xen-pciback/pciback_ops.c ++++ b/drivers/xen/xen-pciback/pciback_ops.c +@@ -335,7 +335,9 @@ void xen_pcibk_do_op(struct work_struct *data) + struct xen_pcibk_dev_data *dev_data = NULL; + struct xen_pci_op *op = &pdev->op; + int test_intx = 0; +- ++#ifdef CONFIG_PCI_MSI ++ unsigned int nr = 0; ++#endif + *op = pdev->sh_info->op; + barrier(); + dev = xen_pcibk_get_pci_dev(pdev, op->domain, op->bus, op->devfn); +@@ -363,6 +365,7 @@ void xen_pcibk_do_op(struct work_struct *data) + op->err = xen_pcibk_disable_msi(pdev, dev, op); + break; + case XEN_PCI_OP_enable_msix: ++ nr = op->value; + op->err = xen_pcibk_enable_msix(pdev, dev, op); + break; + case XEN_PCI_OP_disable_msix: +@@ -385,7 +388,7 @@ void xen_pcibk_do_op(struct work_struct *data) + if (op->cmd == XEN_PCI_OP_enable_msix && op->err == 0) { + unsigned int i; + +- for (i = 0; i < op->value; i++) ++ for (i = 0; i < nr; i++) + pdev->sh_info->op.msix_entries[i].vector = + op->msix_entries[i].vector; + } +-- +2.1.0 + diff --git a/patches.xen/0003-xen-pcifront-Report-the-errors-better.patch b/patches.xen/0003-xen-pcifront-Report-the-errors-better.patch new file mode 100644 index 0000000..7004adf --- /dev/null +++ b/patches.xen/0003-xen-pcifront-Report-the-errors-better.patch @@ -0,0 +1,54 @@ +From 3a0d57b60a61eb461504f8ed1845afd5084b7889 Mon Sep 17 00:00:00 2001 +From: Konrad Rzeszutek Wilk +Date: Wed, 3 Feb 2016 16:39:21 -0500 +Subject: [PATCH 3/4] xen/pcifront: Report the errors better. + +The messages should be different depending on the type of error. + +Signed-off-by: Konrad Rzeszutek Wilk +--- + arch/x86/include/asm/xen/pci.h | 4 ++-- + arch/x86/pci/xen.c | 5 ++++- + 2 files changed, 6 insertions(+), 3 deletions(-) + +diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h +index 968d57d..f320ee3 100644 +--- a/arch/x86/include/asm/xen/pci.h ++++ b/arch/x86/include/asm/xen/pci.h +@@ -57,7 +57,7 @@ static inline int xen_pci_frontend_enable_msi(struct pci_dev *dev, + { + if (xen_pci_frontend && xen_pci_frontend->enable_msi) + return xen_pci_frontend->enable_msi(dev, vectors); +- return -ENODEV; ++ return -ENOSYS; + } + static inline void xen_pci_frontend_disable_msi(struct pci_dev *dev) + { +@@ -69,7 +69,7 @@ static inline int xen_pci_frontend_enable_msix(struct pci_dev *dev, + { + if (xen_pci_frontend && xen_pci_frontend->enable_msix) + return xen_pci_frontend->enable_msix(dev, vectors, nvec); +- return -ENODEV; ++ return -ENOSYS; + } + static inline void xen_pci_frontend_disable_msix(struct pci_dev *dev) + { +diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c +index ff31ab4..beac4df 100644 +--- a/arch/x86/pci/xen.c ++++ b/arch/x86/pci/xen.c +@@ -196,7 +196,10 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) + return 0; + + error: +- dev_err(&dev->dev, "Xen PCI frontend has not registered MSI/MSI-X support!\n"); ++ if (ret == -ENOSYS) ++ dev_err(&dev->dev, "Xen PCI frontend has not registered MSI/MSI-X support!\n"); ++ else if (ret) ++ dev_err(&dev->dev, "Xen PCI frontend error: %d!\n", ret); + free: + kfree(v); + return ret; +-- +2.1.0 + diff --git a/patches.xen/0004-xen-pcifront-Fix-mysterious-crashes-when-NUMA-locali.patch b/patches.xen/0004-xen-pcifront-Fix-mysterious-crashes-when-NUMA-locali.patch new file mode 100644 index 0000000..606fb2e --- /dev/null +++ b/patches.xen/0004-xen-pcifront-Fix-mysterious-crashes-when-NUMA-locali.patch @@ -0,0 +1,117 @@ +From 25af39d59d2ff4a5e5bc872b8d4c451bbeffa312 Mon Sep 17 00:00:00 2001 +From: Konrad Rzeszutek Wilk +Date: Wed, 3 Feb 2016 16:43:56 -0500 +Subject: [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality + information was extracted. + +Occasionaly PV guests would crash with: + +pciback 0000:00:00.1: Xen PCI mapped GSI0 to IRQ16 +BUG: unable to handle kernel paging request at 0000000d1a8c0be0 +.. snip.. + ] find_next_bit+0xb/0x10 + [] cpumask_next_and+0x22/0x40 + [] pci_device_probe+0xb8/0x120 + [] ? driver_sysfs_add+0x77/0xa0 + [] driver_probe_device+0x1a4/0x2d0 + [] ? pci_match_device+0xdd/0x110 + [] __device_attach_driver+0xa7/0xb0 + [] ? __driver_attach+0xa0/0xa0 + [] bus_for_each_drv+0x62/0x90 + [] __device_attach+0xbd/0x110 + [] device_attach+0xb/0x10 + [] pci_bus_add_device+0x3c/0x70 + [] pci_bus_add_devices+0x38/0x80 + [] pcifront_scan_root+0x13e/0x1a0 + [] pcifront_backend_changed+0x262/0x60b + [] ? xenbus_gather+0xd6/0x160 + [] ? put_object+0x2f/0x50 + [] xenbus_otherend_changed+0x9d/0xa0 + [] backend_changed+0xe/0x10 + [] xenwatch_thread+0xc8/0x190 + [] ? woken_wake_function+0x10/0x10 + +which was the result of two things: + +When we call pci_scan_root_bus we would pass in 'sd' (sysdata) +pointer which was an 'pcifront_sd' structure. However in the +pci_device_add it expects that the 'sd' is 'struct sysdata' and +sets the dev->node to what is in sd->node (offset 4): + +set_dev_node(&dev->dev, pcibus_to_node(bus)); + + __pcibus_to_node(const struct pci_bus *bus) +{ + const struct pci_sysdata *sd = bus->sysdata; + + return sd->node; +} + +However our structure was pcifront_sd which had nothing at that +offset: + +struct pcifront_sd { + int domain; /* 0 4 */ + /* XXX 4 bytes hole, try to pack */ + struct pcifront_device * pdev; /* 8 8 */ +} + +That is an hole - filled with garbage as we used kmalloc instead of +kzalloc (the second problem). + +This patch fixes the issue by: + 1) Use kzalloc to initialize to a well known state. + 2) Put 'struct pci_sysdata' at the start of 'pcifront_sd'. That + way access to the 'node' will access the right offset. + +Signed-off-by: Konrad Rzeszutek Wilk +--- + drivers/pci/xen-pcifront.c | 11 +++++++---- + 1 file changed, 7 insertions(+), 4 deletions(-) + +diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c +index c777b97..66d197d 100644 +--- a/drivers/pci/xen-pcifront.c ++++ b/drivers/pci/xen-pcifront.c +@@ -53,7 +53,7 @@ struct pcifront_device { + }; + + struct pcifront_sd { +- int domain; ++ struct pci_sysdata sd; + struct pcifront_device *pdev; + }; + +@@ -67,7 +67,9 @@ static inline void pcifront_init_sd(struct pcifront_sd *sd, + unsigned int domain, unsigned int bus, + struct pcifront_device *pdev) + { +- sd->domain = domain; ++ /* Because we do not expose that information via XenBus. */ ++ sd->sd.node = first_online_node; ++ sd->sd.domain = domain; + sd->pdev = pdev; + } + +@@ -468,8 +470,8 @@ static int pcifront_scan_root(struct pcifront_device *pdev, + dev_info(&pdev->xdev->dev, "Creating PCI Frontend Bus %04x:%02x\n", + domain, bus); + +- bus_entry = kmalloc(sizeof(*bus_entry), GFP_KERNEL); +- sd = kmalloc(sizeof(*sd), GFP_KERNEL); ++ bus_entry = kzalloc(sizeof(*bus_entry), GFP_KERNEL); ++ sd = kzalloc(sizeof(*sd), GFP_KERNEL); + if (!bus_entry || !sd) { + err = -ENOMEM; + goto err_out; +@@ -576,6 +578,7 @@ static void pcifront_free_roots(struct pcifront_device *pdev) + free_root_bus_devs(bus_entry->bus); + + kfree(bus_entry->bus->sysdata); ++ bus_entry->bus->sysdata = NULL; + + device_unregister(bus_entry->bus->bridge); + pci_remove_bus(bus_entry->bus); +-- +2.1.0 + diff --git a/patches.xen/pci_op-cleanup.patch b/patches.xen/pci_op-cleanup.patch new file mode 100644 index 0000000..79be7f8 --- /dev/null +++ b/patches.xen/pci_op-cleanup.patch @@ -0,0 +1,104 @@ +From xen-devel-bounces@lists.xen.org Tue Feb 9 06:00:36 2016 +Date: Mon, 8 Feb 2016 23:59:27 -0500 +From: Konrad Rzeszutek Wilk +To: Marek =?iso-8859-1?Q?Marczykowski-G=F3recki?= + +Message-ID: <20160209045927.GC3853@localhost.localdomain> +References: + <20160127183005.GB3134@char.us.oracle.com> + + <1454323426.28781.73.camel@citrix.com> + <20160201145053.GA21826@char.us.oracle.com> + <20160203142230.GC24446@mail-itl> + <20160203152657.GE20732@char.us.oracle.com> + <20160208173917.GD24446@mail-itl> +MIME-Version: 1.0 +Content-Disposition: inline +In-Reply-To: <20160208173917.GD24446@mail-itl> +User-Agent: Mutt/1.5.24 (2015-08-30) +Cc: Tommi Airikka , Ian Campbell , + 810379@bugs.debian.org, xen-devel@lists.xen.org +Subject: Re: [Xen-devel] [BUG] pci-passthrough generates "xen:events: Failed + to obtain physical IRQ" for some devices + +I posted it at some point. It was that the MSI-X enable op stashes the +error value in op->value. But 'op->value' is an unsigned int so the +value ends up being 0xfffffe or such. And the other PV frontends only +check for !0 - and manufacture their own value (-EINVAL). + +Hence I want to update the pciff.h .. Oh here is the patch: +Oh man. A year?! + +Anyhow this can be posted as a cleanup patch seperately of the +bug-fixes. + +commit 393be47782bca7a24d3e365448d4d3d1a303abfe +Author: Konrad Rzeszutek Wilk +Date: Wed Apr 1 17:01:26 2015 -0400 + + xen/pcifront/pciback: Update pciif.h with ->err and ->result values. + + The '->err' should contain only the XEN_PCI_ERR_* type values. + The '->result' may contain -EXX values or any other value + that the XEN_PCI_OP_* deems appropiate. + + As such update the header and also the implementations. + + Signed-off-by: Konrad Rzeszutek Wilk + + Conflicts: + drivers/xen/xen-pciback/pciback_ops.c + + Conflicts: + drivers/xen/xen-pciback/pciback_ops.c + +diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c +index b1ffebe..353c8a2 100644 +--- a/drivers/pci/xen-pcifront.c ++++ b/drivers/pci/xen-pcifront.c +@@ -297,7 +297,7 @@ static int pci_frontend_enable_msix(struct pci_dev *dev, + } else { + dev_err(&dev->dev, "enable msix get err %x\n", err); + } +- return err; ++ return err ? -EINVAL : 0; + } + + static void pci_frontend_disable_msix(struct pci_dev *dev) +diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c +index fa2b222..4db6c19 100644 +--- a/drivers/xen/xen-pciback/pciback_ops.c ++++ b/drivers/xen/xen-pciback/pciback_ops.c +@@ -266,7 +266,7 @@ error: + if (dev_data) + dev_data->ack_intr = 0; + +- return result > 0 ? 0 : result; ++ return result >= 0 ? 0 : XEN_PCI_ERR_op_failed; + } + + static +diff --git a/include/xen/interface/io/pciif.h b/include/xen/interface/io/pciif.h +index d9922ae..c8b674f 100644 +--- a/include/xen/interface/io/pciif.h ++++ b/include/xen/interface/io/pciif.h +@@ -70,7 +70,7 @@ struct xen_pci_op { + /* IN: what action to perform: XEN_PCI_OP_* */ + uint32_t cmd; + +- /* OUT: will contain an error number (if any) from errno.h */ ++ /* OUT: will contain an XEN_PCI_ERR_* number. */ + int32_t err; + + /* IN: which device to touch */ +@@ -82,7 +82,9 @@ struct xen_pci_op { + int32_t offset; + int32_t size; + +- /* IN/OUT: Contains the result after a READ or the value to WRITE */ ++ /* IN/OUT: Contains the result after a READ or the value to WRITE. ++ * If the err does not have XEN_PCI_ERR_success, depending on ++ * XEN_PCI_OP_* might have the errno value. */ + uint32_t value; + /* IN: Contains extra infor for this operation */ + uint32_t info; diff --git a/patches.xen/xsa157-0001-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msi-wh.patch b/patches.xen/xsa157-0001-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msi-wh.patch new file mode 100644 index 0000000..03802fe --- /dev/null +++ b/patches.xen/xsa157-0001-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msi-wh.patch @@ -0,0 +1,61 @@ +From e3de4a44cfe196e162ddeffd6379e5c4e75ff1d7 Mon Sep 17 00:00:00 2001 +From: Konrad Rzeszutek Wilk +Date: Fri, 3 Apr 2015 11:08:22 -0400 +Subject: [PATCH v2 XSA157 1/5] xen/pciback: Return error on + XEN_PCI_OP_enable_msi when device has MSI or MSI-X enabled + +The guest sequence of: + + a) XEN_PCI_OP_enable_msi + b) XEN_PCI_OP_enable_msi + c) XEN_PCI_OP_disable_msi + +results in hitting an BUG_ON condition in the msi.c code. + +The MSI code uses an dev->msi_list to which it adds MSI entries. +Under the above conditions an BUG_ON() can be hit. The device +passed in the guest MUST have MSI capability. + +The a) adds the entry to the dev->msi_list and sets msi_enabled. +The b) adds a second entry but adding in to SysFS fails (duplicate entry) +and deletes all of the entries from msi_list and returns (with msi_enabled +is still set). c) pci_disable_msi passes the msi_enabled checks and hits: + +BUG_ON(list_empty(dev_to_msi_list(&dev->dev))); + +and blows up. + +The patch adds a simple check in the XEN_PCI_OP_enable_msi to guard +against that. The check for msix_enabled is not stricly neccessary. + +This is part of XSA-157. + +CC: stable@vger.kernel.org +Reviewed-by: David Vrabel +Reviewed-by: Jan Beulich +Signed-off-by: Konrad Rzeszutek Wilk +--- + drivers/xen/xen-pciback/pciback_ops.c | 7 ++++++- + 1 file changed, 6 insertions(+), 1 deletion(-) + +diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c +index c4a0666..5ce573a 100644 +--- a/drivers/xen/xen-pciback/pciback_ops.c ++++ b/drivers/xen/xen-pciback/pciback_ops.c +@@ -144,7 +144,12 @@ int xen_pcibk_enable_msi(struct xen_pcibk_device *pdev, + if (unlikely(verbose_request)) + printk(KERN_DEBUG DRV_NAME ": %s: enable MSI\n", pci_name(dev)); + +- status = pci_enable_msi(dev); ++ if (dev->msi_enabled) ++ status = -EALREADY; ++ else if (dev->msix_enabled) ++ status = -ENXIO; ++ else ++ status = pci_enable_msi(dev); + + if (status) { + pr_warn_ratelimited("%s: error enabling MSI for guest %u: err %d\n", +-- +2.1.0 + diff --git a/patches.xen/xsa157-0002-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msix-w.patch b/patches.xen/xsa157-0002-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msix-w.patch new file mode 100644 index 0000000..c35629d --- /dev/null +++ b/patches.xen/xsa157-0002-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msix-w.patch @@ -0,0 +1,63 @@ +From 19b33b70d423ddfea1daf7615eb7f605371a1841 Mon Sep 17 00:00:00 2001 +From: Konrad Rzeszutek Wilk +Date: Mon, 2 Nov 2015 18:07:44 -0500 +Subject: [PATCH v2 XSA157 2/5] xen/pciback: Return error on + XEN_PCI_OP_enable_msix when device has MSI or MSI-X enabled + +The guest sequence of: + + a) XEN_PCI_OP_enable_msix + b) XEN_PCI_OP_enable_msix + +results in hitting an NULL pointer due to using freed pointers. + +The device passed in the guest MUST have MSI-X capability. + +The a) constructs and SysFS representation of MSI and MSI groups. +The b) adds a second set of them but adding in to SysFS fails (duplicate entry). +'populate_msi_sysfs' frees the newly allocated msi_irq_groups (note that +in a) pdev->msi_irq_groups is still set) and also free's ALL of the +MSI-X entries of the device (the ones allocated in step a) and b)). + +The unwind code: 'free_msi_irqs' deletes all the entries and tries to +delete the pdev->msi_irq_groups (which hasn't been set to NULL). +However the pointers in the SysFS are already freed and we hit an +NULL pointer further on when 'strlen' is attempted on a freed pointer. + +The patch adds a simple check in the XEN_PCI_OP_enable_msix to guard +against that. The check for msi_enabled is not stricly neccessary. + +This is part of XSA-157 + +CC: stable@vger.kernel.org +Reviewed-by: David Vrabel +Reviewed-by: Jan Beulich +Signed-off-by: Konrad Rzeszutek Wilk +--- + drivers/xen/xen-pciback/pciback_ops.c | 7 +++++++ + 1 file changed, 7 insertions(+) + +diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c +index 5ce573a..a107928 100644 +--- a/drivers/xen/xen-pciback/pciback_ops.c ++++ b/drivers/xen/xen-pciback/pciback_ops.c +@@ -206,9 +206,16 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev, + if (unlikely(verbose_request)) + printk(KERN_DEBUG DRV_NAME ": %s: enable MSI-X\n", + pci_name(dev)); ++ + if (op->value > SH_INFO_MAX_VEC) + return -EINVAL; + ++ if (dev->msix_enabled) ++ return -EALREADY; ++ ++ if (dev->msi_enabled) ++ return -ENXIO; ++ + entries = kmalloc(op->value * sizeof(*entries), GFP_KERNEL); + if (entries == NULL) + return -ENOMEM; +-- +2.1.0 + diff --git a/patches.xen/xsa157-0003-xen-pciback-Do-not-install-an-IRQ-handler-for-MSI-in.patch b/patches.xen/xsa157-0003-xen-pciback-Do-not-install-an-IRQ-handler-for-MSI-in.patch new file mode 100644 index 0000000..c91f903 --- /dev/null +++ b/patches.xen/xsa157-0003-xen-pciback-Do-not-install-an-IRQ-handler-for-MSI-in.patch @@ -0,0 +1,81 @@ +From aa48314c60da1035a8e6cc05bec12838a074de98 Mon Sep 17 00:00:00 2001 +From: Konrad Rzeszutek Wilk +Date: Mon, 2 Nov 2015 17:24:08 -0500 +Subject: [PATCH v2 XSA157 3/5] xen/pciback: Do not install an IRQ handler for + MSI interrupts. + +Otherwise an guest can subvert the generic MSI code to trigger +an BUG_ON condition during MSI interrupt freeing: + + for (i = 0; i < entry->nvec_used; i++) + BUG_ON(irq_has_action(entry->irq + i)); + +Xen PCI backed installs an IRQ handler (request_irq) for +the dev->irq whenever the guest writes PCI_COMMAND_MEMORY +(or PCI_COMMAND_IO) to the PCI_COMMAND register. This is +done in case the device has legacy interrupts the GSI line +is shared by the backend devices. + +To subvert the backend the guest needs to make the backend +to change the dev->irq from the GSI to the MSI interrupt line, +make the backend allocate an interrupt handler, and then command +the backend to free the MSI interrupt and hit the BUG_ON. + +Since the backend only calls 'request_irq' when the guest +writes to the PCI_COMMAND register the guest needs to call +XEN_PCI_OP_enable_msi before any other operation. This will +cause the generic MSI code to setup an MSI entry and +populate dev->irq with the new PIRQ value. + +Then the guest can write to PCI_COMMAND PCI_COMMAND_MEMORY +and cause the backend to setup an IRQ handler for dev->irq +(which instead of the GSI value has the MSI pirq). See +'xen_pcibk_control_isr'. + +Then the guest disables the MSI: XEN_PCI_OP_disable_msi +which ends up triggering the BUG_ON condition in 'free_msi_irqs' +as there is an IRQ handler for the entry->irq (dev->irq). + +Note that this cannot be done using MSI-X as the generic +code does not over-write dev->irq with the MSI-X PIRQ values. + +The patch inhibits setting up the IRQ handler if MSI or +MSI-X (for symmetry reasons) code had been called successfully. + +P.S. +Xen PCIBack when it sets up the device for the guest consumption +ends up writting 0 to the PCI_COMMAND (see xen_pcibk_reset_device). +XSA-120 addendum patch removed that - however when upstreaming said +addendum we found that it caused issues with qemu upstream. That +has now been fixed in qemu upstream. + +This is part of XSA-157 + +CC: stable@vger.kernel.org +Reviewed-by: David Vrabel +Signed-off-by: Konrad Rzeszutek Wilk +--- + drivers/xen/xen-pciback/pciback_ops.c | 7 +++++++ + 1 file changed, 7 insertions(+) + +diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c +index a107928..5bb76c0 100644 +--- a/drivers/xen/xen-pciback/pciback_ops.c ++++ b/drivers/xen/xen-pciback/pciback_ops.c +@@ -70,6 +70,13 @@ static void xen_pcibk_control_isr(struct pci_dev *dev, int reset) + enable ? "enable" : "disable"); + + if (enable) { ++ /* ++ * The MSI or MSI-X should not have an IRQ handler. Otherwise ++ * if the guest terminates we BUG_ON in free_msi_irqs. ++ */ ++ if (dev->msi_enabled || dev->msix_enabled) ++ goto out; ++ + rc = request_irq(dev_data->irq, + xen_pcibk_guest_interrupt, IRQF_SHARED, + dev_data->irq_name, dev); +-- +2.1.0 + diff --git a/patches.xen/xsa157-0004-xen-pciback-For-XEN_PCI_OP_disable_msi-x-only-disabl.patch b/patches.xen/xsa157-0004-xen-pciback-For-XEN_PCI_OP_disable_msi-x-only-disabl.patch new file mode 100644 index 0000000..a0d75bf --- /dev/null +++ b/patches.xen/xsa157-0004-xen-pciback-For-XEN_PCI_OP_disable_msi-x-only-disabl.patch @@ -0,0 +1,105 @@ +From 59a403750d3796b45376041a4843fcde436ae37e Mon Sep 17 00:00:00 2001 +From: Konrad Rzeszutek Wilk +Date: Wed, 1 Apr 2015 10:49:47 -0400 +Subject: [PATCH v2 XSA157 4/5] xen/pciback: For XEN_PCI_OP_disable_msi[|x] + only disable if device has MSI(X) enabled. + +Otherwise just continue on, returning the same values as +previously (return of 0, and op->result has the PIRQ value). + +This does not change the behavior of XEN_PCI_OP_disable_msi[|x]. + +The pci_disable_msi or pci_disable_msix have the checks for +msi_enabled or msix_enabled so they will error out immediately. + +However the guest can still call these operations and cause +us to disable the 'ack_intr'. That means the backend IRQ handler +for the legacy interrupt will not respond to interrupts anymore. + +This will lead to (if the device is causing an interrupt storm) +for the Linux generic code to disable the interrupt line. + +Naturally this will only happen if the device in question +is plugged in on the motherboard on shared level interrupt GSI. + +This is part of XSA-157 + +CC: stable@vger.kernel.org +Reviewed-by: David Vrabel +Signed-off-by: Konrad Rzeszutek Wilk +--- + drivers/xen/xen-pciback/pciback_ops.c | 33 ++++++++++++++++++++------------- + 1 file changed, 20 insertions(+), 13 deletions(-) + +diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c +index 5bb76c0..648c09c 100644 +--- a/drivers/xen/xen-pciback/pciback_ops.c ++++ b/drivers/xen/xen-pciback/pciback_ops.c +@@ -185,20 +185,23 @@ static + int xen_pcibk_disable_msi(struct xen_pcibk_device *pdev, + struct pci_dev *dev, struct xen_pci_op *op) + { +- struct xen_pcibk_dev_data *dev_data; +- + if (unlikely(verbose_request)) + printk(KERN_DEBUG DRV_NAME ": %s: disable MSI\n", + pci_name(dev)); +- pci_disable_msi(dev); + ++ if (dev->msi_enabled) { ++ struct xen_pcibk_dev_data *dev_data; ++ ++ pci_disable_msi(dev); ++ ++ dev_data = pci_get_drvdata(dev); ++ if (dev_data) ++ dev_data->ack_intr = 1; ++ } + op->value = dev->irq ? xen_pirq_from_irq(dev->irq) : 0; + if (unlikely(verbose_request)) + printk(KERN_DEBUG DRV_NAME ": %s: MSI: %d\n", pci_name(dev), + op->value); +- dev_data = pci_get_drvdata(dev); +- if (dev_data) +- dev_data->ack_intr = 1; + return 0; + } + +@@ -264,23 +267,27 @@ static + int xen_pcibk_disable_msix(struct xen_pcibk_device *pdev, + struct pci_dev *dev, struct xen_pci_op *op) + { +- struct xen_pcibk_dev_data *dev_data; + if (unlikely(verbose_request)) + printk(KERN_DEBUG DRV_NAME ": %s: disable MSI-X\n", + pci_name(dev)); +- pci_disable_msix(dev); + ++ if (dev->msix_enabled) { ++ struct xen_pcibk_dev_data *dev_data; ++ ++ pci_disable_msix(dev); ++ ++ dev_data = pci_get_drvdata(dev); ++ if (dev_data) ++ dev_data->ack_intr = 1; ++ } + /* + * SR-IOV devices (which don't have any legacy IRQ) have + * an undefined IRQ value of zero. + */ + op->value = dev->irq ? xen_pirq_from_irq(dev->irq) : 0; + if (unlikely(verbose_request)) +- printk(KERN_DEBUG DRV_NAME ": %s: MSI-X: %d\n", pci_name(dev), +- op->value); +- dev_data = pci_get_drvdata(dev); +- if (dev_data) +- dev_data->ack_intr = 1; ++ printk(KERN_DEBUG DRV_NAME ": %s: MSI-X: %d\n", ++ pci_name(dev), op->value); + return 0; + } + #endif +-- +2.1.0 + diff --git a/patches.xen/xsa157-0005-xen-pciback-Don-t-allow-MSI-X-ops-if-PCI_COMMAND_MEM.patch b/patches.xen/xsa157-0005-xen-pciback-Don-t-allow-MSI-X-ops-if-PCI_COMMAND_MEM.patch new file mode 100644 index 0000000..85f0654 --- /dev/null +++ b/patches.xen/xsa157-0005-xen-pciback-Don-t-allow-MSI-X-ops-if-PCI_COMMAND_MEM.patch @@ -0,0 +1,64 @@ +From e9ab9e04ed76ef06b4ba9a30b3724ca563fdf1fa Mon Sep 17 00:00:00 2001 +From: Konrad Rzeszutek Wilk +Date: Mon, 2 Nov 2015 18:13:27 -0500 +Subject: [PATCH v2 XSA157 5/5] xen/pciback: Don't allow MSI-X ops if + PCI_COMMAND_MEMORY is not set. + +commit f598282f51 ("PCI: Fix the NIU MSI-X problem in a better way") +teaches us that dealing with MSI-X can be troublesome. + +Further checks in the MSI-X architecture shows that if the +PCI_COMMAND_MEMORY bit is turned of in the PCI_COMMAND we +may not be able to access the BAR (since they are memory regions). + +Since the MSI-X tables are located in there.. that can lead +to us causing PCIe errors. Inhibit us performing any +operation on the MSI-X unless the MEMORY bit is set. + +Note that Xen hypervisor with: +"x86/MSI-X: access MSI-X table only after having enabled MSI-X" +will return: +xen_pciback: 0000:0a:00.1: error -6 enabling MSI-X for guest 3! + +When the generic MSI code tries to setup the PIRQ without +MEMORY bit set. Which means with later versions of Xen +(4.6) this patch is not neccessary. + +This is part of XSA-157 + +CC: stable@vger.kernel.org +Reviewed-by: Jan Beulich +Signed-off-by: Konrad Rzeszutek Wilk +--- + drivers/xen/xen-pciback/pciback_ops.c | 8 +++++++- + 1 file changed, 7 insertions(+), 1 deletion(-) + +diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c +index 648c09c..edb9357 100644 +--- a/drivers/xen/xen-pciback/pciback_ops.c ++++ b/drivers/xen/xen-pciback/pciback_ops.c +@@ -212,6 +212,7 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev, + struct xen_pcibk_dev_data *dev_data; + int i, result; + struct msix_entry *entries; ++ u16 cmd; + + if (unlikely(verbose_request)) + printk(KERN_DEBUG DRV_NAME ": %s: enable MSI-X\n", +@@ -223,7 +224,12 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev, + if (dev->msix_enabled) + return -EALREADY; + +- if (dev->msi_enabled) ++ /* ++ * PCI_COMMAND_MEMORY must be enabled, otherwise we may not be able ++ * to access the BARs where the MSI-X entries reside. ++ */ ++ pci_read_config_word(dev, PCI_COMMAND, &cmd); ++ if (dev->msi_enabled || !(cmd & PCI_COMMAND_MEMORY)) + return -ENXIO; + + entries = kmalloc(op->value * sizeof(*entries), GFP_KERNEL); +-- +2.1.0 + diff --git a/series.conf b/series.conf index 99c7e61..0ee5b36 100644 --- a/series.conf +++ b/series.conf @@ -25,3 +25,17 @@ patches.xen/xsa155-linux41-0010-xen-netfront-do-not-use-data-already-exposed-to- patches.xen/xsa155-linux-0011-xen-netfront-add-range-check-for-Tx-response-id.patch patches.xen/xsa155-linux312-0012-xen-blkfront-make-local-copy-of-response-before-usin.patch patches.xen/xsa155-linux319-0013-xen-blkfront-prepare-request-locally-only-then-put-i.patch +patches.xen/xsa157-0001-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msi-wh.patch +patches.xen/xsa157-0002-xen-pciback-Return-error-on-XEN_PCI_OP_enable_msix-w.patch +patches.xen/xsa157-0003-xen-pciback-Do-not-install-an-IRQ-handler-for-MSI-in.patch +patches.xen/xsa157-0004-xen-pciback-For-XEN_PCI_OP_disable_msi-x-only-disabl.patch +patches.xen/xsa157-0005-xen-pciback-Don-t-allow-MSI-X-ops-if-PCI_COMMAND_MEM.patch + +# MSI-X enabled device passthrough fix (#1734) +patches.xen/0001-xen-pciback-Check-PF-instead-of-VF-for-PCI_COMMAND_M.patch +patches.xen/0002-xen-pciback-Save-the-number-of-MSI-X-entries-to-be-c.patch +patches.xen/0003-xen-pcifront-Report-the-errors-better.patch +patches.xen/0004-xen-pcifront-Fix-mysterious-crashes-when-NUMA-locali.patch +patches.xen/pci_op-cleanup.patch + +