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

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

Pre-existing issues:
- [High] Software resources (KVM GISC registration and pinned guest memory) are permanently leaked if hardware reset fails unexpectedly.
--

--- Patch [9]: [PATCH v3 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config ---
commit b591517cf03c156d327043d4900cb31e47ff080b
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix memory leak when queue removed from host AP config

When an adapter or domain is removed from the host's AP configuration,
vfio_ap_mdev_remove_queue() can bypass resetting the queue. This patch fixes
a leak of KVM GISC resources and pinned guest memory by calling
vfio_ap_free_aqic_resources() if the adapter or domain is no longer in the
matrix configuration.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 3652ba792be73..faab5e9179ff1 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2507,12 +2507,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);

[Severity: High]
This is a pre-existing issue, but does vfio_ap_mdev_reset_queue() leak
software resources (KVM GISC registration and pinned guest memory) if 
hardware reset fails unexpectedly?

In vfio_ap_mdev_reset_queue(), if ap_zapq() returns an unexpected response
code, the default case is hit:

	default:
		WARN(true,
		     "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
		     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
		     status.response_code);
	}

This default path returns without queuing the reset work or calling
vfio_ap_free_aqic_resources().

A similar situation occurs in the asynchronous worker thread apq_reset_check()
if ap_tapq() returns an unexpected response code:

		ret = apq_status_check(q->apqn, &status);
		if (ret == -EIO)
			return;

Since vfio_ap_mdev_remove_queue() continues on to call kfree(q) regardless of
the outcome of vfio_ap_mdev_reset_queue():

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

Are these resources permanently lost when this happens?

>  		flush_work(&q->reset_work);
> +	} else {
> +		vfio_ap_free_aqic_resources(q);
>  	}
>  
>  done:

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