Re: [PATCH v7 05/15] s390/vfio-ap: Callback to get/set vfio device mig state during guest migration

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] vfio_ap_set_state falsely reports success when attempting to transition out of the VFIO_DEVICE_STATE_ERROR state.
--

--- Patch [5]: [PATCH v7 05/15] s390/vfio-ap: Callback to get/set vfio device mig state during guest migration ---
commit 4d4bb0a626dab0e0e55ea92759151b23e4fe2cdf
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Callback to get/set vfio device mig state during guest migration

This commit implements two callbacks for the vfio-ap device related to its 
current migration state. It provides the mandatory callbacks to get and set 
the migration state during live migration of guests with pass-through access.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 1613813124b6f..af740b177c97c 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)
> +{
> +	int ret;
> +	struct file *filp = NULL;
> +	struct ap_matrix_mdev *matrix_mdev;

[ ... ]

> +	while (mig_data->mig_state != VFIO_DEVICE_STATE_ERROR &&
> +	       mig_data->mig_state != new_state) {
> +		ret = vfio_mig_get_next_state(vdev, mig_data->mig_state,
> +					      new_state, &next_state);
> +		if (ret) {
> +			filp = ERR_PTR(ret);
> +			break;
> +		}
> +
> +		filp = vfio_ap_transition_to_state(matrix_mdev, next_state);
> +		if (IS_ERR(filp))
> +			break;
> +
> +		mig_data->mig_state = next_state;
> +
> +		if (WARN_ON(filp && new_state != next_state)) {
> +			fput(filp);
> +			filp = ERR_PTR(-EINVAL);
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	return filp;

[Severity: High]
If the device is already in VFIO_DEVICE_STATE_ERROR when vfio_ap_set_state()
is called, the while loop condition evaluates to false and is skipped entirely.

Since filp is initialized to NULL, does returning NULL here incorrectly
signal a successful state transition to the VFIO core? Should an attempt to
transition out of the error state return an explicit error pointer (such as
ERR_PTR(-EIO)) instead to properly reject the transition?

> +}

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