Re: [PATCH v7 03/15] s390/vfio-ap: Functions to initialize/release vfio device migration data

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The patch exposes the VFIO_MIGRATION_STOP_COPY capability to userspace before the actual migration data transfer logic is implemented, causing silent migration failures.
- [High] An open migration FD can access a newly allocated migration session if the VFIO device is closed and reopened, bypassing the intended `NULL` check.

Pre-existing issues:
- [High] Unconditional NULL assignments to `req_trigger` and `cfg_chg_trigger` in `probe` can overwrite valid eventfd contexts set by concurrent ioctls, causing a memory leak and breaking interrupts.
- [High] The mediated device is exposed to userspace before it is fully initialized and added to the driver's tracking lists, allowing multiple mdevs to attach to the same KVM guest.
--

--- Patch [3]: [PATCH v7 03/15] s390/vfio-ap: Functions to initialize/release vfio device migration data ---
commit 04c1b466fec62cf9e53c2d6610cde9ecbf5fefad
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Functions to initialize/release vfio device migration data

This commit provides the initialization and release functions for the data
structures used during live guest migration of vfio-ap devices. It introduces
stubs for the migration operations that will be fully implemented in
subsequent patches.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 374d3a67cb214..cf303d228a0a5 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
[ ... ]
> +static struct file *vfio_ap_set_state(struct vfio_device *vdev,
> +				      enum vfio_device_mig_state  new_state)
> +{
> +	return NULL;
> +}
[ ... ]
> +void vfio_ap_init_migration_capabilities(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	if (ap_is_se_guest())
> +		return;
> +
> +	matrix_mdev->vdev.migration_flags = VFIO_MIGRATION_STOP_COPY;
> +	matrix_mdev->vdev.mig_ops = &vfio_ap_migration_ops;
> +}

[Severity: High]
Will advertising the VFIO_MIGRATION_STOP_COPY capability to userspace before 
the actual migration data transfer logic is implemented cause silent failures?

If vfio_ap_set_state() returns NULL for the VFIO_DEVICE_STATE_STOP_COPY
transition, won't the VFIO core interpret this as a successful transition
that does not require a data stream? This could cause QEMU to silently
succeed on the source side without transferring any device state, leaving
the AP subsystem broken for the guest on the target side.

> +/**
> + * vfio_ap_release_mig_files:
> + *
> + * Free the ap_config buffers for any open migration FDs. Although a
> + * migration FD may still be held open by userspace, it is safe to free
> + * mig_data here because:
> + *
> + *   1. matrix_mdev remains valid for the lifetime of any open migration
> + *      FD via the vfio_device registration reference taken in
> + *      vfio_ap_open_file_stream() and dropped in
> + *      vfio_ap_release_mig_file().
> + *
> + *   2. mig_data is only accessed by the migration file ops
> + *      (vfio_ap_stop_copy_read, vfio_ap_resuming_write) under
> + *      mdevs_lock. Once mig_data is set to NULL by the caller, those
> + *      paths will see NULL and return -ENODEV before dereferencing it.
> + *

[Severity: High]
Does this lifecycle design handle the case where the VFIO device is closed
and reopened while a migration FD is kept open?

If userspace keeps the migration FD open, closes the VFIO device, and then
reopens it, vfio_ap_mdev_open_device() will allocate a new mig_data. When the
old migration FD subsequently performs read or write operations, won't it
bypass the NULL check and incorrectly access the mig_data belonging to the
new session?

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 36786d70a88f5..90b0fce0123b5 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -797,13 +809,28 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>  	if (IS_ERR(matrix_mdev))
>  		return PTR_ERR(matrix_mdev);
>  
> +	/*
> +	 * Migration capabilities must be initialized before calling
> +	 * vfio_register_emulated_iommu_dev; otherwise, the VFIO core
> +	 * will see mig_ops as NULL during the registration. This could
> +	 * prevent the VFIO core from properly setting up migration
> +	 * infrastructure like debugfs entries.
> +	 *
> +	 * This must be done before acquiring mdevs_lock to avoid an ABBA
> +	 * deadlock: vfio_register_emulated_iommu_dev() acquires dev_set->lock
> +	 * internally, while vfio_ap_mdev_open_device() is called by the VFIO
> +	 * core with dev_set->lock already held and then acquires mdevs_lock.
> +	 */
> +	vfio_ap_init_migration_capabilities(matrix_mdev);
> +
>  	ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev);
>  	if (ret)
>  		goto err_put_vdev;
> +
> +	mutex_lock(&matrix_dev->mdevs_lock);
>  	matrix_mdev->req_trigger = NULL;
>  	matrix_mdev->cfg_chg_trigger = NULL;

[Severity: High]
This is a pre-existing issue, but does exposing the device to userspace
before setting these triggers to NULL introduce a race condition?

If userspace immediately opens the device after
vfio_register_emulated_iommu_dev() and configures interrupts, it will
allocate and set valid eventfd contexts for req_trigger and cfg_chg_trigger.
When the probe thread continues and overwrites these with NULL, won't it leak
the eventfd contexts and break guest interrupts?

>  	dev_set_drvdata(&mdev->dev, matrix_mdev);
> -	mutex_lock(&matrix_dev->mdevs_lock);
>  	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);

[Severity: High]
This is also a pre-existing issue, but does calling
vfio_register_emulated_iommu_dev() before list_add() allow multiple mediated
devices to attach to the same KVM guest?

If two mdevs are probed and opened concurrently before they are added to
mdev_list, won't they bypass the conflict detection logic in
vfio_ap_mdev_set_kvm() and violate KVM uniqueness invariants?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3