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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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.