Re: [PATCH v10 13/15] s390/vfio-ap: Set up for vfio-ap device migration

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

On 10/23/26 2:14 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] s390/vfio-ap: wrong SE guest check via ap_is_se_guest() on KVM host
> - [Medium] s390/vfio-ap: stale completion flag and f_pos advance in vfio_ap_resuming_write()
> --
>
> commit 7adada8cbaf0b3057bf058ad4617540b9cea4ec9
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Set up for vfio-ap device migration
>
> This commit wires up VFIO migration operations and sets the
> VFIO_MIGRATION_STOP_COPY flag on the vfio-ap mdev during device probe to
> enable live migration support.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
>> index 38098968eaa2c..660b5a21fe9e8 100644
>> --- a/drivers/s390/crypto/vfio_ap_migration.c
>> +++ b/drivers/s390/crypto/vfio_ap_migration.c
>> @@ -212,6 +212,30 @@ static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
>>   	return 0;
>>   }
>>   
>> +static const struct vfio_migration_ops vfio_ap_migration_ops;
>> +
>> +/**
>> + * vfio_ap_setup_vfio_device_migration:
>> + *
>> + * Plug the object containing the function pointers to the migration callbacks
>> + * required to facilitate VFIO AP migration into the mdev
>> + *
>> + *
>> + * @matrix_mdev: pointer to object containing the mdev state
>> + */
>> +void vfio_ap_setup_vfio_device_migration(struct ap_matrix_mdev *matrix_mdev)
>> +{
>> +	/*
>> +	 * Live guest migration is not supported for SE guests, so there is no
>> +	 * need to initialize the migration capabilities.
>> +	 */
>> +	if (ap_is_se_guest())
>> +		return;
> [Severity: High]
> Does calling ap_is_se_guest() here and in vfio_ap_init_migration_data()
> actually detect a Secure Execution guest on the KVM host?

No, but it checks whether the host on which it is executed is an SE
guest, in which case a nested SE guest can not be migrated.

>
> In drivers/s390/crypto/ap_bus.c, ap_is_se_guest() checks whether the
> currently running kernel is itself a Protected Virtualization guest:
>
> bool ap_is_se_guest(void)
> {
> 	return is_prot_virt_guest() && ap_sb_available();
> }
>
> Since vfio_ap runs in the KVM host kernel where is_prot_virt_guest() is
> always false, ap_is_se_guest() will always evaluate to false on the host.
>
> Furthermore, at vfio_ap_mdev_probe() -> vfio_ap_setup_vfio_device_migration()
> time, matrix_mdev->kvm is still NULL, and a guest only enters Protected
> Virtualization mode at runtime via KVM_S390_PV_COMMAND. Should the SE guest
> check instead be performed dynamically against matrix_mdev->kvm via
> kvm_s390_pv_is_protected() when handling migration state transitions in
> vfio_ap_set_state() or vfio_ap_transition_to_state()?

Performing the check dynamically in vfio_ap_set_state() or
vfio_ap_transition_to_state() would also be wrong for a second
reason: ap_is_se_guest() at probe time is the correct place
because the host's SE guest status is fixed at boot. Deferring
the check to the first state transition would mean advertising
migration capability to QEMU via mig_ops and
VFIO_MIGRATION_STOP_COPY and then failing at runtime — which
is worse behavior than simply not advertising the capability
in the first place.

>
>> +
>> +	matrix_mdev->vdev.migration_flags = VFIO_MIGRATION_STOP_COPY;
>> +	matrix_mdev->vdev.mig_ops = &vfio_ap_migration_ops;
>> +}
> [Severity: Medium]
> Now that vfio_ap_migration_ops is wired up and exposes the migration stream
> and state transitions to userspace, can vfio_ap_resuming_write() leave
> resuming_mig_data_complete set to true or leave filp->f_pos advanced when a
> write fails?
>
> In drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_resuming_write():
>
> 	memcpy((char *)ap_config + *pos, kbuf, len);
> 	*pos += len;
>
> 	if (*pos >= offsetofend(struct vfio_ap_config, num_queues)) {
> 		size_t expected_sz;
>
> 		if (ap_config->num_queues > MAX_AP_QUEUES) {
> 			ret = -EINVAL;
> 			goto out_unlock;
> 		}
>
> 		expected_sz = sizeof(struct vfio_ap_config) +
> 			      ap_config->num_queues *
> 			      sizeof(struct vfio_ap_queue_info);
>
> 		if (*pos > (loff_t)expected_sz) {
> 			ret = -EINVAL;
> 			goto out_unlock;
> 		}
>
> 		if (*pos == (loff_t)expected_sz) {
> 			ret = do_post_copy_validation(matrix_mdev, ap_config);
> 			matrix_mdev->mig_data->resuming_mig_data_complete = !ret;
> 		}
> 	}
>
> If userspace writes a valid payload where *pos == expected_sz,
> resuming_mig_data_complete is set to true. If userspace then issues an extra
> write to the resuming FD before transitioning to VFIO_DEVICE_STATE_STOP, the
> extra bytes are copied into ap_config, *pos is incremented past expected_sz,
> and the function returns -EINVAL without clearing resuming_mig_data_complete.
> A subsequent transition from VFIO_DEVICE_STATE_RESUMING to
> VFIO_DEVICE_STATE_STOP in vfio_ap_transition_to_state() only checks
> mig_data->resuming_mig_data_complete and will still succeed.
>
> Also, when any of the checks on num_queues, expected_sz, or
> do_post_copy_validation() fail, *pos has already been incremented even though
> vfio_ap_resuming_write() returns an error. Should the validation happen
> before advancing *pos and clear resuming_mig_data_complete on error?

This is a duplicate of questions asked and answered in previous commit 
reviews.

>