Re: [PATCH] vfio/pci: Restore standard PCI config space in .slot_reset()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The `vfio_pci_core_aer_slot_reset` callback has an incorrect function signature that violates `struct pci_error_handlers`, causing CFI panics.
- [High] The newly added `.slot_reset` callback is unreachable dead code for isolated VFIO devices because `error_detected` never returns `PCI_ERS_RESULT_NEED_RESET`.
- [High] `pci_restore_state()` clobbers userspace runtime PCI configuration with an outdated kernel probe-time snapshot, breaking VFIO virtualization state.
- [High] `pci_restore_state()` modifies hardware state locklessly, bypassing VFIO synchronization primitives and causing severe race conditions with concurrent MMIO accesses.
- [Medium] Prematurely returning `PCI_ERS_RESULT_RECOVERED` from `.slot_reset` races with userspace recovery logic due to the lack of a reset completion notification.
--

--- Patch [1]: [PATCH] vfio/pci: Restore standard PCI config space in .slot_reset() ---
commit e72e9372390b9003aeabcf98521c52beaad6c1a4
Author: Keith Busch <kbusch@kernel.org>

vfio/pci: Restore standard PCI config space in .slot_reset()

Currently, when a link reset or Secondary Bus Reset occurs due to an
AER/DPC event, the kernel PCI error recovery service executes reset
routines across affected downstream devices. Since vfio-pci does not
implement the .slot_reset() callback in its pci_error_handlers struct,
the standard PCI config space (BARs, Command register, MSI-X setup,
DevCtl, etc.) for downstream devices is left entirely uninitialized
after the link is brought back up.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index ea7bcd44bdfa9..9abf725446158 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -2351,6 +2351,13 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_aer_err_detected);
>  
> +pci_ers_result_t vfio_pci_core_aer_slot_reset(struct pci_dev *pdev,
> +					      pci_channel_state_t state)

[Severity: Critical]
Does the extra pci_channel_state_t argument violate the slot_reset
callback signature in struct pci_error_handlers?

The slot_reset callback expects a function pointer with exactly one
argument: pci_ers_result_t (*slot_reset)(struct pci_dev *dev).

Will this extra argument cause a control flow integrity panic or compiler
type-mismatch error when the PCI AER core invokes this via
err_handler->slot_reset(dev)?

[Severity: High]
Is this callback reachable under normal single-device circumstances?

In pcie_do_recovery(), the report_slot_reset broadcast is gated behind
checking if the combined error status is PCI_ERS_RESULT_NEED_RESET.

However, vfio_pci_core_aer_err_detected() unconditionally returns
PCI_ERS_RESULT_CAN_RECOVER. The PCI recovery state machine promotes
CAN_RECOVER to RECOVERED during the mmio_enabled phase, so the
condition to call slot_reset evaluates to false unless another driver
on the same PCIe bridge votes for NEED_RESET.

> +{
> +	pci_restore_state(pdev);

[Severity: High]
Can pci_restore_state() clobber userspace runtime configurations with
an outdated probe-time snapshot?

For a VFIO device, pdev->saved_config_space contains the host kernel's
baseline state captured during vfio_pci_core_enable(). Subsequent
userspace modifications (like enabling bus master or setting up MSI)
are recorded in VFIO's virtual cache (vdev->vconfig) but do not update
pdev->saved_config_space.

Will reverting the hardware to this stale baseline silently destroy
guest configurations while VFIO still thinks they are intact?

[Severity: High]
Does modifying the hardware state here locklessly bypass VFIO
synchronization primitives?

The PCI AER core invokes the slot_reset callback holding only the
baseline device lock. VFIO relies on vdev->memory_lock (using
down_write(&vdev->memory_lock)) to serialize state-mutating operations
against concurrent userspace operations like vfio_pci_rw or MMIO page
faults.

Could calling pci_restore_state() without this lock rewrite BARs and
the PCI Command register concurrently with active userspace MMIO accesses,
potentially causing host crashes or machine check exceptions?

> +	return PCI_ERS_RESULT_RECOVERED;

[Severity: Medium]
Does returning PCI_ERS_RESULT_RECOVERED here create a race condition
with userspace recovery logic?

VFIO notifies userspace via the VFIO_PCI_ERR_IRQ_INDEX eventfd when
an error is initially detected. By unconditionally returning
PCI_ERS_RESULT_RECOVERED, the host kernel completes its physical PCI
recovery and brings the link fully up before userspace has a chance
to react or rebuild its state.

Since there is no explicit completion notification to coordinate with
the kernel's state machine, will userspace operate blindly and potentially
attempt to access the device concurrently during the kernel's reset
sequence?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817195110.2077698-1-kbusch@meta.com?part=1