Re: [RFC PATCH v3 18/20] vfio/pci: Block device access during host recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] vfio/pci: unrestored BARs after bus reset when recovery fails
  in err_detected()
- [Medium] vfio/pci: TOCTOU race on pci_recovery_host_active during
  device open
- [Medium] vfio/pci: close during host recovery races with bus reset and
  loses saved state
- [Medium] vfio/pci: close clears device_open before blocking PM config
  restore
- [Low] vfio/pci: dead !device_open check in
  vfio_pci_core_aer_err_detected()
--

commit f9997001c74d4f1e906c8a9ec79cb059867865ef
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Block device access during host recovery

This commit blocks userspace and asynchronous VFIO device access during
host PCI error recovery in vfio_pci_core_aer_err_detected(), coordinates
blocked power-state and INTx handling with memory_lock and irqlock, and
rejects device open while host recovery is active.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 09923a48a5470..5e9451e082630 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -410,10 +414,17 @@ static void vfio_pci_core_block_access(struct vfio_pci_core_device *vdev)
>  	unsigned long flags;
>  
>  	lockdep_assert_held(&vdev->access_lock);
> +	/*
> +	 * Finish live PM restoration before blocked INTx handlers can mask the
> +	 * device. Otherwise an old restore can undo their hardware mask.
> +	 */
> +	down_write(&vdev->memory_lock);
>  	spin_lock_irqsave(&vdev->irqlock, flags);
>  	rcu_assign_pointer(vdev->access_ops, &vfio_pci_blocked_ops);
>  	spin_unlock_irqrestore(&vdev->irqlock, flags);
> +	up_write(&vdev->memory_lock);
>  
> +	/* SRCU readers may need memory_lock to finish. */
>  	synchronize_srcu(&vdev->access_srcu);
>  }

[Severity: Medium]
Does clearing vdev->device_open before calling vfio_pci_core_block_access()
in vfio_pci_core_close_device() defeat this protection against PM config
restoration unmasking INTx?

In __vfio_pci_set_power_state(), D3hot-to-D0 restoration bypasses
access_ops->restore_config() as soon as vdev->device_open is false:

drivers/vfio/pci/vfio_pci_core.c:__vfio_pci_set_power_state() {
    ...
		} else if (needs_restore) {
			pci_load_and_free_saved_state(pdev, &vdev->pm_save);
			/* Open and close restore state without reopening access. */
			if (!READ_ONCE(vdev->device_open))
				pci_restore_state(pdev);
			else
				vfio_pci_access_ops(vdev)->restore_config(vdev);
		}
    ...
}

If a NoSoftRst- PCI 2.3 device entered D3hot while INTx was unmasked
(saving unmasked PCI_COMMAND in vdev->pm_save) and then had INTx masked by
recovery, both an in-flight runtime PM transition holding memory_lock when
vfio_pci_core_close_device() clears vdev->device_open and the subsequent
vfio_pci_set_power_state(vdev, PCI_D0) at the start of
vfio_pci_core_disable() take the pci_restore_state(pdev) branch.  That
restores the unmasked PCI_COMMAND in hardware while ctx->masked is still
true and the IRQ handler is still registered, so a level-triggered INTx
assertion before vfio_pci_set_irqs_ioctl() runs sees ctx->masked == true in
vfio_pci_intx_mask_for_recovery() and returns IRQ_NONE without masking the
shared interrupt line.

> @@ -756,6 +767,16 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  	u16 cmd;
>  	u8 msix_pos;
>  
> +	if (vdev->pci_recovery_supported) {
> +		if (pci_dev_is_disconnected(pdev))
> +			return -ENODEV;
> +		mutex_lock(&vdev->access_lock);
> +		ret = vdev->pci_recovery_host_active ? -EBUSY : 0;
> +		mutex_unlock(&vdev->access_lock);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (!vdev->disable_idle_d3) {

[Severity: Medium]
Can host recovery start after vfio_pci_core_enable() drops
vdev->access_lock and still allow vfio_pci_core_finish_enable() to publish
live access?

CPU0                                    CPU1
vfio_pci_core_enable()
  mutex_lock(&vdev->access_lock)
  /* pci_recovery_host_active is false */
  mutex_unlock(&vdev->access_lock)
                                        pcie_do_recovery()
                                          vfio_pci_core_aer_err_detected()
                                            mutex_lock(&vdev->access_lock)
                                            vdev->pci_recovery_host_active = true
                                            /* !pci_recovery_enabled */
                                            mutex_unlock(&vdev->access_lock)
                                          reset_subordinates()
  pci_save_state(pdev)
  vfio_config_init(vdev)
vfio_pci_core_finish_enable()
  mutex_lock(&vdev->access_lock)
  WRITE_ONCE(vdev->device_open, true)
  rcu_assign_pointer(vdev->access_ops,
                     &vfio_pci_live_ops) <-- live access while recovery runs

Because vfio_pci_core_enable() runs without device_lock(&pdev->dev) held
across enablement and vfio_pci_core_finish_enable() does not re-check
vdev->pci_recovery_host_active or pci_dev_is_disconnected(pdev), the open
succeeds while host recovery is active, vdev->pci_saved_state and
vdev->rbar[] can capture corrupted or post-reset BAR values, and
vfio_pci_core_aer_slot_reset() skips state restoration because
VFIO_PCI_RECOVERY_IN_PROGRESS was never set.

> @@ -999,6 +1020,10 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
>  			WRITE_ONCE(vdev->device_open, false);
>  			vfio_pci_core_block_access(vdev);
>  			WRITE_ONCE(vdev->pci_recovery_flags, 0);
> +			/* Discard pending D0 before the next open. */
> +			down_write(&vdev->memory_lock);
> +			vdev->power_up_pending = false;
> +			up_write(&vdev->memory_lock);
>  		}
>  	}

[Severity: Medium]
What happens if userspace closes the device after
vfio_pci_core_aer_err_detected() signals err_trigger, while
pcie_do_recovery() is resetting the bus?

CPU0                                    CPU1
pcie_do_recovery()
  vfio_pci_core_aer_err_detected()
    /* returns NEED_RESET */
                                        vfio_pci_core_close_device()
                                          WRITE_ONCE(vdev->device_open, false)
                                          WRITE_ONCE(vdev->pci_recovery_flags, 0)
                                          vfio_pci_core_disable()
                                            pci_load_and_free_saved_state(
                                                pdev, &vdev->pci_saved_state)
                                            pci_restore_state(pdev)
  reset_subordinates()
    /* resets bus, wiping BARs */
  vfio_pci_core_aer_slot_reset()
    /* !device_open -> returns NONE */  <-- BARs left zeroed in hardware

pcie_do_recovery() drops device_lock(&pdev->dev) between error_detected()
and slot_reset().  If vfio_pci_core_disable() consumes and frees
vdev->pci_saved_state before reset_subordinates() finishes resetting the
secondary bus without restoring config space, vfio_pci_core_aer_slot_reset()
sees !vdev->device_open and returns early, leaving the hardware BARs zeroed
for the next open.  If close instead runs just after reset_subordinates()
wipes the BARs, vfio_pci_core_disable() tears down MSI-X in
vfio_pci_set_irqs_ioctl() before restoring vdev->pci_saved_state.

> @@ -2660,17 +2685,142 @@ void vfio_pci_core_unregister_device(struct vfio_pci_core_device *vdev)

[ ... ]

> +	mutex_lock(&vdev->access_lock);
> +	/*
> +	 * Track host recovery even without userspace opt-in so open can reject
> +	 * attempts while recovery is running.
> +	 */
> +	vdev->pci_recovery_host_active =
> +		state != pci_channel_io_perm_failure;
> +	if (!vdev->pci_recovery_enabled)
> +		goto out_unlock;
> +
> +	/* Keep failed devices blocked until close. */
> +	if (vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_FAILED) {
> +		result = PCI_ERS_RESULT_NONE;
> +		goto out_unlock;
> +	}
> +
> +	if (!vdev->device_open) {
> +		result = PCI_ERS_RESULT_NONE;
> +		goto out_unlock;
> +	}

[Severity: Low]
Can the !vdev->device_open check ever be reached here?

Under vdev->access_lock, vdev->pci_recovery_enabled is only set when
vdev->device_open is true and is cleared alongside vdev->device_open in
vfio_pci_core_close_device(), so !vdev->pci_recovery_enabled always
branches to out_unlock first (returning PCI_ERS_RESULT_CAN_RECOVER rather
than PCI_ERS_RESULT_NONE when the device is closed).

> +
> +	vfio_pci_core_block_access(vdev);

[ ... ]

> +	if (!ret && !vdev->pci_2_3 && !nested)
> +		ret = vfio_pci_recovery_save_and_clear_master(vdev, 0);
>  
> +	if (ret) {
> +		flags |= VFIO_PCI_RECOVERY_FAILED;
> +		flags &= ~VFIO_PCI_RECOVERY_IN_PROGRESS;
> +		result = PCI_ERS_RESULT_NONE;
> +	} else {
> +		flags |= VFIO_PCI_RECOVERY_IN_PROGRESS;
> +	}
> +	WRITE_ONCE(vdev->pci_recovery_flags, flags);

[Severity: High]
If recovery fails in vfio_pci_core_aer_err_detected() (or
VFIO_PCI_RECOVERY_FAILED is already set on a subsequent frozen-channel
error) and pcie_do_recovery() still resets the subordinate bus, will the
device's BARs be left zeroed when vfio_pci_core_disable() later tears down
MSI-X?

Clearing VFIO_PCI_RECOVERY_IN_PROGRESS and returning PCI_ERS_RESULT_NONE
allows pcie_do_recovery() to proceed with reset_subordinates() when a
sibling device requests PCI_ERS_RESULT_NEED_RESET or when state is
pci_channel_io_frozen, wiping this device's BARs in hardware.  Because
VFIO_PCI_RECOVERY_IN_PROGRESS is not set, vfio_pci_core_aer_slot_reset()
returns early without calling vfio_pci_recovery_restore_state() or tearing
down IRQs:

drivers/vfio/pci/vfio_pci_core.c:vfio_pci_core_aer_slot_reset() {
	mutex_lock(&vdev->access_lock);
	if (!(vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_IN_PROGRESS) ||
	    !vdev->device_open) {
		mutex_unlock(&vdev->access_lock);
		return PCI_ERS_RESULT_NONE;
	}
    ...
}

When userspace later closes the device, vfio_pci_core_disable() calls
vfio_pci_set_irqs_ioctl() -> vfio_msi_disable() before loading
vdev->pci_saved_state, which enables PCI_COMMAND_MEMORY via
vfio_pci_memory_lock_and_enable() and writes to the MSI-X table while the
hardware BARs are still 0x00000000.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112141632.918954-1-skolothumtho@nvidia.com?part=18