Re: [RFC PATCH 10/19] vfio/pci: Serialize hot reset with recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] AB-BA R-W-R deadlock between memory_lock and pci_bus_sem during hot reset and AER recovery.
- [High] A race condition allows a user-initiated hot reset to bypass the `pci_recovery_access_blocked` check, leading to a bus reset during an ongoing AER recovery.
--

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

vfio/pci: Serialize hot reset with recovery

This patch attempts to serialize user-initiated hot resets with hardware
error recovery by refusing hot resets when recovery blocks access to
the device. It introduces a lockless check of the access_blocked flag
instead of taking recovery_lock to avoid lock inversions.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 876ff51d6987a..bd3d79d28f27a 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -2937,6 +2937,27 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
>  			break;
>  		}
>  
> +		/*
> +		 * Between the AER callbacks memory_lock is not held, so refuse
> +		 * the reset on access_blocked as well. Read it rather than
> +		 * take recovery_lock, which would have to be released before
> +		 * pci_reset_bus() anyway since that reaches pci_bus_sem.
> +		 *
> +		 * Let a device which has failed for good through. Nothing is
> +		 * running on its behalf, and this reset covers the whole set,
> +		 * so refusing there would stop a healthy sibling from being
> +		 * reset because an unrelated device errored. The failed one
> +		 * stays blocked, since the reset does not unblock a device
> +		 * with FAILED set.
> +		 */
> +		if (vdev->pci_recovery_supported &&
> +		    READ_ONCE(vdev->pci_recovery_access_blocked) &&
> +		    !(READ_ONCE(vdev->pci_recovery_flags) &
> +		      VFIO_PCI_RECOVERY_FAILED)) {
> +			ret = -EBUSY;
> +			break;
> +		}
> +

[Severity: High]
Does this lockless check of pci_recovery_access_blocked create a race
window before memory_lock is acquired?

If the hot reset thread checks this flag and sees it as false, it can be
preempted. If the AER handler then runs and sets access_blocked to true:

vfio_pci_core_aer_err_detected()
    WRITE_ONCE(vdev->pci_recovery_access_blocked, true);
    ...
    vfio_pci_zap_and_down_write_memory_lock(vdev);
    ...
    up_write(&vdev->memory_lock);

Could the hot reset thread subsequently wake up, successfully acquire
memory_lock in the down_write_trylock() below, and call pci_reset_bus() while
the AER process is in the middle of recovery (e.g., between error_detected and
slot_reset)? 

This could bypass the intended protection and corrupt the recovery state.

>  		/*
>  		 * Take the memory write lock for each device and zap BAR
>  		 * mappings to prevent the user accessing the device while in
>  		 * reset.  Locking multiple devices is prone to deadlock,
>  		 * runaway and unwind if we hit contention.
>  		 */
>  		if (!down_write_trylock(&vdev->memory_lock)) {
>  			ret = -EBUSY;
>  			break;
>  		}

[ ... ]

>  	list_for_each_entry(vdev, &dev_set->device_list, vdev.dev_set_list)
>  		vfio_pci_set_power_state(vdev, PCI_D0);
>  
>  	ret = pci_reset_bus(pdev);

[Severity: High]
Can this sequence lead to an AB-BA R-W-R deadlock between memory_lock and
pci_bus_sem?

In this path, the user-initiated hot reset acquires memory_lock for write, and
then pci_reset_bus() acquires pci_bus_sem for read via
pci_bridge_wait_for_secondary_bus().

However, during AER recovery, pci_walk_bridge() holds pci_bus_sem for read,
and then calls into vfio_pci_core_aer_err_detected() which waits for
memory_lock for write:

vfio_pci_core_aer_err_detected()
    vfio_pci_zap_and_down_write_memory_lock(vdev)
        down_write(&vdev->memory_lock);

If a third thread (e.g., sysfs rescan) requests pci_bus_sem for write, it
blocks new readers (the hot reset thread) to prevent starvation, while waiting
on existing readers (the AER thread).

This creates an unresolvable cycle:
- Hot reset thread holds memory_lock (W), waits for pci_bus_sem (R)
- Third thread waits for pci_bus_sem (W), blocks new readers
- AER thread holds pci_bus_sem (R), waits for memory_lock (W)

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