Re: [PATCH 5/5] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

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

On 8/8/26 1:30 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Missing KVM guest configuration updates when the last AP device or domain is unplugged due to incorrect usage of `bitmap_andnot` return value.
> --
>
> --- Patch [5]: [PATCH 5/5] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove ---
> commit c11c3bac302d3bc1f0a23db222df493383b0819f
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove
>
> The vfio_ap_config_remove function uses the bitmap_andnot function to clear
> bits from the matrix_mdev->matrix.adm bitmap (specifies the control domains
> assigned to the mdev). This prevents the explicitly unplugged control
> domains from being removed the KVM guest. The bitmap_and function is used
> instead.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 897518b88c281..9c7257c179a7a 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2621,9 +2621,9 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>>   		do_remove |= bitmap_and(aqrem, aq_remove,
>>   					  matrix_mdev->matrix.aqm,
>>   					  AP_DOMAINS);
>> -		do_remove |= bitmap_andnot(cdrem, cd_remove,
>> -					     matrix_mdev->matrix.adm,
>> -					     AP_DOMAINS);
>> +		do_remove |= bitmap_and(cdrem, cd_remove,
>> +					matrix_mdev->matrix.adm,
>> +					AP_DOMAINS);
>>   
>>   		if (do_remove)
>>   			vfio_ap_mdev_hot_unplug_cfg(matrix_mdev, aprem, aqrem,
> [Severity: Critical]
> This is a pre-existing issue, but does the bitwise logic in
> vfio_ap_mdev_hot_unplug_cfg() incorrectly assume bitmap_andnot() returns true
> when bits are cleared?
>
> Looking at vfio_ap_mdev_hot_unplug_cfg(), the return value of bitmap_andnot()
> is used to determine if a hotplug update is necessary:
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_cfg() {
> 	...
> 	if (!bitmap_empty(aprem, AP_DEVICES)) {
> 		do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.apm,
> 					    matrix_mdev->shadow_apcb.apm,
> 					    aprem, AP_DEVICES);
> 	}
> 	...
> 	if (do_hotplug)
> 		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> }
>
> However, bitmap_andnot() returns false when the resulting destination bitmap
> becomes empty.
>
> If a host administrator unplugs the final active AP adapter or domain assigned
> to an mdev, the unplugged bit is cleared from the guest mask, leaving the
> shadow_apcb bitmap entirely empty. Because it is empty, bitmap_andnot()
> returns false.
>
> Will this cause do_hotplug to evaluate to 0 and skip calling
> vfio_ap_mdev_update_guest_apcb() entirely, allowing the KVM guest to retain
> hardware access to unplugged or revoked AP devices? The same pattern is used
> for aqm and adm on subsequent lines.

This is a bug. The author obviously mistook the value returned from 
bitmap_andnot.
The fix will be to check whether the input bitmaps intersect with the 
corresponding
bitmaps in matrix_mdev->shadow_apm/aqm/adm and then do the bitmap_and
only if they intersect and set do_hotplug to true.

>