Message ID | 20250106215231.2104123-1-naravamudan@nvidia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | pci: account for sysfs-disabled reset in pci_{slot,bus}_resettable | expand |
On Mon, Jan 06, 2025 at 03:52:31PM -0600, Nishanth Aravamudan wrote: > vfio_pci_ioctl_get_pci_hot_reset_info checks if either the vdev's slot > or bus is not resettable by calling pci_probe_reset_{slot,bus}. Those > functions in turn call pci_{slot,bus}_resettable() to see if the PCI > device supports reset. This change makes sense to me, but.. > However, commit d88f521da3ef ("PCI: Allow userspace to query and set > device reset mechanism") added support for userspace to disable reset of > specific PCI devices (by echo'ing "" into reset_method) and > pci_{slot,bus}_resettable methods do not check pci_reset_supported() to > see if userspace has disabled reset. Therefore, if an administrator > disables PCI reset of a specific device, but then uses vfio-pci with > that device (e.g. with qemu), vfio-pci will happily end up issuing a > reset to that device. How does vfio-pci endup issuing a reset? It looked like all the paths are blocked in the pci core with pci_reset_supported()? Is there also a path that vfio is calling that is missing a pci_reset_supported() check? If yes that should probably be fixed in another patch. Or do you mean that VFIO tries to do a reset but it fails and nothing happens, the real issue is that hot_reset_info is reporting incorrect information to userspace? Jason
Please update subject line to match historical capitalization convention. On Mon, Jan 06, 2025 at 03:52:31PM -0600, Nishanth Aravamudan wrote: > vfio_pci_ioctl_get_pci_hot_reset_info checks if either the vdev's slot > or bus is not resettable by calling pci_probe_reset_{slot,bus}. Those > functions in turn call pci_{slot,bus}_resettable() to see if the PCI > device supports reset. > > However, commit d88f521da3ef ("PCI: Allow userspace to query and set > device reset mechanism") added support for userspace to disable reset of > specific PCI devices (by echo'ing "" into reset_method) and > pci_{slot,bus}_resettable methods do not check pci_reset_supported() to > see if userspace has disabled reset. Therefore, if an administrator > disables PCI reset of a specific device, but then uses vfio-pci with > that device (e.g. with qemu), vfio-pci will happily end up issuing a > reset to that device. Please consistently add "()" after function names. > Add an explicit check of pci_reset_supported() in both paths. Bjorn
On Mon, Jan 13, 2025 at 04:42:00PM -0400, Jason Gunthorpe wrote: > On Mon, Jan 06, 2025 at 03:52:31PM -0600, Nishanth Aravamudan wrote: > > vfio_pci_ioctl_get_pci_hot_reset_info checks if either the vdev's slot > > or bus is not resettable by calling pci_probe_reset_{slot,bus}. Those > > functions in turn call pci_{slot,bus}_resettable() to see if the PCI > > device supports reset. > > This change makes sense to me, but.. > > > However, commit d88f521da3ef ("PCI: Allow userspace to query and set > > device reset mechanism") added support for userspace to disable reset of > > specific PCI devices (by echo'ing "" into reset_method) and > > pci_{slot,bus}_resettable methods do not check pci_reset_supported() to > > see if userspace has disabled reset. Therefore, if an administrator > > disables PCI reset of a specific device, but then uses vfio-pci with > > that device (e.g. with qemu), vfio-pci will happily end up issuing a > > reset to that device. > > How does vfio-pci endup issuing a reset? It looked like all the paths > are blocked in the pci core with pci_reset_supported()? Is there also > a path that vfio is calling that is missing a pci_reset_supported() > check? If yes that should probably be fixed in another patch. This is the path I observed: drivers/vfio/vfio_pci_core::vfio_pci_ioctl_get_pci_hot_reset_info() indicates a reset is possible if either -> drivers/pci/pci.c::pci_probe_reset_slot() || -> drivers/pci/pci.c::pci_probe_reset_bus() returns 0 drivers/pci/pci.c::pci_probe_reset_slot() -> pci_slot_reset(..., PCI_RESET_PROBE) -> pci_slot_resettable() drivers/pci/pci.c::pci_probe_reset_bus() -> pci_bus_reset(..., PCI_RESET_PROBE) -> pci_bus_resettable() Both pci_{slot,bus}_resettable() before my change returned true even if the sysfs files indicated resets were disabled. Separate from this path, e.g., a poorly-behaving userspace that ignores or does not execute the VFIO_DEVICE_GET_PCI_HOT_RESET_INFO ioctl before issuing a VFIO_DEVICE_PCI_HOT_RESET ioctl, actual resets check the same return values: drivers/vfio/vfio_pci_core::vfio_pci_ioctl_pci_hot_reset() indicates a reset is possible if either -> drivers/pci/pci.c::pci_probe_reset_slot() || -> drivers/pci/pci.c::pci_probe_reset_bus() returns 0 and will then issue a reset to the device via either -> vfio_pci_ioctl_pci_hot_reset_groups() || -> vfio_pci_dev_set_hot_reset() Thanks, Nish
On Mon, Jan 13, 2025 at 05:25:40PM -0600, Bjorn Helgaas wrote: > Please update subject line to match historical capitalization > convention. Thank you, I will fix in v2. > > On Mon, Jan 06, 2025 at 03:52:31PM -0600, Nishanth Aravamudan wrote: > > vfio_pci_ioctl_get_pci_hot_reset_info checks if either the vdev's slot > > or bus is not resettable by calling pci_probe_reset_{slot,bus}. Those > > functions in turn call pci_{slot,bus}_resettable() to see if the PCI > > device supports reset. > > > > However, commit d88f521da3ef ("PCI: Allow userspace to query and set > > device reset mechanism") added support for userspace to disable reset of > > specific PCI devices (by echo'ing "" into reset_method) and > > pci_{slot,bus}_resettable methods do not check pci_reset_supported() to > > see if userspace has disabled reset. Therefore, if an administrator > > disables PCI reset of a specific device, but then uses vfio-pci with > > that device (e.g. with qemu), vfio-pci will happily end up issuing a > > reset to that device. > > Please consistently add "()" after function names. Will fix in v2, as well. -Nish
On Wed, Jan 22, 2025 at 12:14:02PM -0600, Nishanth Aravamudan wrote: > On Mon, Jan 13, 2025 at 04:42:00PM -0400, Jason Gunthorpe wrote: > > On Mon, Jan 06, 2025 at 03:52:31PM -0600, Nishanth Aravamudan wrote: > > > vfio_pci_ioctl_get_pci_hot_reset_info checks if either the vdev's slot > > > or bus is not resettable by calling pci_probe_reset_{slot,bus}. Those > > > functions in turn call pci_{slot,bus}_resettable() to see if the PCI > > > device supports reset. > > > > This change makes sense to me, but.. > > > > > However, commit d88f521da3ef ("PCI: Allow userspace to query and set > > > device reset mechanism") added support for userspace to disable reset of > > > specific PCI devices (by echo'ing "" into reset_method) and > > > pci_{slot,bus}_resettable methods do not check pci_reset_supported() to > > > see if userspace has disabled reset. Therefore, if an administrator > > > disables PCI reset of a specific device, but then uses vfio-pci with > > > that device (e.g. with qemu), vfio-pci will happily end up issuing a > > > reset to that device. > > > > How does vfio-pci endup issuing a reset? It looked like all the paths > > are blocked in the pci core with pci_reset_supported()? Is there also > > a path that vfio is calling that is missing a pci_reset_supported() > > check? If yes that should probably be fixed in another patch. > > This is the path I observed: You didn't answer the question, I didn't ask about pci_probe_*() I asked why doesn't pci_reset_supported() directly block the actual reset? Should we be adding: @@ -5919,6 +5919,9 @@ int __pci_reset_bus(struct pci_bus *bus) */ int pci_reset_bus(struct pci_dev *pdev) { + if (!pci_reset_supported(pdev)) + return -EOPNOTSUPP; + return (!pci_probe_reset_slot(pdev->slot)) ? __pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus); And maybe more? Jason
On Thu, Jan 23, 2025 at 09:33:12AM -0400, Jason Gunthorpe wrote: > On Wed, Jan 22, 2025 at 12:14:02PM -0600, Nishanth Aravamudan wrote: > > On Mon, Jan 13, 2025 at 04:42:00PM -0400, Jason Gunthorpe wrote: > > > On Mon, Jan 06, 2025 at 03:52:31PM -0600, Nishanth Aravamudan wrote: > > > > vfio_pci_ioctl_get_pci_hot_reset_info checks if either the vdev's slot > > > > or bus is not resettable by calling pci_probe_reset_{slot,bus}. Those > > > > functions in turn call pci_{slot,bus}_resettable() to see if the PCI > > > > device supports reset. > > > > > > This change makes sense to me, but.. > > > > > > > However, commit d88f521da3ef ("PCI: Allow userspace to query and set > > > > device reset mechanism") added support for userspace to disable reset of > > > > specific PCI devices (by echo'ing "" into reset_method) and > > > > pci_{slot,bus}_resettable methods do not check pci_reset_supported() to > > > > see if userspace has disabled reset. Therefore, if an administrator > > > > disables PCI reset of a specific device, but then uses vfio-pci with > > > > that device (e.g. with qemu), vfio-pci will happily end up issuing a > > > > reset to that device. > > > > > > How does vfio-pci endup issuing a reset? It looked like all the paths > > > are blocked in the pci core with pci_reset_supported()? Is there also > > > a path that vfio is calling that is missing a pci_reset_supported() > > > check? If yes that should probably be fixed in another patch. > > > > This is the path I observed: > > You didn't answer the question, I didn't ask about pci_probe_*() I > asked why doesn't pci_reset_supported() directly block the actual > reset? Sorry, I misunderstood your question. __pci_reset_bus() -> pci_bus_reset(..., PCI_RESET_PROBE) -> pci_bus_resettable() __pci_reset_slot() -> pci_slot_reset(..., PCI_RESET_PROBE) -> pci_slot_resettable() pci_reset_bus() -> pci_probe_reset_slot() -> pci_slot_reset(..., PCI_RESET_PROBE) -> pci_bus_resettable() if true: __pci_reset_slot() else: __pci_reset_bus() Before my change, both call paths would end up calling pci_slot_resettable() and not checking the sysfs file-contents. Please let me know if that addresses your concern, I think my changes fixes the paths you are talking about as well. If I need to clarify this in the commit message, I can. -Nish
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 661f98c6c63a..809936e1c3b7 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5536,6 +5536,8 @@ static bool pci_bus_resettable(struct pci_bus *bus) return false; list_for_each_entry(dev, &bus->devices, bus_list) { + if (!pci_reset_supported(dev)) + return false; if (dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET || (dev->subordinate && !pci_bus_resettable(dev->subordinate))) return false; @@ -5612,6 +5614,8 @@ static bool pci_slot_resettable(struct pci_slot *slot) list_for_each_entry(dev, &slot->bus->devices, bus_list) { if (!dev->slot || dev->slot != slot) continue; + if (!pci_reset_supported(dev)) + return false; if (dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET || (dev->subordinate && !pci_bus_resettable(dev->subordinate))) return false;
vfio_pci_ioctl_get_pci_hot_reset_info checks if either the vdev's slot or bus is not resettable by calling pci_probe_reset_{slot,bus}. Those functions in turn call pci_{slot,bus}_resettable() to see if the PCI device supports reset. However, commit d88f521da3ef ("PCI: Allow userspace to query and set device reset mechanism") added support for userspace to disable reset of specific PCI devices (by echo'ing "" into reset_method) and pci_{slot,bus}_resettable methods do not check pci_reset_supported() to see if userspace has disabled reset. Therefore, if an administrator disables PCI reset of a specific device, but then uses vfio-pci with that device (e.g. with qemu), vfio-pci will happily end up issuing a reset to that device. Add an explicit check of pci_reset_supported() in both paths. Fixes: d88f521da3ef ("PCI: Allow userspace to query and set device reset mechanism") Signed-off-by: Nishanth Aravamudan <naravamudan@nvidia.com> Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com> Cc: Amey Narkhede <ameynarkhede03@gmail.com> Cc: linux-pci@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: Jason Gunthorpe <jgg@nvidia.com> Cc: Yishai Hadas <yishaih@nvidia.com> Cc: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> Cc: Kevin Tian <kevin.tian@intel.com> Cc: kvm@vger.kernel.org --- drivers/pci/pci.c | 4 ++++ 1 file changed, 4 insertions(+)