Re: [PATCH v4 7/9] s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain removed

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

Pre-existing issues:
- [High] Pinned NIB pages and KVM ISC registrations are permanently leaked when an AP queue is removed from the host configuration.
--

--- Patch [7]: [PATCH v4 7/9] s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain removed ---
commit b0a12cab04822e364a31d2095615141da73872b3
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain removed

This commit fixes an issue where the guest APCB is not updated when the
last AP adapter or domain is removed. It replaces the bitmap_andnot check
with bitmap_intersects to properly detect overlaps and trigger the guest
update.

Cc: [email protected]

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2568,24 +2568,28 @@ static void vfio_ap_mdev_hot_unplug_cfg(struct ap_matrix_mdev *matrix_mdev,
[ ... ]
> +	if (bitmap_intersects(matrix_mdev->shadow_apcb.adm, cdrem, AP_DOMAINS)) {
> +		bitmap_andnot(matrix_mdev->shadow_apcb.adm,
> +			      matrix_mdev->shadow_apcb.adm,
> +			      cdrem, AP_DOMAINS);
> +		do_hotplug = true;
> +	}
>  
>  	if (do_hotplug)
>  		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
>  }

[Severity: High]
This is a pre-existing issue, but while reviewing the hot-unplug path I
noticed a potential resource leak in vfio_ap_mdev_remove_queue().

When an AP adapter or domain is hot-unplugged, it is removed from
matrix_dev->info during the configuration change. Later, when the AP bus
calls vfio_ap_mdev_remove_queue(), it checks the matrix_dev->info bitmaps:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_remove_queue() {
        ...
        if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
            test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
                vfio_ap_mdev_reset_queue(q);
                flush_work(&q->reset_work);
        }

done:
        if (matrix_mdev)
                vfio_ap_unlink_queue_fr_mdev(q);

        dev_set_drvdata(&apdev->device, NULL);
        kfree(q);
}

Because the queue is already removed from matrix_dev->info by the time this
is called, the test_bit_inv() checks evaluate to false.

Does this bypass vfio_ap_mdev_reset_queue() and permanently leak AQIC
resources like NIB pinned pages and KVM ISC registrations before the queue
is freed?

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