Re: [PATCH v16 6/7] 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] Concurrent execution of `vfio_pci_core_aer_reset_done` and `vfio_pci_core_disable` causes a Use-After-Free and data race on saved state.
- [High] UAPI breakage and runtime state corruption due to `reset_done` unconditionally restoring initialization state.
- [Medium] The error return value of `pci_load_saved_state()` is ignored, leading to potentially corrupted state restoration.
--

commit 50de5d9e39c609551f40181c5b1916513f9d200a
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
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -2255,6 +2255,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);
> +

Since pci_error_handlers->reset_done can be invoked by the PCI core at the
end of any device reset, including those initiated by userspace via the
VFIO_DEVICE_RESET ioctl, could this unconditionally overwrite the runtime
userspace configurations with the initial host state?

> +	if (!vdev->pci_saved_state)
> +		return;

Is there a potential use-after-free race on vdev->pci_saved_state here if a
user concurrently closes the VFIO file descriptor?

In vfio_pci_core_disable(), vdev->pci_saved_state can be freed via
pci_load_and_free_saved_state() before acquiring the PCI device lock. Since
this callback does not synchronize with the VFIO teardown path, could the
AER thread see vdev->pci_saved_state as valid, but then pass a dangling
pointer to pci_load_saved_state() after it is freed?

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

Should the return value of pci_load_saved_state() be checked here?

If it returns an error due to mismatched capability sizes, the partial
memcpy into pdev->saved_config_space is kept. Proceeding to call
pci_restore_state() might then push an inconsistent state to the hardware.

> +	pci_restore_state(pdev);
> +}

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