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

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

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.

>