Re: [PATCH v4 02/16] vfio/pci: Preserve vfio-pci device files across Live Update

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition when validating `vdev->irq_type`.
- [High] Post-freeze access vulnerability allows userspace to re-activate the device.
- [High] Silent failure of `vfio_pci_core_try_reset()` leaves the device in an inconsistent state.
- [Medium] Missing `kho_is_enabled()` check before calling KHO preservation APIs.
- [Medium] Missing INTx disable before device reset may cause spurious interrupts.
--

commit 8744f868d8d8c3c92253e4db7b371be095529dce
Author: Vipin Sharma <vipinsh@google.com>

vfio/pci: Preserve vfio-pci device files across Live Update

Implement the live update file handler callbacks to preserve a vfio-pci
device across a Live Update. State about each device is serialized into
a new ABI struct vfio_pci_core_device_ser and preserved using KHO and LUO.

> diff --git a/drivers/vfio/pci/vfio_pci_liveupdate.c b/drivers/vfio/pci/vfio_pci_liveupdate.c
> index 1c74854600ea4..5cc57bf39dbee 100644
> --- a/drivers/vfio/pci/vfio_pci_liveupdate.c
> +++ b/drivers/vfio/pci/vfio_pci_liveupdate.c

[ ... ]

> +static int vfio_pci_liveupdate_preserve(struct liveupdate_file_op_args *args)
>  {
> -	return -EOPNOTSUPP;
> +	struct vfio_device *device = vfio_device_from_file(args->file);
> +	struct vfio_pci_core_device_ser *ser;
> +	struct vfio_pci_core_device *vdev;
> +	struct pci_dev *pdev;
> +
> +	vdev = container_of(device, struct vfio_pci_core_device, vdev);
> +	pdev = vdev->pdev;
> +
> +	ser = kho_alloc_preserve(sizeof(*ser));

Is it safe to call kho_alloc_preserve() without first checking if the KHO
subsystem is enabled via kho_is_enabled()?

Unconditionally allocating and preserving memory might add tracking state to
folios that will never be utilized if the subsystem is disabled.

[ ... ]

> +static int vfio_pci_liveupdate_freeze(struct liveupdate_file_op_args *args)
> +{
> +	struct vfio_device *device = vfio_device_from_file(args->file);
> +	struct vfio_pci_core_device *vdev;
> +	struct pci_dev *pdev;
> +	int ret;
> +
> +	vdev = container_of(device, struct vfio_pci_core_device, vdev);
> +	pdev = vdev->pdev;
> +
> +	guard(mutex)(&device->dev_set->lock);
> +
> +	/*
> +	 * Userspace must disable interrupts on the device prior to freeze so
> +	 * that the device does not send any interrupts until new interrupt
> +	 * handlers have been established by the next kernel.
> +	 */
> +	if (vdev->irq_type != VFIO_PCI_NUM_IRQS) {

Does this check need to hold vdev->igate?

Without the mutex, could a concurrent ioctl() such as VFIO_DEVICE_SET_IRQS
modify the irq_type immediately after this check passes, allowing the device
to generate interrupts during kexec?

> +		pci_err(pdev, "Freeze failed! Interrupts are still enabled.\n");
> +		return -EINVAL;
> +	}
> +
> +	guard(rwsem_write)(&vdev->memory_lock);

[ ... ]

> +	/*
> +	 * Reset is a temporary measure to provide kernel after kexec a clean
> +	 * device while VFIO live update work is under development and not
> +	 * fully supported.  It will go away once continuous DMA support is
> +	 * added to device preservation.
> +	 */
> +	vfio_pci_zap_bars(vdev);
> +	ret = pci_load_saved_state(pdev, vdev->pci_saved_state);
> +	if (ret)
> +		return ret;
> +	pci_clear_master(pdev);

Does the PCI_COMMAND register need to have PCI_COMMAND_INTX_DISABLE explicitly
written prior to calling vfio_pci_core_try_reset()?

I noticed that the standard disable path in vfio_pci_core_disable() does this
to prevent spurious interrupts from firing during the hardware reset.

> +	vfio_pci_core_try_reset(vdev);

If vfio_pci_core_try_reset() silently fails because it cannot acquire the PCI
locks or because the device does not support reset, the kexec will still
proceed.

Could this allow the next kernel to inherit a device that still has active DMA
engines or internal state?

> +	pci_restore_state(pdev);
> +	return 0;
> +}

Once vfio_pci_liveupdate_freeze() completes and releases the locks, what
prevents userspace threads from issuing new write(), mmap(), or ioctl() calls
on the device file descriptor?

Because kexec doesn't freeze userspace tasks, could a concurrent thread
re-enable DMA and interrupts on the device right before the kexec jump,
risking memory corruption in the successor kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511234802.2280368-1-vipinsh@google.com?part=2