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

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

On 8/12/26 5:12 PM, Matthew Rosato wrote:
> On 8/12/26 4:02 PM, Anthony Krowiak wrote:
>> When an adapter or domain is removed from the host's AP configuration, the
>> AP bus invokes vfio_ap_on_cfg_changed() to notify the vfio_ap device
>> driver. For each ap_matrix_mdev object to which the adapter or domain
>> is assigned, vfio_ap_mdev_hot_unplug_cfg() is called and removes the
>> adapter or domain from the matrix_mdev->shadow_apcb (i.e., the guest's AP
>> configuration) and hot unplugs it if a guest is using it. The new host
>> AP configuration (sans adapter or domain) is then stored in
>> matrix_dev->info.
>>
>> When the AP bus subsequently unbinds the physical queue devices associated
>> with the adapter or domain that has been removed, it invokes
>> vfio_ap_mdev_remove_queue() for each queue removed. At this point, the
>> adapter or domain will no longer be assigned to the
>> matrix_mdev->shadow_apcb or the matrix_dev->info object because they would
>> have been removed by vfio_ap_on_cfg_changed(). Consequently,
>> vfio_ap_mdev_reset_queue(q) is bypassed and kfree(q) is called
>> without executing vfio_ap_free_aqic_resources(). This indefinitely pins
>> guest memory (q->saved_iova) and leaks KVM GISC resources (q->saved_isc).
>>
>> Note that resetting the queue would fail with an invalid APQN error due to
>> the fact the queue is not longer in the host's AP configuration; however,
>> it is still necessary to free the AQIC resources. The fix here is to call
>> vfio_ap_free_aqic_resources if the adapter or domain is neither in
>> matrix_mdev->shadow_apcb or matrix_dev->info.
>>
>> 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>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 1edd0b7a3cce..bd9d239caeba 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2533,12 +2533,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, then 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);
> Re: the Sashiko finding for this one:
>
> [Severity: High]
> Can this still trigger a WARN_ON and leak KVM GISC and pinned memory if
> a guest shuts down or the mdev is removed before the physical queue is
> unbound?
>
> I think Sashiko has a point here...  At first I thought it was a
> pre-existing issue (just another path that fails to free resources) but
> I think this patch actually makes the described thread of execution a
> bit worse:
>
> Before this patch you would leak resources on the described path.
>
> After this patch, you will leak the resources on that path, then reach
> this new else statement and could potentially now WARN about the null
> pointer(s) in vfio_ap_free_aqic_resources() and still not unregister the
> gisc and/or unpin the saved_iova due to the null pointers.

This is a legitimate concern. I will fix this via an individual patch since
this 9/9 patch will not be included in the series for the pull request.

>
>