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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] s390/vfio-ap: Data race on matrix_dev->info during mdev init
--

commit 61bb5cee71d03ac3e82343abe74a853a264da523
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

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

Provides the functions that initialize and release the data structures
used during live guest migration.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 01439f0ca42ab..6829e423b5a46 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -782,18 +782,30 @@ static bool vfio_ap_mdev_filter_matrix(struct ap_matrix_mdev *matrix_mdev,
>  
>  static int vfio_ap_mdev_init_dev(struct vfio_device *vdev)
>  {
> -	struct ap_matrix_mdev *matrix_mdev =
> -		container_of(vdev, struct ap_matrix_mdev, vdev);
> +	struct ap_matrix_mdev *matrix_mdev;
>  
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
>  	matrix_mdev->mdev = to_mdev_device(vdev->dev);
>  	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);

[Severity: Medium]
Could this result in a data race on matrix_dev->info? 

The modified vfio_ap_mdev_init_dev() reads matrix_dev->info via 
vfio_ap_matrix_init() while holding only matrix_dev->mdevs_lock.

However, vfio_ap_on_cfg_changed() updates matrix_dev->info via memcpy() 
while holding only matrix_dev->guests_lock:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_on_cfg_changed() {
    ...
    memcpy(&matrix_dev->info, cur_cfg_info, sizeof(*cur_cfg_info));
    ...
}

Since memcpy() is not atomic and the two functions acquire different locks,
can a concurrent AP configuration change cause vfio_ap_mdev_init_dev()
to read a torn or partially updated ap_config_info struct, potentially
leading to incorrect apm_max, aqm_max, or adm_max values?

Note that follow-up patches in the series do not resolve this lock mismatch.

>  	matrix_mdev->pqap_hook = handle_pqap;
>  	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->shadow_apcb);
>  	hash_init(matrix_mdev->qtable.queues);
> +	mutex_unlock(&matrix_dev->mdevs_lock);
>  
>  	return 0;
>  }

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