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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 8/7/26 6:38 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review fosund 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 <akrowiak@linux.ibm.com>
>
> 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 100644s
>> --- 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.

This is just one of a series of 15 patches that implement VFIO migration of
vfio-ap devices. The rest of the series fills in the blanks.

>
>> +/**
>> + * 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?

This patch (03/15) introduces the initialization and teardown functions
for the migration lifecycle, while subsequent patches in the series
implement the full data streaming operations and validation checks:

- Patch 07/15 ("s390/vfio-ap: File ops called to save the vfio device
   state") implements validate_stop_copy_read_parms().
- Patch 10/15 ("s390/vfio-ap: File ops called to resume the vfio device
   state") implements validate_resuming_write_parms().

In those handlers, both read and write operations verify under mdevs_lock
that the passed struct file * matches the active migration stream
pointer for the current session (mig_data->stop_copy_mig_file.filp or
mig_data->resuming_mig_file.filp).

When the device is closed and reopened, open_device allocates a fresh
mig_data with NULL file pointers. If a stale migration FD from a
previous session attempts to perform read or write operations, the
equality check against the current session's filp fails (-EINVAL or
-ENXIO), safely preventing access to the new session's mig_data.

>
>> 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?

This was fixed with commit 7fa61c29850d in response to this same comment
in another review. 7fa61c29850d

>
>>   	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?

This will be fixed with a standalone patch since it is not related to this
series.

>