Re: [PATCH v5 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:
- [High] Incomplete fix for resource leak: `vfio_ap_mdev_reset_queue()` does not handle `AP_RESPONSE_Q_NOT_AVAIL` (0x01), causing KVM GISC/pinned memory leaks and spurious WARN_ON stack traces.
--

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

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

This patch aims to fix a memory leak of KVM GISC resources and pinned guest
memory when an adapter or domain is removed from the host's AP configuration.
It bypasses the queue reset and directly frees the AQIC resources if the
queue is no longer in the host config.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 1edd0b7a3cce7..bd9d239caeba2 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);
[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?

When an adapter is removed from the host config, ap_zapq() returns 0x01
(AP_RESPONSE_Q_NOT_AVAIL). If vfio_ap_mdev_reset_queue() receives this response
(for example, during guest shutdown), it falls into the default case, logs a
WARN, and skips freeing AQIC resources:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_reset_queue() {
	...
	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);
	...
}

The driver might then clear the kvm or q->matrix_mdev pointers. Later, when
the physical queue is unbound and this else branch is reached,
vfio_ap_free_aqic_resources(q) would hit the WARN_ON checks because the
pointers are now NULL:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_free_aqic_resources() {
	...
	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
	    !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) {
	...
	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
	...
}

Should vfio_ap_mdev_reset_queue() be updated to explicitly handle response
code 0x01 and clean up resources there as well to ensure they are freed
before the pointers are cleared?

>  	}
>  
>  done:

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