Re: [PATCH v4 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] The patch introduces potential `WARN_ON` kernel splats and fails to fully fix the resource leak if the mdev is destroyed or the queue is unassigned before the AP bus invokes `vfio_ap_mdev_remove_queue()`.
--

--- Patch [9]: [PATCH v4 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config ---
commit 29bff4de7f99aebfbf379edfd50ba6273390d8d9
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, 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.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index b0454a296c67e..a8de98d5e71ed 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2515,12 +2515,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 bypass the resource cleanup and cause kernel splats if the mdev is 
destroyed or the queue is unassigned before this point?

If userspace unassigns the queue or QEMU closes the mdev, paths like
vfio_ap_mdev_unset_kvm() or unassign_adapter_store() will attempt to reset 
the queue by calling vfio_ap_mdev_reset_queue().

Since the adapter is no longer in the host config, ap_zapq() returns 0x01
(AP_RESPONSE_Q_NOT_AVAIL). However, vfio_ap_mdev_reset_queue() does not handle
this response code in its switch statement. This bypasses the synchronous
cleanup in vfio_ap_free_aqic_resources(), leaving q->saved_isc and
q->saved_iova valid.

The cleanup routines then proceed to clear q->matrix_mdev or
q->matrix_mdev->kvm.

When the AP bus later asynchronously calls vfio_ap_mdev_remove_queue(), this
newly added else block is reached.

Inside vfio_ap_free_aqic_resources():

	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
	    !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) {
		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
		...
	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
		vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);

Because q->matrix_mdev or q->matrix_mdev->kvm is now NULL, the WARN_ON checks
evaluate to true. This triggers a kernel splat and bypasses the actual cleanup
calls, leaking the KVM GISC resources and pinned pages.

>  	}
>  
>  done:

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