Re: [PATCH v18 3/4] vfio/pci: Add a reset_done callback for vfio-pci driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The `reset_done` callback breaks standard device resets by irreversibly overwriting the runtime PCI configuration space.
- [High] Concurrent execution of `vfio_pci_core_aer_reset_done` and `vfio_pci_core_disable` leads to a Use-After-Free on `vdev->pci_saved_state`.
- [High] The `reset_done` callback is never invoked during AER error recovery.
--

commit ea3a78b14c5b561020bd16d25cbb09f0536e6e7a
Author: Farhan Ali <alifm@linux.ibm.com>

vfio/pci: Add a reset_done callback for vfio-pci driver

On error recovery for a PCI device bound to vfio-pci driver, we want to
recover the state of the device to its last known saved state. The callback
restores the state of the device to its initial saved state.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 27642f10fe973..95af3e827ee05 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -2290,6 +2290,17 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_aer_err_detected);
>  
> +static void vfio_pci_core_aer_reset_done(struct pci_dev *pdev)
> +{
> +	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
> +
> +	if (!vdev->pci_saved_state)
> +		return;

[Severity: High]
Can this race with vfio_pci_core_disable() and cause a use-after-free on
vdev->pci_saved_state?

When a user closes the device fd, vfio_pci_core_disable() is invoked:

vfio_pci_core_disable()
    pci_load_and_free_saved_state(pdev, &vdev->pci_saved_state);

This frees the memory and sets the pointer to NULL without holding the PCI
device_lock.

Concurrently, if a system-initiated function reset occurs, the PCI core
holds device_lock and invokes this reset_done callback via
pci_dev_restore().

Because there is no common lock between these two paths, can the pointer be
freed by vfio_pci_core_disable() immediately after the NULL check here, but
before it is dereferenced in pci_load_saved_state()?

> +
> +	pci_load_saved_state(pdev, vdev->pci_saved_state);
> +	pci_restore_state(pdev);

[Severity: Critical]
Does this unintentionally overwrite the runtime PCI configuration space
during normal device resets?

The PCI core invokes reset_done at the end of standard PCI function resets
via pci_dev_restore(), which includes pci_try_reset_function() triggered by
the VFIO_DEVICE_RESET ioctl.

Normally, pci_try_reset_function() transparently saves the runtime state,
resets the device, and restores it.

However, restoring vdev->pci_saved_state here unconditionally applies the
state from when the device was initially opened. Could this revert all
runtime configuration changes made by a guest VM, such as BAR programming
and MSI-X enablement?

> +}
> +
>  int vfio_pci_core_sriov_configure(struct vfio_pci_core_device *vdev,
>  				  int nr_virtfn)

[ ... ]

> @@ -2354,6 +2365,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_sriov_configure);
>  
>  const struct pci_error_handlers vfio_pci_core_err_handlers = {
>  	.error_detected = vfio_pci_core_aer_err_detected,
> +	.reset_done = vfio_pci_core_aer_reset_done,

[Severity: High]
Will this callback actually be invoked during AER error recovery?

The commit message mentions restoring the device state during PCIe AER error
recovery. However, the AER core in pcie_do_recovery() coordinates recovery
by calling .error_detected, .mmio_enabled, .slot_reset, and .resume.

It uses low-level hardware resets like pci_bus_error_reset() or
pcie_reset_flr() which do not trigger the pci_dev_restore() wrapper.

Since reset_done is exclusively called from pci_dev_restore() during
standard function resets, does the state restoration logic belong in
.slot_reset instead to ensure it executes during an AER event?

>  };
>  EXPORT_SYMBOL_GPL(vfio_pci_core_err_handlers);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603182415.2324-1-alifm@linux.ibm.com?part=3