Re: [PATCH] pci: account for sysfs-disabled reset in pci_{slot,bus}_resettable

[Nishanth Aravamudan <naravamudan@xxxxxxxxxx>]

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