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

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 <[email protected]>

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.

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