Re: [PATCH v15 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` leads to an exploitable Use-After-Free on `vdev->pci_saved_state`.
- [High] The return value of `pci_load_saved_state()` is ignored, potentially pushing corrupted state to the hardware.
- [Medium] The state recovery logic is marked `static`, depriving vendor-specific VFIO PCI drivers of the feature.
--

commit 50af2d4f1aef440c94cd13a26e090b9661198f8c
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 the vfio-pci driver, this
commit attempts to restore the device state to its initial saved state
by adding a reset_done callback.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index f47a49ca67fae..6358b59fab2b1 100644
> --- 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)

Vendor-specific VFIO PCI drivers sometimes override the .reset_done callback
in their pci_error_handlers. Should this function be exported instead of
static, so that vendor drivers can also invoke the core framework's state
restoration?

> +{
> +	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
> +
> +	if (!vdev->pci_saved_state)
> +		return;
> +

Is there a race condition here between the AER recovery thread and the VFIO
device close path?

The AER thread invokes vfio_pci_core_aer_reset_done() holding
device_lock(&pdev->dev), but without VFIO-specific locks like dev_set->lock.

Concurrently, closing the VFIO device file descriptor invokes the disable
path:

vfio_pci_core_close_device() -> vfio_pci_core_disable()

The disable path runs with dev_set->lock held, but without
device_lock(&pdev->dev), and it frees the saved state:

pci_load_and_free_saved_state(pdev, &vdev->pci_saved_state)

Could vdev->pci_saved_state be freed immediately after the check here,
causing a use-after-free when it is passed to pci_load_saved_state() below?

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

Can pci_load_saved_state() fail and return an error?

If it returns an error, such as -EINVAL for capability size mismatches,
dev->state_saved remains false and the capability buffers may only be
partially copied.

Since the return value is ignored here, does calling pci_restore_state(pdev)
immediately afterward risk pushing partially populated or corrupted state back
to the hardware?

> +}
> +

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