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

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

On 10/13/26 11:47 AM, sashiko-bot@kernel.org wrote:
> 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.

This is actually a pre-existing bug. Also, the chunk above is unrelated to
this patch series. This will be fixed in a separate patch.

>
>>   	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;
>>   }