There are two ways of creating virtual functions: by using the sriov-pf device
parameter (this way, creating a user created VF and binding it to a PF), or
by using a device specific parameter, where the device manually creates a given
amount of VFs.
When a PCI device is realized, the device specific realize function is called
first, and then the pcie_sriov_register_device function is called, which checks
whether the device that has been created is a user created VF, and if it is the
case and the device allows this kind of VFs, it inserts it into a hashmap, with
the key being the ID of the PF, and the items being arrays of VFs.
User created VFs are instantiated independently, and later, when the PF calls
pcie_sriov_pf_init_from_user_created_vfs, it discovers its VFs from the hashmap
mentioned previously, and sets up in their PCIDevice.ex.sriov_pf structure
the pointer to the PF. This means that, during realization, VFs have no access
to the PF.
Device created VFs are instantiated during or after PF realization using the
pcie_sriov_pf_init function, which sets up for the VFs the pointer to the PF
before their realization, so when they're realized, they can access the PF
with no issues.
The problem here is that pcie_sriov_register_device is called after the
realization, and not before. This means that when a user created VF is created
for a device which does not support user created VFs, but supports device
created VFs, the realize function will notice that a VF is being created, and
may try to access the PF, causing a null pointer dereference fault.
Fix this by placing the user created VF check and registering, done by
pcie_sriov_register_device before realization. This way, incorrectly created
user created VFs will be noticed, and device creation will be aborted.
Additionally, create a new function pcie_sriov_check_after_realize, which
will be called after device realization, and its purpose is to check that
the VF or PF is in a consistent state. Currently, it checks that, if the device
is a PF, then it has initialized its list of VFs (this check was formerly done
by pcie_sriov_register_device), and in the case where the device is a VF,
it checks that the device is still a PCI Express device (some devices, e.g.
virtio-pci, clear this status during realization if the device is not in a
PCIe bus, or is located at the root complex).
Moreover, move the unregistering of a device's acpi-index and the removal
of a VF from its PF list from pci_qdev_unrealize to do_pci_unregister_device.
We do the former so that the acpi-index is unclaimed on user created VF
creation error, and the latter to not have extra code on the device specific
realization error path. We also gain from this that the acpi-index of a
device whose creation has errored will always be released, unlike until now,
where it is left stale if only do_pci_unregister_device were called instead
of pci_qdev_unrealize.
When instantiating a user created VF for a NVME controller (which is not
allowed):
Command line:
qemu-system-x86_64 -device nvme-subsys,id=subsys0 \
-device nvme,id=vctrl0,sriov-pf=ctrl0,subsys=subsys0 \
-device nvme,id=ctrl0,subsys=subsys0,serial=s
ASAN splat:
../hw/nvme/ctrl.c:9613:28: runtime error: member access within null pointer of
type 'struct NvmeCtrl'
AddressSanitizer:DEADLYSIGNAL
=================================================================
==91050==ERROR: AddressSanitizer: SEGV on unknown address 0x000000001cf0 (pc
0x7f79a8573dcd bp 0x7ffd622c1bc0 sp 0x7ffd622c1b68 T0)
==91050==The signal is caused by a READ memory access.
#0 0x7f79a8573dcd (/usr/lib/libc.so.6+0x173dcd)
#1 0x5644c6c0266e in nvme_realize ../hw/nvme/ctrl.c:9613
#2 0x5644c6c67213 in pci_qdev_realize ../hw/pci/pci.c:2316
#3 0x5644c7a2cfc1 in device_set_realized ../hw/core/qdev.c:514
#4 0x5644c7a4f767 in property_set_bool ../qom/object.c:2484
#5 0x5644c7a48d0b in object_property_set ../qom/object.c:1548
#6 0x5644c7a568a5 in object_property_set_qobject ../qom/qom-qobject.c:28
#7 0x5644c7a49385 in object_property_set_bool ../qom/object.c:1618
#8 0x5644c7a2aeb0 in qdev_realize ../hw/core/qdev.c:277
#9 0x5644c735f29f in qdev_device_add_from_qdict ../system/qdev-monitor.c:740
#10 0x5644c735f3ab in qdev_device_add ../system/qdev-monitor.c:758
#11 0x5644c72b1901 in device_init_func ../system/vl.c:1217
#12 0x5644c829d4a3 in qemu_opts_foreach ../util/qemu-option.c:1148
#13 0x5644c72bc33e in qemu_create_cli_devices ../system/vl.c:2762
#14 0x5644c72bcaa1 in qmp_x_exit_preconfig ../system/vl.c:2822
#15 0x5644c72c3241 in qemu_init ../system/vl.c:3862
#16 0x5644c801abf8 in main ../system/main.c:71
Cc: [email protected]
Fixes: 19e55471d4e8 ("pcie_sriov: Allow user to create SR-IOV device")
Signed-off-by: Daniel Paziyski <[email protected]>
---
Changes since v1
(https://lore.kernel.org/qemu-devel/[email protected]/):
- Clean up ASAN splat, no need to be so verbose.
- The return value of do_pci_register_device is checked, instead of being
assigned to pci_dev, this way avoiding dereferencing it when it is null
when cleaning up the acpi-index on device registration error.
- Create pcie_sriov_check_after_realize, for the reasons explained in the commit
description.
hw/pci/pci.c | 43 ++++++++++++++++++++++---------------
hw/pci/pcie_sriov.c | 25 +++++++++++++++------
include/hw/pci/pcie_sriov.h | 2 ++
3 files changed, 47 insertions(+), 23 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 0efb4eb4bb..e85b34d06d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1227,6 +1227,19 @@ static void pci_config_free(PCIDevice *pci_dev)
g_free(pci_dev->used);
}
+static void pci_unregister_acpi_index(PCIDevice *pci_dev)
+{
+ if (pci_dev->acpi_index) {
+ GSequence *used_indexes = pci_acpi_index_list();
+
+ g_sequence_remove(g_sequence_lookup(used_indexes,
+ GINT_TO_POINTER(pci_dev->acpi_index),
+ g_cmp_uint32, NULL));
+
+ pci_dev->acpi_index = 0;
+ }
+}
+
static void do_pci_unregister_device(PCIDevice *pci_dev)
{
pci_get_bus(pci_dev)->devices[pci_dev->devfn] = NULL;
@@ -1240,6 +1253,9 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
&pci_dev->bus_master_enable_region);
}
address_space_destroy(&pci_dev->bus_master_as);
+
+ pcie_sriov_unregister_device(pci_dev);
+ pci_unregister_acpi_index(pci_dev);
}
/* Extract PCIReqIDCache into BDF format */
@@ -1491,7 +1507,6 @@ static void pci_qdev_unrealize(DeviceState *dev)
pci_unregister_io_regions(pci_dev);
pci_del_option_rom(pci_dev);
- pcie_sriov_unregister_device(pci_dev);
if (pc->exit) {
pc->exit(pci_dev);
@@ -1501,17 +1516,6 @@ static void pci_qdev_unrealize(DeviceState *dev)
do_pci_unregister_device(pci_dev);
pci_dev->msi_trigger = NULL;
-
- /*
- * clean up acpi-index so it could reused by another device
- */
- if (pci_dev->acpi_index) {
- GSequence *used_indexes = pci_acpi_index_list();
-
- g_sequence_remove(g_sequence_lookup(used_indexes,
- GINT_TO_POINTER(pci_dev->acpi_index),
- g_cmp_uint32, NULL));
- }
}
void pci_register_bar(PCIDevice *pci_dev, int region_num,
@@ -2323,11 +2327,16 @@ static void pci_qdev_realize(DeviceState *qdev, Error
**errp)
pci_dev->cap_present |= QEMU_PCIE_CAP_CXL;
}
- pci_dev = do_pci_register_device(pci_dev,
- object_get_typename(OBJECT(qdev)),
- pci_dev->devfn, errp);
- if (pci_dev == NULL)
+ if (!do_pci_register_device(pci_dev, object_get_typename(OBJECT(qdev)),
+ pci_dev->devfn, errp)) {
+ pci_unregister_acpi_index(pci_dev);
return;
+ }
+
+ if (!pcie_sriov_register_device(pci_dev, errp)) {
+ do_pci_unregister_device(pci_dev);
+ return;
+ }
if (pc->realize) {
pc->realize(pci_dev, &local_err);
@@ -2338,7 +2347,7 @@ static void pci_qdev_realize(DeviceState *qdev, Error
**errp)
}
}
- if (!pcie_sriov_register_device(pci_dev, errp)) {
+ if (!pcie_sriov_check_after_realize(pci_dev, errp)) {
pci_qdev_unrealize(DEVICE(pci_dev));
return;
}
diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index c41ac95bee..23a2a43af7 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -357,12 +357,6 @@ int16_t pcie_sriov_pf_init_from_user_created_vfs(PCIDevice
*dev,
bool pcie_sriov_register_device(PCIDevice *dev, Error **errp)
{
- if (!dev->exp.sriov_pf.vf && dev->qdev.id &&
- pfs && g_hash_table_contains(pfs, dev->qdev.id)) {
- error_setg(errp, "attaching user-created SR-IOV VF unsupported");
- return false;
- }
-
if (dev->sriov_pf) {
PCIDevice *pci_pf;
GPtrArray *pf;
@@ -398,6 +392,25 @@ bool pcie_sriov_register_device(PCIDevice *dev, Error
**errp)
return true;
}
+bool pcie_sriov_check_after_realize(PCIDevice *dev, Error **errp)
+{
+ if (!dev->exp.sriov_pf.vf && dev->qdev.id &&
+ pfs && g_hash_table_contains(pfs, dev->qdev.id)) {
+ error_setg(errp, "attaching user-created SR-IOV VF unsupported");
+ return false;
+ }
+
+ if (dev->sriov_pf) {
+ /* some devices might clear their PCIe capability during realization */
+ if (!pci_is_express(dev)) {
+ error_setg(errp, "PCI Express is required for SR-IOV VF");
+ return false;
+ }
+ }
+
+ return true;
+}
+
void pcie_sriov_unregister_device(PCIDevice *dev)
{
if (dev->sriov_pf && pfs) {
diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
index b0ea6a62c7..00fe8c4ada 100644
--- a/include/hw/pci/pcie_sriov.h
+++ b/include/hw/pci/pcie_sriov.h
@@ -57,6 +57,8 @@ int16_t pcie_sriov_pf_init_from_user_created_vfs(PCIDevice
*dev,
bool pcie_sriov_register_device(PCIDevice *dev, Error **errp);
void pcie_sriov_unregister_device(PCIDevice *dev);
+bool pcie_sriov_check_after_realize(PCIDevice *dev, Error **errp);
+
/*
* Default (minimal) page size support values
* as required by the SR/IOV standard:
--
2.56.0
