[PATCH] s390/vfio-ap: Fix leak of KVM GISC resources

[PATCH] s390/vfio-ap: Fix leak of KVM GISC resources

[Anthony Krowiak]

s390/vfio-ap: fix KVM GISC and page leak when queue removed from host config

Two related problems exist in the handling of KVM interrupt and page
resources when a queue is removed from the host's AP configuration
while assigned to a mediated device (mdev).

Problem 1: AP_RESPONSE_Q_NOT_AVAIL not handled in
vfio_ap_mdev_reset_queue()

When the AP bus removes a queue device whose adapter or domain has
been removed from the host's AP configuration,
vfio_ap_mdev_remove_queue() is called. If the queue is still in the
host's AP configuration at that point, it calls
vfio_ap_mdev_reset_queue(), which issues a PQAP(ZAPQ). Since the
adapter is already gone from the host configuration, ap_zapq() returns
AP_RESPONSE_Q_NOT_AVAIL (0x01). This response code is not handled in
vfio_ap_mdev_reset_queue()'s switch statement and falls through to
the default case, which issues a WARN but does not call
vfio_ap_free_aqic_resources(). As a result, if IRQ handling was
enabled for the queue by the guest, the KVM GISC registration and
the pinned guest page holding the notification indicator byte (NIB)
are both leaked.

This is fixed by adding AP_RESPONSE_Q_NOT_AVAIL to the same case as
AP_RESPONSE_DECONFIGURED and AP_RESPONSE_CHECKSTOPPED in
vfio_ap_mdev_reset_queue(). Like those response codes, Q_NOT_AVAIL
indicates the queue is not operational and no further reset attempts
are possible; the correct action is to free the IRQ resources
immediately.

Problem 2: vfio_ap_free_aqic_resources() leaks saved_isc when
kvm is NULL

vfio_ap_free_aqic_resources() guards the call to
kvm_s390_gisc_unregister() with:

    if (q->saved_isc != VFIO_AP_ISC_INVALID &&
        !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm)))

If matrix_mdev->kvm is NULL -- which can happen when
vfio_ap_mdev_unset_kvm() has already run and cleared kvm before a
subsequent cleanup path reaches this function -- the WARN_ON fires
and the entire block is skipped. This leaves q->saved_isc set to a
non-invalid value, creating a potential double-free on any subsequent
call to this function.

When kvm is NULL the KVM guest is already torn down, so
kvm_s390_gisc_unregister() need not and cannot be called; however,
q->saved_isc must always be cleared. Fix this by separating the
kvm_s390_gisc_unregister() call from the q->saved_isc reset. The
WARN_ON now guards only the genuinely impossible case of matrix_mdev
being NULL. A NULL kvm is handled gracefully by skipping only the
unregister call, and q->saved_isc = VFIO_AP_ISC_INVALID is set
unconditionally whenever saved_isc was not already invalid.

Additionally, add an else clause to the host-config check in
vfio_ap_mdev_remove_queue() to call vfio_ap_free_aqic_resources()
directly when the queue is not in the host's AP configuration. This
serves as a backstop: when the AP bus fires the driver .remove
callback after an adapter is removed from the host config, the queue
is by definition no longer addressable, so vfio_ap_mdev_reset_queue()
would always return Q_NOT_AVAIL. The else clause handles this case
directly without the unnecessary ap_zapq() call, and ensures cleanup
occurs even if kvm has already been set to NULL by a prior call to
vfio_ap_mdev_unset_kvm().

Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 44b3a1dcc1b3..e277062e92be 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -270,9 +270,9 @@ static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
 {
 	if (!q)
 		return;
-	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
-	    !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) {
-		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
+	if (q->saved_isc != VFIO_AP_ISC_INVALID) {
+		if (!WARN_ON(!q->matrix_mdev) && q->matrix_mdev->kvm)
+			kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
 		q->saved_isc = VFIO_AP_ISC_INVALID;
 	}
 	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
@@ -2004,6 +2004,7 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
 		break;
 	case AP_RESPONSE_DECONFIGURED:
 	case AP_RESPONSE_CHECKSTOPPED:
+	case AP_RESPONSE_Q_NOT_AVAIL:
 		vfio_ap_free_aqic_resources(q);
 		break;
 	default:
@@ -2499,12 +2500,15 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
 	/*
 	 * If the queue is not in the host's AP configuration, then resetting
 	 * it will fail with response code 01, (APQN not valid); so, let's make
-	 * sure it is in the host's config.
+	 * sure it is in the host's config. If it is not, free the KVM GISC
+	 * resources.
 	 */
 	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);
+	} else {
+		vfio_ap_free_aqic_resources(q);
 	}
 
 done:
-- 
2.53.0

Re: [PATCH] s390/vfio-ap: Fix leak of KVM GISC resources

[Matthew Rosato]

On 8/18/26 7:58 AM, Anthony Krowiak wrote:
> s390/vfio-ap: fix KVM GISC and page leak when queue removed from host config
> 
> Two related problems exist in the handling of KVM interrupt and page
> resources when a queue is removed from the host's AP configuration
> while assigned to a mediated device (mdev).
> 
> Problem 1: AP_RESPONSE_Q_NOT_AVAIL not handled in
> vfio_ap_mdev_reset_queue()
> 
> When the AP bus removes a queue device whose adapter or domain has
> been removed from the host's AP configuration,
> vfio_ap_mdev_remove_queue() is called. If the queue is still in the
> host's AP configuration at that point, it calls
> vfio_ap_mdev_reset_queue(), which issues a PQAP(ZAPQ). Since the
> adapter is already gone from the host configuration, ap_zapq() returns
> AP_RESPONSE_Q_NOT_AVAIL (0x01). This response code is not handled in
> vfio_ap_mdev_reset_queue()'s switch statement and falls through to
> the default case, which issues a WARN but does not call
> vfio_ap_free_aqic_resources(). As a result, if IRQ handling was
> enabled for the queue by the guest, the KVM GISC registration and
> the pinned guest page holding the notification indicator byte (NIB)
> are both leaked.
> 
> This is fixed by adding AP_RESPONSE_Q_NOT_AVAIL to the same case as
> AP_RESPONSE_DECONFIGURED and AP_RESPONSE_CHECKSTOPPED in
> vfio_ap_mdev_reset_queue(). Like those response codes, Q_NOT_AVAIL
> indicates the queue is not operational and no further reset attempts
> are possible; the correct action is to free the IRQ resources
> immediately.
> 
> Problem 2: vfio_ap_free_aqic_resources() leaks saved_isc when
> kvm is NULL
> 
> vfio_ap_free_aqic_resources() guards the call to
> kvm_s390_gisc_unregister() with:
> 
>     if (q->saved_isc != VFIO_AP_ISC_INVALID &&
>         !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm)))
> 
> If matrix_mdev->kvm is NULL -- which can happen when
> vfio_ap_mdev_unset_kvm() has already run and cleared kvm before a
> subsequent cleanup path reaches this function -- the WARN_ON fires
> and the entire block is skipped. This leaves q->saved_isc set to a
> non-invalid value, creating a potential double-free on any subsequent
> call to this function.
> 
> When kvm is NULL the KVM guest is already torn down, so
> kvm_s390_gisc_unregister() need not and cannot be called; however,
> q->saved_isc must always be cleared. Fix this by separating the
> kvm_s390_gisc_unregister() call from the q->saved_isc reset. The
> WARN_ON now guards only the genuinely impossible case of matrix_mdev
> being NULL. A NULL kvm is handled gracefully by skipping only the
> unregister call, and q->saved_isc = VFIO_AP_ISC_INVALID is set
> unconditionally whenever saved_isc was not already invalid.
> 
> Additionally, add an else clause to the host-config check in
> vfio_ap_mdev_remove_queue() to call vfio_ap_free_aqic_resources()
> directly when the queue is not in the host's AP configuration. This
> serves as a backstop: when the AP bus fires the driver .remove
> callback after an adapter is removed from the host config, the queue
> is by definition no longer addressable, so vfio_ap_mdev_reset_queue()
> would always return Q_NOT_AVAIL. The else clause handles this case
> directly without the unnecessary ap_zapq() call, and ensures cleanup
> occurs even if kvm has already been set to NULL by a prior call to
> vfio_ap_mdev_unset_kvm().
> 
> Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
> Cc: stable@vger.kernel.org
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

For archive purposes, this is a continuation of 

[PATCH v5 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config

https://lore.kernel.org/linux-s390/20260812200240.818004-10-akrowiak@linux.ibm.com/

Re: [PATCH] s390/vfio-ap: Fix leak of KVM GISC resources

[Anthony Krowiak]

On 8/18/26 1:32 PM, Matthew Rosato wrote:
> On 8/18/26 7:58 AM, Anthony Krowiak wrote:
>> s390/vfio-ap: fix KVM GISC and page leak when queue removed from host config
>>
>> Two related problems exist in the handling of KVM interrupt and page
>> resources when a queue is removed from the host's AP configuration
>> while assigned to a mediated device (mdev).
>>
>> Problem 1: AP_RESPONSE_Q_NOT_AVAIL not handled in
>> vfio_ap_mdev_reset_queue()
>>
>> When the AP bus removes a queue device whose adapter or domain has
>> been removed from the host's AP configuration,
>> vfio_ap_mdev_remove_queue() is called. If the queue is still in the
>> host's AP configuration at that point, it calls
>> vfio_ap_mdev_reset_queue(), which issues a PQAP(ZAPQ). Since the
>> adapter is already gone from the host configuration, ap_zapq() returns
>> AP_RESPONSE_Q_NOT_AVAIL (0x01). This response code is not handled in
>> vfio_ap_mdev_reset_queue()'s switch statement and falls through to
>> the default case, which issues a WARN but does not call
>> vfio_ap_free_aqic_resources(). As a result, if IRQ handling was
>> enabled for the queue by the guest, the KVM GISC registration and
>> the pinned guest page holding the notification indicator byte (NIB)
>> are both leaked.
>>
>> This is fixed by adding AP_RESPONSE_Q_NOT_AVAIL to the same case as
>> AP_RESPONSE_DECONFIGURED and AP_RESPONSE_CHECKSTOPPED in
>> vfio_ap_mdev_reset_queue(). Like those response codes, Q_NOT_AVAIL
>> indicates the queue is not operational and no further reset attempts
>> are possible; the correct action is to free the IRQ resources
>> immediately.
>>
>> Problem 2: vfio_ap_free_aqic_resources() leaks saved_isc when
>> kvm is NULL
>>
>> vfio_ap_free_aqic_resources() guards the call to
>> kvm_s390_gisc_unregister() with:
>>
>>      if (q->saved_isc != VFIO_AP_ISC_INVALID &&
>>          !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm)))
>>
>> If matrix_mdev->kvm is NULL -- which can happen when
>> vfio_ap_mdev_unset_kvm() has already run and cleared kvm before a
>> subsequent cleanup path reaches this function -- the WARN_ON fires
>> and the entire block is skipped. This leaves q->saved_isc set to a
>> non-invalid value, creating a potential double-free on any subsequent
>> call to this function.
>>
>> When kvm is NULL the KVM guest is already torn down, so
>> kvm_s390_gisc_unregister() need not and cannot be called; however,
>> q->saved_isc must always be cleared. Fix this by separating the
>> kvm_s390_gisc_unregister() call from the q->saved_isc reset. The
>> WARN_ON now guards only the genuinely impossible case of matrix_mdev
>> being NULL. A NULL kvm is handled gracefully by skipping only the
>> unregister call, and q->saved_isc = VFIO_AP_ISC_INVALID is set
>> unconditionally whenever saved_isc was not already invalid.
>>
>> Additionally, add an else clause to the host-config check in
>> vfio_ap_mdev_remove_queue() to call vfio_ap_free_aqic_resources()
>> directly when the queue is not in the host's AP configuration. This
>> serves as a backstop: when the AP bus fires the driver .remove
>> callback after an adapter is removed from the host config, the queue
>> is by definition no longer addressable, so vfio_ap_mdev_reset_queue()
>> would always return Q_NOT_AVAIL. The else clause handles this case
>> directly without the unnecessary ap_zapq() call, and ensures cleanup
>> occurs even if kvm has already been set to NULL by a prior call to
>> vfio_ap_mdev_unset_kvm().
>>
>> Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> ---
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>
> For archive purposes, this is a continuation of
>
> [PATCH v5 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config
>
> https://lore.kernel.org/linux-s390/20260812200240.818004-10-akrowiak@linux.ibm.com/

I meant to version this one, but it slipped my mind by the time I created
this patch.

>

Re: [PATCH] s390/vfio-ap: Fix leak of KVM GISC resources

[Anthony Krowiak]

On 8/18/26 1:32 PM, Matthew Rosato wrote:
> On 8/18/26 7:58 AM, Anthony Krowiak wrote:
>> s390/vfio-ap: fix KVM GISC and page leak when queue removed from host config
>>
>> Two related problems exist in the handling of KVM interrupt and page
>> resources when a queue is removed from the host's AP configuration
>> while assigned to a mediated device (mdev).
>>
>> Problem 1: AP_RESPONSE_Q_NOT_AVAIL not handled in
>> vfio_ap_mdev_reset_queue()
>>
>> When the AP bus removes a queue device whose adapter or domain has
>> been removed from the host's AP configuration,
>> vfio_ap_mdev_remove_queue() is called. If the queue is still in the
>> host's AP configuration at that point, it calls
>> vfio_ap_mdev_reset_queue(), which issues a PQAP(ZAPQ). Since the
>> adapter is already gone from the host configuration, ap_zapq() returns
>> AP_RESPONSE_Q_NOT_AVAIL (0x01). This response code is not handled in
>> vfio_ap_mdev_reset_queue()'s switch statement and falls through to
>> the default case, which issues a WARN but does not call
>> vfio_ap_free_aqic_resources(). As a result, if IRQ handling was
>> enabled for the queue by the guest, the KVM GISC registration and
>> the pinned guest page holding the notification indicator byte (NIB)
>> are both leaked.
>>
>> This is fixed by adding AP_RESPONSE_Q_NOT_AVAIL to the same case as
>> AP_RESPONSE_DECONFIGURED and AP_RESPONSE_CHECKSTOPPED in
>> vfio_ap_mdev_reset_queue(). Like those response codes, Q_NOT_AVAIL
>> indicates the queue is not operational and no further reset attempts
>> are possible; the correct action is to free the IRQ resources
>> immediately.
>>
>> Problem 2: vfio_ap_free_aqic_resources() leaks saved_isc when
>> kvm is NULL
>>
>> vfio_ap_free_aqic_resources() guards the call to
>> kvm_s390_gisc_unregister() with:
>>
>>      if (q->saved_isc != VFIO_AP_ISC_INVALID &&
>>          !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm)))
>>
>> If matrix_mdev->kvm is NULL -- which can happen when
>> vfio_ap_mdev_unset_kvm() has already run and cleared kvm before a
>> subsequent cleanup path reaches this function -- the WARN_ON fires
>> and the entire block is skipped. This leaves q->saved_isc set to a
>> non-invalid value, creating a potential double-free on any subsequent
>> call to this function.
>>
>> When kvm is NULL the KVM guest is already torn down, so
>> kvm_s390_gisc_unregister() need not and cannot be called; however,
>> q->saved_isc must always be cleared. Fix this by separating the
>> kvm_s390_gisc_unregister() call from the q->saved_isc reset. The
>> WARN_ON now guards only the genuinely impossible case of matrix_mdev
>> being NULL. A NULL kvm is handled gracefully by skipping only the
>> unregister call, and q->saved_isc = VFIO_AP_ISC_INVALID is set
>> unconditionally whenever saved_isc was not already invalid.
>>
>> Additionally, add an else clause to the host-config check in
>> vfio_ap_mdev_remove_queue() to call vfio_ap_free_aqic_resources()
>> directly when the queue is not in the host's AP configuration. This
>> serves as a backstop: when the AP bus fires the driver .remove
>> callback after an adapter is removed from the host config, the queue
>> is by definition no longer addressable, so vfio_ap_mdev_reset_queue()
>> would always return Q_NOT_AVAIL. The else clause handles this case
>> directly without the unnecessary ap_zapq() call, and ensures cleanup
>> occurs even if kvm has already been set to NULL by a prior call to
>> vfio_ap_mdev_unset_kvm().
>>
>> Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> ---
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>
> For archive purposes, this is a continuation of
>
> [PATCH v5 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config
>
> https://lore.kernel.org/linux-s390/20260812200240.818004-10-akrowiak@linux.ibm.com/

I just noticed that I had a commit editor open for this patch when I got 
back
from an appointment for which I had to run out the door. I added a case for
the AP_RESPONSE_Q_NOT_AVAIL  to apq_reset_check() for the same reason
a case was added to vfio_ap_mdev_reset_queue(). When 
vfio_ap_mdev_reset_queue()
has to wait for an asynch reset to complete, it queues the 
apq_status_check()
function to a work queue. That function calls apq_reset_check() to check 
the
status response code which also did not check for AP_RESPONSE_Q_NOT_AVAIL
and leaked AQIC resources. That patch is forthcoming today.

>