Re: [PATCH v4 04/15] s390/vfio-ap: Reset migration state in VFIO_DEVICE_RESET ioctl handler

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Build Failure / Undefined Function / Potential NULL Pointer Dereference in `vfio_ap_reset_migration_state`.
- [Low] Typo in comment: 'stte' instead of 'state'.

Pre-existing issues:
- [High] System-wide Denial of Service via unbounded wait holding the global `matrix_dev->mdevs_lock`.
--

--- Patch [4]: [PATCH v4 04/15] s390/vfio-ap: Reset migration state in VFIO_DEVICE_RESET ioctl handler ---
commit 35f281972b36c8fc12cff370107f5d4f61c3aa01
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Reset migration state in VFIO_DEVICE_RESET ioctl handler

This patch adds a function called during VFIO_DEVICE_RESET to release
migration files and free allocated storage for migration state. It resets
the device migration state to RUNNING as required by the specification.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 5b6d48ccf332c..c7c71980bdbb8 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
[ ... ]
> @@ -129,3 +129,23 @@ void vfio_ap_release_migration_data(struct ap_matrix_mdev *matrix_mdev)
>  	kfree(matrix_mdev->mig_data);
>  	matrix_mdev->mig_data = NULL;
>  }
> +
> +/**
> + * vfio_ap_reset_migration_state - Reset the vfio-ap migration state
> + *
> + * @matrix_mdev: pointer to the object maintaining the vfio-ap device state
> + *
> + * Called during VFIO_DEVICE_RESET to clean up any active migration
> + * stte and reset the device to RUNNING state as required by the VFIO

[Severity: Low]
This isn't a bug, but there is a typo here. Did you mean "state" instead of
"stte"?

> + * migration specification.
> + */
> +void vfio_ap_reset_migration_state(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
> +
> +	if (!matrix_mdev->mig_data)
> +		return;
> +
> +	vfio_ap_release_mig_files(matrix_mdev);

[Severity: Critical]
Does vfio_ap_release_mig_files() exist in the kernel? I could not find a
definition or declaration for it.

If the intention was to call vfio_ap_release_migration_data() to free the
storage as mentioned in the commit message, wouldn't that set
matrix_mdev->mig_data to NULL?

> +	matrix_mdev->mig_data->mig_state = VFIO_DEVICE_STATE_RUNNING;

If matrix_mdev->mig_data is freed and set to NULL just prior to this, won't
this line unconditionally dereference the newly NULL pointer and cause a
crash?

> +}
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 8132902148669..8a9e83921f74e 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2332,6 +2332,10 @@ static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev,
>  		break;
>  	case VFIO_DEVICE_RESET:
>  		ret = vfio_ap_mdev_reset_queues(matrix_mdev);

[Severity: High]
This is a pre-existing issue, but can this lead to a system-wide deadlock?

In vfio_ap_mdev_ioctl(), the global matrix_dev->mdevs_lock is acquired
before calling vfio_ap_mdev_reset_queues(). 

Inside vfio_ap_mdev_reset_queues(), flush_work() is called, which
synchronously waits for apq_reset_check() to complete:

vfio_ap_mdev_reset_queues()
    flush_work(&q->reset_work);

If the hardware faults, apq_reset_check() polls indefinitely in its
infinite while (true) loop. Because the global mdevs_lock is held during
this time, would this permanently block all other operations across all
vfio-ap mediated devices?

> +		if (ret == 0) {
> +			/* Reset migration state per VFIO migration spec */
> +			vfio_ap_reset_migration_state(matrix_mdev);
> +		}
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707090226.3206695-1-akrowiak@linux.ibm.com?part=4